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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv marked an inline comment as done.
alexbdv added inline comments.



Comment at: clang/lib/AST/Mangle.cpp:56
+
+  // Strip out addresses
+  char *ptr = &strStmtBuff[1];

manmanren wrote:
> Is this needed to have deterministic behavior?
Correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith - any suggestions to move forward on this ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 257508.

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

https://reviews.llvm.org/D74813

Files:
  clang/include/clang/AST/Mangle.h
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1173,7 +1173,8 @@
   void AddDefaultFnAttrs(llvm::Function &F);
 
   StringRef getMangledName(GlobalDecl GD);
-  StringRef getBlockMangledName(GlobalDecl GD, const BlockDecl *BD);
+  StringRef getBlockMangledName(GlobalDecl GD, const BlockDecl *BD,
+const StringRef *parentFuncName);
 
   void EmitTentativeDefinition(const VarDecl *D);
 
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1177,7 +1177,8 @@
 }
 
 StringRef CodeGenModule::getBlockMangledName(GlobalDecl GD,
- const BlockDecl *BD) {
+ const BlockDecl *BD,
+ const StringRef *parentFuncName) {
   MangleContext &MangleCtx = getCXXABI().getMangleContext();
   const Decl *D = GD.getDecl();
 
@@ -1187,11 +1188,11 @@
 MangleCtx.mangleGlobalBlock(BD,
   dyn_cast_or_null(initializedGlobalDecl.getDecl()), Out);
   else if (const auto *CD = dyn_cast(D))
-MangleCtx.mangleCtorBlock(CD, GD.getCtorType(), BD, Out);
+MangleCtx.mangleCtorBlock(CD, GD.getCtorType(), BD, Out, parentFuncName);
   else if (const auto *DD = dyn_cast(D))
-MangleCtx.mangleDtorBlock(DD, GD.getDtorType(), BD, Out);
+MangleCtx.mangleDtorBlock(DD, GD.getDtorType(), BD, Out, parentFuncName);
   else
-MangleCtx.mangleBlock(cast(D), BD, Out);
+MangleCtx.mangleBlock(cast(D), BD, Out, parentFuncName);
 
   auto Result = Manglings.insert(std::make_pair(Out.str(), BD));
   return Result.first->first();
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -212,7 +212,8 @@
   if (const auto *FD = dyn_cast(DC))
 ContextName = std::string(CGM.getMangledName(FD));
   else if (const auto *BD = dyn_cast(DC))
-ContextName = std::string(CGM.getBlockMangledName(GlobalDecl(), BD));
+ContextName =
+std::string(CGM.getBlockMangledName(GlobalDecl(), BD, nullptr));
   else if (const auto *OMD = dyn_cast(DC))
 ContextName = OMD->getSelector().getAsString();
   else
Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -1571,7 +1571,7 @@
 
   llvm::FunctionType *fnLLVMType = CGM.getTypes().GetFunctionType(fnInfo);
 
-  StringRef name = CGM.getBlockMangledName(GD, blockDecl);
+  StringRef name = CGM.getBlockMangledName(GD, blockDecl, &blockInfo.Name);
   llvm::Function *fn = llvm::Function::Create(
   fnLLVMType, llvm::GlobalValue::InternalLinkage, name, &CGM.getModule());
   CGM.SetInternalFunctionAttributes(blockDecl, fn, fnInfo);
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -26,21 +26,70 @@
 #include "llvm/IR/Mangler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+#include 
 
 using namespace clang;
 
+static std::string getBlockDeclHash(const BlockDecl *BD, 
+const StringRef *parentFuncName) {
+  std::vector hashItems;
+
+  ArrayRef params = BD->parameters();
+  std::string strTypeBuff;
+  llvm::raw_string_ostream osTypeStream(strTypeBuff);
+
+  hashItems.push_back(params.size());
+  for(unsigned i = 0; i < params.size(); i++) {
+ParmVarDecl *param = params[i];
+
+osTypeStream << param->getNameAsString();
+osTypeStream << " [";
+param->getType()->dump(osTypeStream);
+osTypeStream << "] \n";
+  }
+
+  osTypeStream.flush();
+
+  // Remove (non-deterministic) pointers from the param string
+  char *ptr = &strTypeBuff[1];
+  while (*ptr) {
+if (*ptr == 'x' && *(ptr - 1) == '0') {
+  ptr++;
+  while ((*ptr >= '0' && *ptr <= '9') || (*ptr >= 'a' && *ptr <= 'f')) {
+*ptr = '_';
+ptr++;
+  }
+  continue;
+}
+ptr++;
+  }
+
+  // Hash the param string
+  llvm::MD5 Hash;
+  llvm::MD5::MD5Result Result;
+
+  if(parentFuncName) {
+Hash.update(*parentFuncName);
+  }
+  Hash.update(strTypeBuff);
+  Hash.final(Result);
+
+  std::ostringstream osRet;
+  unsigned short hash = (*(unsigned short *)&Result);
+
+  osRet << st

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@erik.pilkington / @vsk / @dexonsmith - how block name = 
hash(parent_function_name + block_type) ? 
So any blocks that are inside the same parent function + have the same type 
will get an incremental number to de-duplicate names.


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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 257616.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp


Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -36,11 +36,14 @@
 StringRef Outer,
 const BlockDecl *BD,
 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-Out << "__" << Outer << "_block_invoke";
-  else
-Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  Out << "__" << Outer << "_block_invoke__";
+
+  ArrayRef params = BD->parameters();
+  for(unsigned i = 0; i < params.size(); i++) {
+ParmVarDecl *param = params[i];
+Out << "_";
+Context.mangleTypeName(param->getType(), Out);
+  }
 }
 
 void MangleContext::anchor() { }


Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -36,11 +36,14 @@
 StringRef Outer,
 const BlockDecl *BD,
 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-Out << "__" << Outer << "_block_invoke";
-  else
-Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  Out << "__" << Outer << "_block_invoke__";
+
+  ArrayRef params = BD->parameters();
+  for(unsigned i = 0; i < params.size(); i++) {
+ParmVarDecl *param = params[i];
+Out << "_";
+Context.mangleTypeName(param->getType(), Out);
+  }
 }
 
 void MangleContext::anchor() { }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith - I think that should work - like this ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith, @erik.pilkington - how about this ?

Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss

