[PATCH] D96109: Refactor implementation of -funique-internal-linkage-names.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/AST/Mangle.cpp:119-124
+  // C functions with internal linkage have to be mangled with option
+  // -funique-internal-linkage-names.
+  if (!getASTContext().getLangOpts().CPlusPlus &&
+      isUniqueInternalLinkageDecl(D))
+    return true;
+
----------------
tmsriram wrote:
> dblaikie wrote:
> > Strikes me as strange to test the language mode here (presumably this logic would apply equally to C++ code, right? If at most it'd be a no-op because things are already mangled)
> > 
> > But, for instance - what about functions with extern "C" linkage in a C++ file?
> Internal linkage functions under extern "C" are already mangled even without this patch. I was surprised too. Here is the example:
> 
> ```
> extern "C" {
>   static int foo(void) {
>    return 0; 
>   }
>   static int l
>   int bar() {
>     return foo() + l; 
> }
> }
> ```
> ```
> $ clang++ -c foo.cc
> $ nm foo.o
> T bar
> t _ZL3foov
> b _ZL1l
> ```
> 
> 
> 
> 
> 
> 
ah, right, my bad.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:205
+    // that this symbol is of internal linkage type.
+    ModuleNameHash = (Twine(".__uniq.")
+                      + Twine(IntHash.toString(10, false))).str();
----------------
tmsriram wrote:
> dblaikie wrote:
> > Does this need the double underscore? I thought other suffixes here generally don't have that feature?
> This was discussed in D89617 to have two underscores before uniq as the convention when I first added this identifier.  Not sure if you want to revisit that.
Ah, thanks for the pointer! Might be worth documenting in a comment (if this is the only place that string appears)

Do you have a rough sense/list of the suffixes used by LLVM or other implementations at the moment (I see some code in LLVM that looks for ".part." and ".llvm." at least) - I don't know that "__" really provides much. The double underscore rule is to differentiate user symbols from C++ symbols in the C++ standard, but doesn't do much/anything to protect one implementation from another implementation, for instance.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1179-1182
+      ((isa<FunctionDecl>(D) &&
+        CGM.getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) ||
+       (isa<VarDecl>(D) && CGM.getContext().GetGVALinkageForVariable(
+                               cast<VarDecl>(D)) == GVA_Internal)))
----------------
tmsriram wrote:
> dblaikie wrote:
> > The variety of things being tested here seems non-trivial, and I've no idea how to validate it's correct. Any chance this is existing logic that could be reused from somewhere else/refactored into a common function to call from some existing use case and this new one? (similar thougts on ItaniumDemangle.cpp:isInternalLinkageDecl)
> So the Itanium's internal linakge decl check is exactly the logic that is used to determine when to add the "_ZL" to internal linkage names.  I also refactored the original code to share the logic.  For CodeGenModule.cpp, I am not sure what to use as C++ has other cases where internal linkage could be used, like anonymous namespaces maybe.  I will try to dig a little more here. 
Thanks!


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

https://reviews.llvm.org/D96109