[PATCH] D74813: [RFC] Add hash of block contents to function block names

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/lib/AST/Mangle.cpp:41
+  for (ParmVarDecl *PVD : BD->parameters()) {
+    Context.mangleTypeName(PVD->getType(), Out);
+  }
----------------
>>! In D74813#1996143, @alexbdv wrote:
> @dexonsmith, @erik.pilkington - how about this ? 
> 
> ____Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss
> 

That doesn't look quite right, `_ZTSi` means the typeinfo symbol for int, but we really should just be printing the type, `i`. I think we should add a new member on MangleContext (in Mangle.h) for mangling block invocation functions, then implement it in ItaniumMangle.cpp to call `CXXNameMangler::mangleType` for each parameter (the MS mangling can probably just be left as-is, I don't think it matters all that much for blocks).


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:5537
+
+      paramType->print(ParamOS);
+    }
----------------
alexbdv wrote:
> erik.pilkington wrote:
> > Can you make a special BlockInvocationFunction AST node that stores the parameter types as AST nodes (rather than strings) in a NodeArray? Right now it looks like this code is leaking, since initializeOutputStream allocates a buffer that it expects the user of OutputStream to manage (the ownership is a bit awkward, but its meant to accommodate the API of `__cxa_demangle`). 
> Resolved with better memory management.
Okay, but now this is just leaking in the buffer you allocate in DescOS. There isn't any way for this approach to work, because you need the temporary buffer you're allocating to live until after the SpecialName is printed, and the AST nodes do not get destroyed. Can you please do what I said above, and create a BlockInvocationFunction node that has a NodeArray of parameters? You can look at e.g. `FunctionType` above for an example of this. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74813/new/

https://reviews.llvm.org/D74813