demangled as:

invocation function for block with params '(int, unsigned int, sss const 
volatile*)' in my_main()


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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 259181.

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp
  llvm/include/llvm/Demangle/ItaniumDemangle.h


Index: llvm/include/llvm/Demangle/ItaniumDemangle.h
===
--- llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -5524,14 +5524,35 @@
 Node *Encoding = getDerived().parseEncoding();
 if (Encoding == nullptr || !consumeIf("_block_invoke"))
   return nullptr;
-bool RequireNumber = consumeIf('_');
-if (parseNumber().empty() && RequireNumber)
-  return nullptr;
+
+OutputStream ParamOS;
+if (!initializeOutputStream(nullptr, nullptr, ParamOS, 1024)) {
+  std::terminate();
+}
+
+while (consumeIf("_ZTS")) {
+  Node *paramType = getDerived().parseType();
+  ParamOS += ParamOS.empty() ? "(" : ", ";
+
+  paramType->print(ParamOS);
+}
+if (!ParamOS.empty()) {
+  ParamOS += ")";
+}
+
 if (look() == '.')
   First = Last;
 if (numLeft() != 0)
   return nullptr;
-return make("invocation function for block in ", Encoding);
+
+OutputStream DescOS;
+if (!initializeOutputStream(nullptr, nullptr, DescOS, 1024)) {
+  std::terminate();
+}
+DescOS += "invocation function for block with params '";
+DescOS += ParamOS.getBuffer();
+DescOS += "' in ";
+return make(DescOS.getBuffer(), Encoding);
   }
 
   Node *Ty = getDerived().parseType();
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -36,11 +36,14 @@
 StringRef Outer,
 const BlockDecl *BD,
 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-Out << "__" << Outer << "_block_invoke";
-  else
-Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  Out << "__" << Outer << "_block_invoke";
+
+  ArrayRef params = BD->parameters();
+  for(unsigned i = 0; i < params.size(); i++) {
+ParmVarDecl *param = params[i];
+Context.mangleTypeName(param->getType(), Out);
+  }
+  llvm::raw_svector_ostream *Out2 = (llvm::raw_svector_ostream*)&Out;
 }
 
 void MangleContext::anchor() { }


Index: llvm/include/llvm/Demangle/ItaniumDemangle.h
===
--- llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -5524,14 +5524,35 @@
 Node *Encoding = getDerived().parseEncoding();
 if (Encoding == nullptr || !consumeIf("_block_invoke"))
   return nullptr;
-bool RequireNumber = consumeIf('_');
-if (parseNumber().empty() && RequireNumber)
-  return nullptr;
+
+OutputStream ParamOS;
+if (!initializeOutputStream(nullptr, nullptr, ParamOS, 1024)) {
+  std::terminate();
+}
+
+while (consumeIf("_ZTS")) {
+  Node *paramType = getDerived().parseType();
+  ParamOS += ParamOS.empty() ? "(" : ", ";
+
+  paramType->print(ParamOS);
+}
+if (!ParamOS.empty()) {
+  ParamOS += ")";
+}
+
 if (look() == '.')
   First = Last;
 if (numLeft() != 0)
   return nullptr;
-return make("invocation function for block in ", Encoding);
+
+OutputStream DescOS;
+if (!initializeOutputStream(nullptr, nullptr, DescOS, 1024)) {
+  std::terminate();
+}
+DescOS += "invocation function for block with params '";
+DescOS += ParamOS.getBuffer();
+DescOS += "' in ";
+return make(DescOS.getBuffer(), Encoding);
   }
 
   Node *Ty = getDerived().parseType();
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -36,11 +36,14 @@
 StringRef Outer,
 const BlockDecl *BD,
 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-Out << "__" << Outer << "_block_invoke";
-  else
-Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  Out << "__" << Outer << "_block_invoke";
+
+  ArrayRef params = BD->parameters();
+  for(unsigned i = 0; i < params.size(); i++) {
+ParmVarDecl *param = params[i];
+Context.mangleTypeName(param->getType(), Out);
+  }
+  llvm::raw_svector_ostream *Out2 = (llvm::raw_svector_ostream*)&Out;
 }
 
 void MangleContext::anchor() { }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 259487.
