[Clang] Allow vanilla C function symbol name to be used in __attribute__((alias)) when -funique-internal-linkage-names is specified

[HighW4y2H3ll]

-funique-internal-linkage-names mangles function symbols even for C functions. When using __attribute__((alias())), it's a bit counter-intuitive to specify a C++ mangled symbol name in the attribute. Besides, EmitGlobal won't be able to emit the aliasee function because its name has been changed which no longer matches any GlobalValue name in the emitted LLVM Module. This patch fixes the EmitGlobal function which emits a function with the mangled name, and resolves the C symbol inside __attribute__((alias)) to its uniquely mangled name to keep the behavior consistent.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (HighW4y2H3ll)

Changes

-funique-internal-linkage-names mangles function symbols even for C functions. When using __attribute__((alias())), it's a bit counter-intuitive to specify a C++ mangled symbol name in the attribute. Besides, EmitGlobal won't be able to emit the aliasee function because its name has been changed which no longer matches any GlobalValue name in the emitted LLVM Module. This patch fixes the EmitGlobal function which emits a function with the mangled name, and resolves the C symbol inside __attribute__((alias)) to its uniquely mangled name to keep the behavior consistent.

---

Full diff: https://github.com/llvm/llvm-project/pull/145652.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (+44-4)
• (added) clang/test/CodeGen/unique-internal-linkage-names-alias.c (+10)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 16688810d0685..90f02220ec306 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -588,8 +588,9 @@ static const llvm::GlobalValue *getAliasedGlobal(const llvm::GlobalValue *GV) {
 }
 
 static bool checkAliasedGlobal(
-    const ASTContext &Context, DiagnosticsEngine &Diags, SourceLocation Location,
-    bool IsIFunc, const llvm::GlobalValue *Alias, const llvm::GlobalValue *&GV,
+    const CodeGenModule *CGM, const ASTContext &Context,
+    DiagnosticsEngine &Diags, SourceLocation Location, bool IsIFunc,
+    const llvm::GlobalValue *Alias, const llvm::GlobalValue *&GV,
     const llvm::MapVector<GlobalDecl, StringRef> &MangledDeclNames,
     SourceRange AliasRange) {
   GV = getAliasedGlobal(Alias);
@@ -598,6 +599,23 @@ static bool checkAliasedGlobal(
     return false;
   }
 
+  // Only resolve unique internal linkage symbols for C code
+  if (!CGM->getLangOpts().CPlusPlus) {
+    for (const auto &[Decl, Name] : MangledDeclNames) {
+      if (const auto *ND = dyn_cast<NamedDecl>(Decl.getDecl())) {
+        IdentifierInfo *II = ND->getIdentifier();
+        if (II && II->getName() == GV->getName() &&
+            Name.contains(llvm::FunctionSamples::UniqSuffix)) {
+          GlobalDecl GD;
+          if (CGM->lookupRepresentativeDecl(Name, GD)) {
+            GV = CGM->getModule().getNamedValue(Name);
+            break;
+          }
+        }
+      }
+    }
+  }
+
   if (GV->hasCommonLinkage()) {
     const llvm::Triple &Triple = Context.getTargetInfo().getTriple();
     if (Triple.getObjectFormat() == llvm::Triple::XCOFF) {
@@ -687,8 +705,8 @@ void CodeGenModule::checkAliases() {
     StringRef MangledName = getMangledName(GD);
     llvm::GlobalValue *Alias = GetGlobalValue(MangledName);
     const llvm::GlobalValue *GV = nullptr;
-    if (!checkAliasedGlobal(getContext(), Diags, Location, IsIFunc, Alias, GV,
-                            MangledDeclNames, Range)) {
+    if (!checkAliasedGlobal(this, getContext(), Diags, Location, IsIFunc, Alias,
+                            GV, MangledDeclNames, Range)) {
       Error = true;
       continue;
     }
@@ -4038,6 +4056,7 @@ void CodeGenModule::EmitGlobal(GlobalDecl GD) {
     CXXGlobalInits.push_back(nullptr);
   }
 
+  const auto *ND = dyn_cast<NamedDecl>(GD.getDecl());
   StringRef MangledName = getMangledName(GD);
   if (GetGlobalValue(MangledName) != nullptr) {
     // The value has already been used and should therefore be emitted.
@@ -4046,6 +4065,12 @@ void CodeGenModule::EmitGlobal(GlobalDecl GD) {
     // The value must be emitted, but cannot be emitted eagerly.
     assert(!MayBeEmittedEagerly(Global));
     addDeferredDeclToEmit(GD);
+  } else if (!getLangOpts().CPlusPlus && ND &&
+             GetGlobalValue(ND->getName()) != nullptr &&
+             MangledName.contains(llvm::FunctionSamples::UniqSuffix)) {
+    // Emit static C function that is mangled with
+    // -funique-internal-linkage-names.
+    addDeferredDeclToEmit(GD);
   } else {
     // Otherwise, remember that we saw a deferred decl with this name.  The
     // first use of the mangled name will cause it to move into
@@ -6189,6 +6214,21 @@ void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
                                                    /*DontDefer=*/true,
                                                    ForDefinition));
 
+  if (!getLangOpts().CPlusPlus &&
+      getCXXABI().getMangleContext().shouldMangleDeclName(D)) {
+    // -funique-internal-linkage-names may change the symbol name of C function.
+    // Replace all uses of old symbol with the emitted global value.
+    if (IdentifierInfo *II = D->getIdentifier()) {
+      if (II->getName() != GV->getName() &&
+          GV->getName().contains(llvm::FunctionSamples::UniqSuffix)) {
+        if (llvm::GlobalValue *GVDef =
+                getModule().getNamedValue(D->getName())) {
+          GVDef->replaceAllUsesWith(GV);
+        }
+      }
+    }
+  }
+
   // Already emitted.
   if (!GV->isDeclaration())
     return;
diff --git a/clang/test/CodeGen/unique-internal-linkage-names-alias.c b/clang/test/CodeGen/unique-internal-linkage-names-alias.c
new file mode 100644
index 0000000000000..14bfea08367d3
--- /dev/null
+++ b/clang/test/CodeGen/unique-internal-linkage-names-alias.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -emit-llvm -funique-internal-linkage-names -o - | FileCheck %s
+
+struct A;
+static long foo(const struct A*p);
+
+long bar(const struct A*p);
+long bar(const struct A*p) __attribute__((__alias__("foo")));
+
+// CHECK: define internal i64 @_ZL3fooPK1A.__uniq.[[ATTR:[0-9]+]](ptr noundef %p) #1 {
+static long foo(const struct A*p) {return 1;}

[efriedma-quic]

I'm not really happy with this. The code for deferred declarations and diagnosing name conflicts is complicated enough without adding the possibility that the name of a declaration can change later.

Can we just suppress mangling in cases where we reference a symbol without knowing if it's internal, then it turns out to be internal later?

[HighW4y2H3ll]

The problem is we need -funique-internal-linkage-names to keep all the static function symbols unique... It is to make sure that we have unique symbols names in the DWARF info after linking. Besides.... it's also a bit weird to specify something like __attribute__((__alias__(_ZL3fooPK1A.__uniq.43626431813877880438002914689114607187))) in the C Code.. It's only a problem with attribute alias tho... Do you think we should add more unit tests for this change?

[efriedma-quic]

Is this C-only? Consider something like:

long bar(const struct A*p) __attribute__((__alias__("foo")));
namespace {
extern "C" long foo(const struct A*p) {return 1;}
}

[HighW4y2H3ll]

Yup, we are trying to build a codebase written entirely in C... I'm not too sure how namespace and extern "C" would translate in C tho...