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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram 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;
+
----------------
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
```








================
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();
----------------
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.


================
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)))
----------------
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. 


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

https://reviews.llvm.org/D96109