Herald added a reviewer: jhenderson.
Herald added a project: libc++abi.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++abi.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp
  clang/test/CodeGen/block-with-perdefinedexpr.c
  clang/test/CodeGen/block-with-perdefinedexpr.cpp
  clang/test/CodeGen/blockwithlocalstatic.c
  libcxxabi/src/demangle/ItaniumDemangle.h
  llvm/include/llvm/Demangle/ItaniumDemangle.h
  llvm/test/tools/llvm-cxxfilt/invalid.test
  llvm/unittests/Demangle/DemangleTest.cpp

Index: llvm/unittests/Demangle/DemangleTest.cpp
===
--- llvm/unittests/Demangle/DemangleTest.cpp
+++ llvm/unittests/Demangle/DemangleTest.cpp
@@ -16,9 +16,11 @@
   EXPECT_EQ(demangle("_Z3fooi"), "foo(int)");
   EXPECT_EQ(demangle("__Z3fooi"), "foo(int)");
   EXPECT_EQ(demangle("___Z3fooi_block_invoke"),
-"invocation function for block in foo(int)");
+"invocation function for block with params '()' in foo(int)");
   EXPECT_EQ(demangle("Z3fooi_block_invoke"),
-"invocation function for block in foo(int)");
+"invocation function for block with params '()' in foo(int)");
+  EXPECT_EQ(demangle("Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss"),
+"invocation function for block with params '(int, unsigned int, sss const volatile*)' in my_main()");
   EXPECT_EQ(demangle("?foo@@YAXH@Z"), "void __cdecl foo(int)");
   EXPECT_EQ(demangle("foo"), "foo");
 }
Index: llvm/test/tools/llvm-cxxfilt/invalid.test
===
--- llvm/test/tools/llvm-cxxfilt/invalid.test
+++ llvm/test/tools/llvm-cxxfilt/invalid.test
@@ -1,6 +1,7 @@
-RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke | FileCheck %s
+RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke ___Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss | FileCheck %s
 
 CHECK: f(int)
 CHECK-NEXT: __Z1fi
 CHECK-NEXT: f
-CHECK-NEXT: invocation function for block in std::f(float)
+CHECK-NEXT: invocation function for block with params '()' in std::f(float)
+CHECK-NEXT: invocation function for block with params '(int, unsigned int, sss const volatile*)' in my_main()
Index: llvm/include/llvm/Demangle/ItaniumDemangle.h
===
--- llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 #define FOR_EACH_NODE_KIND(X) \
@@ -5503,8 +5505,7 @@
 //  ::= _Z 
 //::= 
 // extension  ::= ___Z  _block_invoke
-// extension  ::= ___Z  _block_invoke+
-// extension  ::= ___Z  _block_invoke_+
+// extension  ::= ___Z  _block_invoke+
 template 
 Node *AbstractManglingParser::parse() {
   if (consumeIf("_Z") || consumeIf("__Z")) {
@@ -5524,14 +5525,39 @@
 Node *Encoding = getDerived().parseEncoding();
 if (Encoding == nullptr || !consumeIf("_block_invoke"))
   return nullptr;
-bool RequireNumber = consumeIf('_');
-if (parseNumber().empty() && RequireNumber)
-  return nullptr;
+
+OutputStream ParamOS;
+if (!initializeOutputStream(nullptr, nullptr, ParamOS, 1024)) {
+  std::terminate();
+}
+
+ParamOS += "(";
+while (consumeIf("_ZTS")) {
+  Node *paramType = getDerived().parseType();
+  if (ParamOS.back() != '(')
+ParamOS += ", ";
+
+  paramType->print(ParamOS);
+}
+ParamOS += ")";
+ParamOS += '\0';
+
 if (look() == '.')
   First = Last;
 if (numLeft() != 0)
   return nullptr;
-return make("invocation function for block in ", Encoding);
+
+OutputStream DescOS;
+if (!initializeOutputStream(nullptr, nullptr, DescOS, 1024)) {
+  std::terminate();
+}
+DescOS << "invocation function for block with params '";
+DescOS << ParamOS.getBuffer();
+DescOS << "' in ";
+DescOS << '\0';
+std::free(ParamOS.getBuffer());
+
+return make(DescOS.getBuffer(), Encoding);
   }
 
   Node *Ty = getDerived().parseType();
Index: libcxxabi/src/demangle/ItaniumDemangle.h
===
--- libcxxabi/src/demangle/ItaniumDemangle.h
+++ libcxxabi/src/demangle/ItaniumDemangle.h
@@ -5503,8 +5503,7 @@
 //  ::= _Z 
 //::= 
 // extension  ::= ___Z  _block_invoke
-// extension  ::= ___Z  _block_invoke+
-// extension  ::= ___Z  _block_invoke_+
+// extension  ::= ___Z  _block_invoke+
 template 
 Node *AbstractManglingParser::parse() {
   if (consumeIf("_Z") || consumeIf("__Z")) {
@@ -5524,14 +5523,38 @@
 Node *Encoding = getDerived().parseEncoding();
 if (Encoding == nullptr || !consumeIf("_block_invoke"))
   return nullptr;
-boo

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv marked 6 inline comments as done.
alexbdv added a comment.

For tests - the existing block tests should be enough, just need to update them.
I updated a few - want to make sure changes are final before updating all the 
tests to not have to update tests again.




Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:5537
+
+  paramType->print(ParamOS);
+}

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 259684.
alexbdv marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp


Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -36,11 +36,18 @@
 StringRef Outer,
 const BlockDecl *BD,
 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-Out << "__" << Outer << "_block_invoke";
-  else
-Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  SmallString<256> ParamBuff;
+  llvm::raw_svector_ostream ParamTypes(ParamBuff);
+  for (ParmVarDecl *PVD : BD->parameters()) {
+Context.mangleTypeName(PVD->getType(), ParamTypes);
+  }
+
+  llvm::MD5 Hash;
+  llvm::MD5::MD5Result Result;
+  Hash.update(ParamTypes.str());
+  Hash.final(Result);
+
+  Out << "__" << Outer << "_block_invoke_" << *(unsigned short*)&Result;
 }
 
 void MangleContext::anchor() { }


Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -36,11 +36,18 @@
 StringRef Outer,
 const BlockDecl *BD,
 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-Out << "__" << Outer << "_block_invoke";
-  else
-Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  SmallString<256> ParamBuff;
+  llvm::raw_svector_ostream ParamTypes(ParamBuff);
+  for (ParmVarDecl *PVD : BD->parameters()) {
+Context.mangleTypeName(PVD->getType(), ParamTypes);
+  }
+
+  llvm::MD5 Hash;
+  llvm::MD5::MD5Result Result;
+  Hash.update(ParamTypes.str());
+  Hash.final(Result);
+
+  Out << "__" << Outer << "_block_invoke_" << *(unsigned short*)&Result;
 }
 
 void MangleContext::anchor() { }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@erik.pilkington How about just adding numeric hash of block parameters for now 
? This way we don't have to change the mangling / demangling scheme at all.
The mangling / demangling changes bring me into parts of LLVM that I'm not 
familiar with.
(PS - I still have to update tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@erik.pilkington would the hash-based numbering be OK for now ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith what regression are you referring to ? 
What this change is effectively doing now is changing the numbering of the 
blocks from incremental to hash-based.
So the demangler functionality remains the same (i think) - I saw that it is 
ignoring the (currently incrememntal) number, so changing it shouldn't make a 
difference.
Did I miss anything ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 261978.
alexbdv added a comment.
Herald added subscribers: kerbowa, nhaehnle, jvesely.

Updating tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/blocks-irgen.mm
  clang/test/CodeGen/block-with-perdefinedexpr.cpp
  clang/test/CodeGen/blocks.c
  clang/test/CodeGen/debug-info-block-vars.c
  clang/test/CodeGen/func-in-block.c
  clang/test/CodeGen/mangle-blocks.c
  clang/test/CodeGenCXX/block-byref.cpp
  clang/test/CodeGenCXX/blocks.cpp
  clang/test/CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
  clang/test/CodeGenCXX/predefined-expr-cxx14.cpp
  clang/test/CodeGenObjC/blocks-2.m
  clang/test/CodeGenObjC/blocks.m
  clang/test/CodeGenObjC/debug-info-block-line.m
  clang/test/CodeGenObjC/debug-info-nested-blocks.m
  clang/test/CodeGenObjC/mangle-blocks.m
  clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  clang/test/CodeGenObjCXX/block-default-arg.mm
  clang/test/CodeGenObjCXX/lambda-expressions.mm
  clang/test/CodeGenObjCXX/mangle-blocks.mm
  clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
  clang/test/CodeGenOpenCL/blocks.cl
  clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
  clang/test/PCH/block-helpers.cpp
  clang/test/PCH/no-escaping-block-tail-calls.cpp
  clang/test/Profile/Inputs/objc-general.proftext
  clang/test/Profile/objc-general.m

Index: clang/test/Profile/objc-general.m
===
--- clang/test/Profile/objc-general.m
+++ clang/test/Profile/objc-general.m
@@ -34,7 +34,7 @@
 #endif
 
 // PGOGEN: @[[FRC:"__profc_objc_general.m_\+\[A foreach_\]"]] = private global [2 x i64] zeroinitializer
-// PGOGEN: @[[BLC:"__profc_objc_general.m___13\+\[A foreach_\]_block_invoke"]] = private global [2 x i64] zeroinitializer
+// PGOGEN: @[[BLC:"__profc_objc_general.m___13\+\[A foreach_\]_block_invoke_7636"]] = private global [2 x i64] zeroinitializer
 // PGOGEN: @[[MAC:__profc_main]] = private global [1 x i64] zeroinitializer
 
 @interface A : NSObject
@@ -52,8 +52,8 @@
   // PGOUSE: br {{.*}} !prof ![[FR1:[0-9]+]]
   // PGOUSE: br {{.*}} !prof ![[FR2:[0-9]+]]
   for (id x in array) {
-// PGOGEN: define {{.*}}_block_invoke
-// PGOUSE: define {{.*}}_block_invoke
+// PGOGEN: define {{.*}}_block_invoke_7636
+// PGOUSE: define {{.*}}_block_invoke_7636
 // PGOGEN: store {{.*}} @[[BLC]], i64 0, i64 0
 ^{ static int init = 0;
   // PGOGEN: store {{.*}} @[[BLC]], i64 0, i64 1
Index: clang/test/Profile/Inputs/objc-general.proftext
===
--- clang/test/Profile/Inputs/objc-general.proftext
+++ clang/test/Profile/Inputs/objc-general.proftext
@@ -1,4 +1,4 @@
-objc-general.m:__13+[A foreach:]_block_invoke
+objc-general.m:__13+[A foreach:]_block_invoke_7636
 42129
 2
 2
Index: clang/test/PCH/no-escaping-block-tail-calls.cpp
===
--- clang/test/PCH/no-escaping-block-tail-calls.cpp
+++ clang/test/PCH/no-escaping-block-tail-calls.cpp
@@ -4,7 +4,7 @@
 // Check that -fno-escaping-block-tail-calls doesn't disable tail-call
 // optimization if the block is non-escaping.
 
-// CHECK-LABEL: define internal i32 @___ZN1S1mEv_block_invoke(
+// CHECK-LABEL: define internal i32 @___ZN1S1mEv_block_invoke_7636(
 // CHECK: %[[CALL:.*]] = tail call i32 @_ZN1S3fooER2S0(
 // CHECK-NEXT: ret i32 %[[CALL]]
 
Index: clang/test/PCH/block-helpers.cpp
===
--- clang/test/PCH/block-helpers.cpp
+++ clang/test/PCH/block-helpers.cpp
@@ -19,7 +19,7 @@
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_BLOCK_BYREF_Y]]* %[[Y]] to i8*
 // CHECK: store i8* %[[V1]], i8** %[[BLOCK_CAPTURED10]], align 8
 
-// CHECK-LABEL: define internal void @___ZN1S1mEv_block_invoke(
+// CHECK-LABEL: define internal void @___ZN1S1mEv_block_invoke_7636(
 
 // The second call to block_object_assign should be an invoke.
 
Index: clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
@@ -312,7 +312,7 @@
   };
 
   // Uses global block literal [[BLG8]] and invoke function [[INVG8]].
-  // COMMON: call spir_func void @__device_side_enqueue_block_invoke_11(i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG8]] to i8 addrspace(1)*) to i8 addrspace(4)*))
+  // COMMON: call spir_func void @__device_side_enqueue_block_invoke_7636.{{[0-9]+}}(i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG8]] to i8 addrspace(1)*) to i8 addrspace(4)*))
   block_A();
 
   // Emits global block literal [[BLG8]] and block kerne

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 262272.
alexbdv marked an inline comment as done.
alexbdv edited the summary of this revision.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/blocks-irgen.mm
  clang/test/CodeGen/block-with-perdefinedexpr.cpp
  clang/test/CodeGen/blocks.c
  clang/test/CodeGen/debug-info-block-vars.c
  clang/test/CodeGen/func-in-block.c
  clang/test/CodeGen/mangle-blocks.c
  clang/test/CodeGenCXX/block-byref.cpp
  clang/test/CodeGenCXX/blocks.cpp
  clang/test/CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
  clang/test/CodeGenCXX/predefined-expr-cxx14.cpp
  clang/test/CodeGenObjC/blocks-2.m
  clang/test/CodeGenObjC/blocks.m
  clang/test/CodeGenObjC/debug-info-block-line.m
  clang/test/CodeGenObjC/debug-info-nested-blocks.m
  clang/test/CodeGenObjC/mangle-blocks.m
  clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  clang/test/CodeGenObjCXX/block-default-arg.mm
  clang/test/CodeGenObjCXX/lambda-expressions.mm
  clang/test/CodeGenObjCXX/mangle-blocks.mm
  clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
  clang/test/CodeGenOpenCL/blocks.cl
  clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
  clang/test/PCH/block-helpers.cpp
  clang/test/PCH/no-escaping-block-tail-calls.cpp
  clang/test/Profile/Inputs/objc-general.proftext
  clang/test/Profile/objc-general.m

Index: clang/test/Profile/objc-general.m
===
--- clang/test/Profile/objc-general.m
+++ clang/test/Profile/objc-general.m
@@ -34,7 +34,7 @@
 #endif
 
 // PGOGEN: @[[FRC:"__profc_objc_general.m_\+\[A foreach_\]"]] = private global [2 x i64] zeroinitializer
-// PGOGEN: @[[BLC:"__profc_objc_general.m___13\+\[A foreach_\]_block_invoke"]] = private global [2 x i64] zeroinitializer
+// PGOGEN: @[[BLC:"__profc_objc_general.m___13\+\[A foreach_\]_block_invoke_7636"]] = private global [2 x i64] zeroinitializer
 // PGOGEN: @[[MAC:__profc_main]] = private global [1 x i64] zeroinitializer
 
 @interface A : NSObject
@@ -52,8 +52,8 @@
   // PGOUSE: br {{.*}} !prof ![[FR1:[0-9]+]]
   // PGOUSE: br {{.*}} !prof ![[FR2:[0-9]+]]
   for (id x in array) {
-// PGOGEN: define {{.*}}_block_invoke
-// PGOUSE: define {{.*}}_block_invoke
+// PGOGEN: define {{.*}}_block_invoke_7636
+// PGOUSE: define {{.*}}_block_invoke_7636
 // PGOGEN: store {{.*}} @[[BLC]], i64 0, i64 0
 ^{ static int init = 0;
   // PGOGEN: store {{.*}} @[[BLC]], i64 0, i64 1
Index: clang/test/Profile/Inputs/objc-general.proftext
===
--- clang/test/Profile/Inputs/objc-general.proftext
+++ clang/test/Profile/Inputs/objc-general.proftext
@@ -1,4 +1,4 @@
-objc-general.m:__13+[A foreach:]_block_invoke
+objc-general.m:__13+[A foreach:]_block_invoke_7636
 42129
 2
 2
Index: clang/test/PCH/no-escaping-block-tail-calls.cpp
===
--- clang/test/PCH/no-escaping-block-tail-calls.cpp
+++ clang/test/PCH/no-escaping-block-tail-calls.cpp
@@ -4,7 +4,7 @@
 // Check that -fno-escaping-block-tail-calls doesn't disable tail-call
 // optimization if the block is non-escaping.
 
-// CHECK-LABEL: define internal i32 @___ZN1S1mEv_block_invoke(
+// CHECK-LABEL: define internal i32 @___ZN1S1mEv_block_invoke_7636(
 // CHECK: %[[CALL:.*]] = tail call i32 @_ZN1S3fooER2S0(
 // CHECK-NEXT: ret i32 %[[CALL]]
 
Index: clang/test/PCH/block-helpers.cpp
===
--- clang/test/PCH/block-helpers.cpp
+++ clang/test/PCH/block-helpers.cpp
@@ -19,7 +19,7 @@
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_BLOCK_BYREF_Y]]* %[[Y]] to i8*
 // CHECK: store i8* %[[V1]], i8** %[[BLOCK_CAPTURED10]], align 8
 
-// CHECK-LABEL: define internal void @___ZN1S1mEv_block_invoke(
+// CHECK-LABEL: define internal void @___ZN1S1mEv_block_invoke_7636(
 
 // The second call to block_object_assign should be an invoke.
 
Index: clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
@@ -312,7 +312,7 @@
   };
 
   // Uses global block literal [[BLG8]] and invoke function [[INVG8]].
-  // COMMON: call spir_func void @__device_side_enqueue_block_invoke_11(i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG8]] to i8 addrspace(1)*) to i8 addrspace(4)*))
+  // COMMON: call spir_func void @__device_side_enqueue_block_invoke_7636.{{[0-9]+}}(i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG8]] to i8 addrspace(1)*) to i8 addrspace(4)*))
   block_A();
 
   // Emits global block literal [[BLG8]] and block kernel [[INVGK8

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

[Alex Borcan via Phabricator via cfe-commits]

alexbdv updated this revision to Diff 262281.

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

https://reviews.llvm.org/D74813

Files:
  clang/lib/AST/Mangle.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/blocks-irgen.mm
  clang/test/CodeGen/block-with-perdefinedexpr.cpp
  clang/test/CodeGen/blocks.c
  clang/test/CodeGen/debug-info-block-vars.c
  clang/test/CodeGen/func-in-block.c
  clang/test/CodeGen/mangle-blocks.c
  clang/test/CodeGenCXX/block-byref.cpp
  clang/test/CodeGenCXX/blocks.cpp
  clang/test/CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
  clang/test/CodeGenCXX/predefined-expr-cxx14.cpp
  clang/test/CodeGenObjC/blocks-2.m
  clang/test/CodeGenObjC/blocks.m
  clang/test/CodeGenObjC/debug-info-block-line.m
  clang/test/CodeGenObjC/debug-info-nested-blocks.m
  clang/test/CodeGenObjC/mangle-blocks.m
  clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  clang/test/CodeGenObjCXX/block-default-arg.mm
  clang/test/CodeGenObjCXX/lambda-expressions.mm
  clang/test/CodeGenObjCXX/mangle-blocks.mm
  clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
  clang/test/CodeGenOpenCL/blocks.cl
  clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
  clang/test/PCH/block-helpers.cpp
  clang/test/PCH/no-escaping-block-tail-calls.cpp
  clang/test/Profile/Inputs/objc-general.proftext
  clang/test/Profile/objc-general.m

Index: clang/test/Profile/objc-general.m
===
--- clang/test/Profile/objc-general.m
+++ clang/test/Profile/objc-general.m
@@ -34,7 +34,7 @@
 #endif
 
 // PGOGEN: @[[FRC:"__profc_objc_general.m_\+\[A foreach_\]"]] = private global [2 x i64] zeroinitializer
-// PGOGEN: @[[BLC:"__profc_objc_general.m___13\+\[A foreach_\]_block_invoke"]] = private global [2 x i64] zeroinitializer
+// PGOGEN: @[[BLC:"__profc_objc_general.m___13\+\[A foreach_\]_block_invoke_7636"]] = private global [2 x i64] zeroinitializer
 // PGOGEN: @[[MAC:__profc_main]] = private global [1 x i64] zeroinitializer
 
 @interface A : NSObject
@@ -52,8 +52,8 @@
   // PGOUSE: br {{.*}} !prof ![[FR1:[0-9]+]]
   // PGOUSE: br {{.*}} !prof ![[FR2:[0-9]+]]
   for (id x in array) {
-// PGOGEN: define {{.*}}_block_invoke
-// PGOUSE: define {{.*}}_block_invoke
+// PGOGEN: define {{.*}}_block_invoke_7636
+// PGOUSE: define {{.*}}_block_invoke_7636
 // PGOGEN: store {{.*}} @[[BLC]], i64 0, i64 0
 ^{ static int init = 0;
   // PGOGEN: store {{.*}} @[[BLC]], i64 0, i64 1
Index: clang/test/Profile/Inputs/objc-general.proftext
===
--- clang/test/Profile/Inputs/objc-general.proftext
+++ clang/test/Profile/Inputs/objc-general.proftext
@@ -1,4 +1,4 @@
-objc-general.m:__13+[A foreach:]_block_invoke
+objc-general.m:__13+[A foreach:]_block_invoke_7636
 42129
 2
 2
Index: clang/test/PCH/no-escaping-block-tail-calls.cpp
===
--- clang/test/PCH/no-escaping-block-tail-calls.cpp
+++ clang/test/PCH/no-escaping-block-tail-calls.cpp
@@ -4,7 +4,7 @@
 // Check that -fno-escaping-block-tail-calls doesn't disable tail-call
 // optimization if the block is non-escaping.
 
-// CHECK-LABEL: define internal i32 @___ZN1S1mEv_block_invoke(
+// CHECK-LABEL: define internal i32 @___ZN1S1mEv_block_invoke_7636(
 // CHECK: %[[CALL:.*]] = tail call i32 @_ZN1S3fooER2S0(
 // CHECK-NEXT: ret i32 %[[CALL]]
 
Index: clang/test/PCH/block-helpers.cpp
===
--- clang/test/PCH/block-helpers.cpp
+++ clang/test/PCH/block-helpers.cpp
@@ -19,7 +19,7 @@
 // CHECK: %[[V1:.*]] = bitcast %[[STRUCT_BLOCK_BYREF_Y]]* %[[Y]] to i8*
 // CHECK: store i8* %[[V1]], i8** %[[BLOCK_CAPTURED10]], align 8
 
-// CHECK-LABEL: define internal void @___ZN1S1mEv_block_invoke(
+// CHECK-LABEL: define internal void @___ZN1S1mEv_block_invoke_7636(
 
 // The second call to block_object_assign should be an invoke.
 
Index: clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
@@ -312,7 +312,7 @@
   };
 
   // Uses global block literal [[BLG8]] and invoke function [[INVG8]].
-  // COMMON: call spir_func void @__device_side_enqueue_block_invoke_11(i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG8]] to i8 addrspace(1)*) to i8 addrspace(4)*))
+  // COMMON: call spir_func void @__device_side_enqueue_block_invoke_7636.{{[0-9]+}}(i8 addrspace(4)* addrspacecast (i8 addrspace(1)* bitcast ({ i32, i32, i8 addrspace(4)* } addrspace(1)* [[BLG8]] to i8 addrspace(1)*) to i8 addrspace(4)*))
   block_A();
 
   // Emits global block literal [[BLG8]] and block kernel [[INVGK8]]. [[INVGK8]] calls [[INVG8]].
@@ -331,7 +331,7 @@
   unsigned size = get_kernel_work_group_size(block_A);
 
   // Uses globa

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith  @erik.pilkington  The change is final now, could we get this in ?


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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

Could I please get a review on this ? Thanks :) !


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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith - Are you OK with that ?


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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv created this revision.
alexbdv added reviewers: MaskRay, vsk, JonasToth, ruiu.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Function blocks don't have a name specified in source code. Currently their 
symbol name is based on the parent function's name and an index. 
Ex: _ZN1Parent_Funciton_block_invoke_1
One issue that happens with the current naming scheme is that in subsequent 
builds, the name of the function block can change even if the function block or 
the parent function has changed. This presents issues for tools that use symbol 
names to identify changes in source code / tracking of binary metrics.

The proposed solution here is to add a flag (default=off) that enables adding a 
hash of the block contents to the block name. 
Ex: _ZN1Parent_Funciton_block_invoke_38172  (38172 is the hash)
Therefore, the function block name will only change if its content or the 
parent function name changes. And will now remain stable regardless if new 
function blocks are added.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74813

Files:
  clang/include/clang/AST/Mangle.h
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/Mangle.cpp
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2817,6 +2817,9 @@
   Opts.ExternCNoUnwind = Args.hasArg(OPT_fexternc_nounwind);
   Opts.TraditionalCPP = Args.hasArg(OPT_traditional_cpp);
 
+  Opts.ObjCEnableHashFuncBlockNames =
+  Args.hasArg(OPT_enable_hash_in_objc_func_block_names);
+
   Opts.RTTI = Opts.CPlusPlus && !Args.hasArg(OPT_fno_rtti);
   Opts.RTTIData = Opts.RTTI && !Args.hasArg(OPT_fno_rtti_data);
   Opts.Blocks = Args.hasArg(OPT_fblocks) || (Opts.OpenCL
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -24,11 +24,61 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Mangler.h"
-#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ErrorHandling.h"	
+#include "llvm/Support/MD5.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 
+static std::string getBlockIDByHash(const BlockDecl *BD,
+unsigned discriminator) {
+  Stmt *stmt = BD ? BD->getBody() : nullptr;
+
+  std::string strOutBuff;
+  llvm::raw_string_ostream strOutStream(strOutBuff);
+
+  strOutStream << "_block_invoke";
+
+  if (!stmt) {
+if (discriminator)
+  strOutStream << "_" << discriminator + 1;
+
+return strOutBuff;
+  }
+
+  std::string strStmtBuff;
+  llvm::raw_string_ostream strStmtStream(strStmtBuff);
+
+  // Dump the statement IR to a text stream for hasing
+  stmt->dump(strStmtStream);
+  strStmtBuff = strStmtStream.str();
+
+  // Strip out addresses
+  char *ptr = &strStmtBuff[1];
+  while (*ptr) {
+if (*ptr == 'x' && *(ptr - 1) == '0') {
+  ptr++;
+  while ((*ptr >= '0' && *ptr <= '9') || (*ptr >= 'a' && *ptr <= 'f')) {
+*ptr = '_';
+ptr++;
+  }
+  continue;
+}
+ptr++;
+  }
+
+  // Hash the statement string
+  llvm::MD5 Hash;
+  llvm::MD5::MD5Result Result;
+
+  Hash.update(strStmtBuff);
+  Hash.final(Result);
+
+  strOutStream << "_" << *(unsigned short *)&Result;
+  return strOutBuff;
+}
+
+
 // FIXME: For blocks we currently mimic GCC's mangling scheme, which leaves
 // much to be desired. Come up with a better mangling scheme.
 
@@ -37,6 +87,12 @@
 const BlockDecl *BD,
 raw_ostream &Out) {
   unsigned discriminator = Context.getBlockId(BD, true);
+  	
+  if (Context.shouldAddHashOfBlockToName()) {
+Out << "__" << Outer << getBlockIDByHash(BD, discriminator);
+return;
+  }
+
   if (discriminator == 0)
 Out << "__" << Outer << "_block_invoke";
   else
@@ -89,6 +145,10 @@
 return CCM_Vector;
   }
 }
+	
+bool MangleContext::shouldAddHashOfBlockToName() {
+  return getASTContext().getLangOpts().ObjCEnableHashFuncBlockNames;
+}
 
 bool MangleContext::shouldMangleDeclName(const NamedDecl *D) {
   const ASTContext &ASTContext = getASTContext();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1349,6 +1349,8 @@
 def fthin_link_bitcode_EQ : Joined<["-"], "fthin-link-bitcode=">,
   Flags<[CoreOption, CC1Option]>, Group,
   HelpText<"Write minimized bitcode to  for the ThinLTO thin link only">;
+def enable_hash_in_objc_func_block_names : Flag<["-"], "enable-hash-in-objc-func-block-names">, Flags<[CC1Option]>, Group,
+  HelpText<"For objc function blocks - enable adding a hash of the block in the blocks's

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a subscriber: vsk.
alexbdv added a comment.

@vsk  - sure will add tests when removing from RFC.
As for making it default - would rather have this under a flag as hashing the 
block contents does have some overhead and I imagine this feature wouldn't be 
beneficial in most scenarios. Also, unexpectedly (by default) changing the name 
of the function blocks might have a negative impact on some existing workflows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@vsk - about breaking existing workflows - I was referring only to if / when 
this gets shipped out as the default - all the names for the function blocks 
will change and this might cause issue with tooling that relies on symbol names 
being consistent across builds.

@dexonsmith

- Just to make sure - this isn't dumping LLVM IR but a textual representation 
of the AST. Does this have the same issues with metadata / metadata numbering 
that LLVM IR has, or you were referring to the AST text dump, not llvm IR ? 
Also I don't think that clang would be a good test case here - it doesn't have 
many block functions.
- Stability wise - I'm still not sure if this has the metadata numbering issue 
(since it's AST text representation), perhaps you can tell me how to check.
- Correct, the hash might change if the block contents changes - this is kind 
of the intended use for this. As the flag mentions, it is hash-based.

Moving to a per-function index-based approach seems like the correct default 
approach. I would still like to have the hash version under a flag though. 
Moving to per-function index-based should be as simple as not adding the  
discriminator at all. Since function blocks contain the parent's function's 
name and given that llvm auto-renames duplicate symbols by adding an 
incremental number - the end result is that function blocks will be 
incrementally named based on the order that they have in the parent function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@dexonsmith I did a benchmark with the worst case example I can come up with - 
1000 regular functions and 1000 blocks : 
https://paste.ofcode.org/CFU6b46nuAA6ymxUEpkzka
I didn't manage to measure any performance decrease (the performance decrease 
was within the noise of repeated runs) - so seems the hashing is insignificant 
compile-time-wise even in worst case scenario (where most of the code is in 
function blocks).
And here is the actual text that is being hashed - the textual representation 
of the AST, not the IR: https://paste.ofcode.org/bfMzhbvHz9HRT7mVMe48Mx


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Borcan via Phabricator via cfe-commits]

alexbdv added a comment.

@vsk  / @dexonsmith - I've added some more info in latest comments. Let me know 
if I can provide more info / context on this to be able to reach a conclusion. 
Or if you think it is clear at this point that the hash-based approach is a 
no-go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits