[CFI] Create an external linkage alias instead of promoting internals

[mtrofin]

The problem is described in PR #201849. This is a simple fix - thanks, @teresajohnson for the idea. When CFI promotes an internal symbol, it previously would have to create an alias to avoid breakages in the case of references from inline asm. The alternative here is to leave the symbol as-is and create an alias for CFI's purposes. This makes the calculation of a GUID for this new symbol avoid the class of colisions discussed in the earlier PR. Then, during LowerTypeTests, we give the functon the alias' name and the rest of the CFI functionality remains unaffected.

An alternative we discussed would be to use the same kind of hash as CFI uses - based on module content. That would add some cost to compile time for all modules (it could be paid only in PGO/ThinLTO cases, though). We preferred this alternative here because it's simpler and has no compile-time penalty.

Co-authored-by: Teresa Johnson <tejohnson@google.com>

[mtrofin]

• [CFI] Create an external linkage alias instead of promoting internals #203171 👈 (View in Graphite)
• Reland "Compute GUIDs once and store in metadata" (#184065)  #201849
• [Coro] Handle aliases to coroutines #204408
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

🪟 Windows x64 Test Results

• 138460 tests passed
• 3523 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🐧 Linux x64 Test Results

• 200095 tests passed
• 5501 tests skipped

✅ The build succeeded and all tests passed.

[teresajohnson]

Maybe rename Promote Extra to what it actually is, so that it is more obvious why the set needs an update (probably add a comment too, or at least)

[mtrofin]

would do as NFC, before this. Just reusing the current name here.

[teresajohnson]

Why does the SetVector need to be a pointer now? Can it just be a non-const ref?

[mtrofin]

I was "meh" about it, too... yes, it could be, but then we'd have to pass down an empty CfiFunctions at the call site. When reading this function, the nuance that "it's empty in one case, intentionally" may be missed. The pointer sends a clearer message, imho.

[teresajohnson]

What happens if there is more than one? Also, add a comment about why we need to get aliasee

[mtrofin]

Rewrote, ptal

[teresajohnson]

why is this needed?

[mtrofin]

because of A.eraseFromParent(), which then invalidates the iterator.

[llvmorg-github-actions]

@llvm/pr-subscribers-lto

@llvm/pr-subscribers-llvm-transforms

Author: Mircea Trofin (mtrofin)

Changes

The problem is described in PR #201849. This is a simple fix - thanks, @teresajohnson for the idea. When CFI promotes an internal symbol, it previously would have to create an alias to avoid breakages in the case of references from inline asm. The alternative here is to leave the symbol as-is and create an alias for CFI's purposes. This makes the calculation of a GUID for this new symbol avoid the class of colisions discussed in the earlier PR.

An alternative we discussed would be to use the same kind of hash as CFI uses - based on module content. That would add some cost to compile time for all modules (it could be paid only in PGO/ThinLTO cases, though). We preferred this alternative here because it's simpler and has no compile-time penalty.

---

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

9 Files Affected:

• (modified) llvm/lib/Transforms/IPO/LowerTypeTests.cpp (+16)
• (modified) llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp (+20-33)
• (modified) llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp (+20-5)
• (modified) llvm/test/ThinLTO/X86/devirt_function_alias2.ll (+6-4)
• (modified) llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-icall-static-inline-asm.ll (+2-2)
• (modified) llvm/test/Transforms/ThinLTOBitcodeWriter/comdat.ll (+10-7)
• (modified) llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal1.ll (+2-1)
• (modified) llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll (+3-2)
• (modified) llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll (+4-2)

diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index 7c045c90ead5b..d399b6f9d1090 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -2138,6 +2138,22 @@ bool LowerTypeTestsModule::lower() {
       report_fatal_error(
           "unexpected call to llvm.icall.branch.funnel during import phase");
 
+    for (auto &A : llvm::make_early_inc_range(M.aliases())) {
+      if (A.hasLocalLinkage())
+        continue;
+      if (ImportSummary->cfiFunctionDefs().contains(A.getName()) ||
+          ImportSummary->cfiFunctionDecls().contains(A.getName())) {
+        if (auto *F = dyn_cast<Function>(A.getAliasee()->stripPointerCasts())) {
+          A.replaceAllUsesWith(F);
+          std::string Name = std::string(A.getName());
+          A.eraseFromParent();
+          F->setName(Name);
+          F->setLinkage(GlobalValue::ExternalLinkage);
+          F->setVisibility(GlobalValue::HiddenVisibility);
+        }
+      }
+    }
+
     SmallVector<Function *, 8> Defs;
     SmallVector<Function *, 8> Decls;
     for (auto &F : M) {
diff --git a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
index 40200a6554249..1f4dff0aae472 100644
--- a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -30,23 +30,10 @@ using namespace llvm;
 
 namespace {
 
-// Determine if a promotion alias should be created for a symbol name.
-static bool allowPromotionAlias(const std::string &Name) {
-  // Promotion aliases are used only in inline assembly. It's safe to
-  // simply skip unusual names. Subset of MCAsmInfo::isAcceptableChar()
-  // and MCAsmInfoXCOFF::isAcceptableChar().
-  for (const char &C : Name) {
-    if (isAlnum(C) || C == '_' || C == '.')
-      continue;
-    return false;
-  }
-  return true;
-}
-
 // Promote each local-linkage entity defined by ExportM and used by ImportM by
 // changing visibility and appending the given ModuleId.
 void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId,
-                      const SetVector<GlobalValue *> &PromoteExtra) {
+                      SetVector<GlobalValue *> *PromoteExtra = nullptr) {
   DenseMap<const Comdat *, Comdat *> RenamedComdats;
   for (auto &ExportGV : ExportM.global_values()) {
     if (!ExportGV.hasLocalLinkage())
@@ -54,7 +41,8 @@ void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId,
 
     auto Name = ExportGV.getName();
     GlobalValue *ImportGV = nullptr;
-    if (!PromoteExtra.count(&ExportGV)) {
+    const bool MustPromote = PromoteExtra && PromoteExtra->count(&ExportGV);
+    if (!MustPromote) {
       ImportGV = ImportM.getNamedValue(Name);
       if (!ImportGV)
         continue;
@@ -72,24 +60,24 @@ void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId,
       if (C->getName() == Name)
         RenamedComdats.try_emplace(C, ExportM.getOrInsertComdat(NewName));
 
-    ExportGV.setName(NewName);
-    ExportGV.setLinkage(GlobalValue::ExternalLinkage);
-    ExportGV.setVisibility(GlobalValue::HiddenVisibility);
-    // TODO: remove this reassign and instead create an alias.
-    ExportGV.reassignGUID();
+    auto *ExternalAlias = GlobalAlias::create(
+        ExportGV.getType(), ExportGV.getAddressSpace(),
+        GlobalValue::ExternalLinkage, NewName, &ExportGV, &ExportM);
+    ExternalAlias->setVisibility(GlobalValue::HiddenVisibility);
+    ExportGV.replaceUsesWithIf(ExternalAlias, [ExternalAlias](Use &U) {
+      return U.getUser() != ExternalAlias;
+    });
+
+    if (MustPromote) {
+      PromoteExtra->remove(&ExportGV);
+      PromoteExtra->insert(ExternalAlias);
+    }
+
     if (ImportGV) {
       ImportGV->setName(NewName);
       ImportGV->setVisibility(GlobalValue::HiddenVisibility);
       ImportGV->reassignGUID();
     }
-
-    if (isa<Function>(&ExportGV) && allowPromotionAlias(OldName)) {
-      // Create a local alias with the original name to avoid breaking
-      // references from inline assembly.
-      std::string Alias =
-          ".lto_set_conditional " + OldName + "," + NewName + "\n";
-      ExportM.appendModuleInlineAsm(Alias);
-    }
   }
 
   if (!RenamedComdats.empty())
@@ -191,9 +179,8 @@ void simplifyExternals(Module &M) {
         F.getName().starts_with("llvm."))
       continue;
 
-    Function *NewF =
-        Function::Create(EmptyFT, GlobalValue::ExternalLinkage,
-                         F.getAddressSpace(), "", &M);
+    Function *NewF = Function::Create(EmptyFT, GlobalValue::ExternalLinkage,
+                                      F.getAddressSpace(), "", &M);
     NewF->copyAttributesFrom(&F);
     // Only copy function attribtues.
     NewF->setAttributes(AttributeList::get(M.getContext(),
@@ -419,8 +406,8 @@ void splitAndWriteThinLTOBitcode(
   // match values from its first argument (the "exporting module") in
   // CfiFunctions. So we only need CfiFunctions for the second promotion (M ->
   // MergedM)
-  promoteInternals(*MergedM, M, ModuleId, {});
-  promoteInternals(M, *MergedM, ModuleId, CfiFunctions);
+  promoteInternals(*MergedM, M, ModuleId, nullptr);
+  promoteInternals(M, *MergedM, ModuleId, &CfiFunctions);
 
   auto &Ctx = MergedM->getContext();
   SmallVector<MDNode *, 8> CfiFunctionMDs;
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index f876d4f02bb15..2c199896e2fe3 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -1147,6 +1147,8 @@ bool DevirtModule::tryFindVirtualCallTargets(
     // target.
     auto *GV = dyn_cast<GlobalValue>(C);
     assert(GV);
+    if (auto *GA = dyn_cast<GlobalAlias>(GV))
+      GV = GA->getAliaseeObject();
     TargetsForSlot.push_back({GV, &TM});
   }
 
@@ -1203,7 +1205,17 @@ bool DevirtIndex::tryFindVirtualCallTargets(
       if (mustBeUnreachableFunction(VTP.FuncVI))
         continue;
 
-      TargetsForSlot.push_back(VTP.FuncVI);
+      ValueInfo TheFn = VTP.FuncVI;
+      // Internal linkage functions get an external linkage alias to them.
+      if (!TheFn.getSummaryList().empty())
+        if (auto *AS =
+                dyn_cast<AliasSummary>(TheFn.getSummaryList()[0].get())) {
+          assert(TheFn.getSummaryList().size() == 1 &&
+                 "Aliases to internal linkage targets of indirect calls are "
+                 "expected to point to exactly one definition.");
+          TheFn = AS->getAliaseeVI();
+        }
+      TargetsForSlot.push_back(TheFn);
     }
   }
 
@@ -2261,10 +2273,13 @@ void DevirtModule::importResolution(VTableSlot Slot, VTableSlotInfo &SlotInfo) {
     assert(!Res.SingleImplName.empty());
     // The type of the function in the declaration is irrelevant because every
     // call site will cast it to the correct type.
-    Constant *SingleImpl =
-        cast<Constant>(M.getOrInsertFunction(Res.SingleImplName,
-                                             Type::getVoidTy(M.getContext()))
-                           .getCallee());
+    Value *SingleImplVal =
+        M.getOrInsertFunction(Res.SingleImplName,
+                              Type::getVoidTy(M.getContext()))
+            .getCallee();
+    if (auto *A = dyn_cast<GlobalAlias>(SingleImplVal->stripPointerCasts()))
+      SingleImplVal = A->getAliaseeObject();
+    Constant *SingleImpl = cast<Constant>(SingleImplVal);
 
     // This is the import phase so we should not be exporting anything.
     bool IsExported = false;
diff --git a/llvm/test/ThinLTO/X86/devirt_function_alias2.ll b/llvm/test/ThinLTO/X86/devirt_function_alias2.ll
index 3b3208e300aa8..e4f93208fba54 100644
--- a/llvm/test/ThinLTO/X86/devirt_function_alias2.ll
+++ b/llvm/test/ThinLTO/X86/devirt_function_alias2.ll
@@ -24,8 +24,8 @@
 ; RUN:   2>&1 | FileCheck %s --check-prefix=REMARK --check-prefix=PRINT
 ; RUN: llvm-dis %t2.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR1
 
-; PRINT-DAG: Devirtualized call to {{.*}} (_ZN1D1mEiAlias)
-; REMARK-DAG: single-impl: devirtualized a call to _ZN1D1mEiAlias
+; PRINT-DAG: Devirtualized call to {{.*}} (_ZN1D1mEi)
+; REMARK-DAG: single-impl: devirtualized a call to _ZN1D1mEi
 
 ;; Test hybrid Thin/Regular LTO
 
@@ -63,13 +63,15 @@ target triple = "x86_64-grtev4-linux-gnu"
 
 @_ZTV1D = constant { [3 x ptr] } { [3 x ptr] [ptr null, ptr undef, ptr @_ZN1D1mEiAlias] }, !type !3
 
-define i32 @_ZN1D1mEi(ptr %this, i32 %a) {
+define i32 @_ZN1D1mEi(ptr %this, i32 %a) #0 {
    ret i32 0;
 }
 
 @_ZN1D1mEiAlias = unnamed_addr alias i32 (ptr, i32), ptr @_ZN1D1mEi
 
-; CHECK-IR1-LABEL: define i32 @test
+attributes #0 = { noinline optnone }
+
+; CHECK-IR1-LABEL: define {{.*}}i32 @test
 define i32 @test(ptr %obj2, i32 %a) {
 entry:
   %vtable2 = load ptr, ptr %obj2
diff --git a/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-icall-static-inline-asm.ll b/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-icall-static-inline-asm.ll
index d8ebae17d4693..9d2ee425031ab 100644
--- a/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-icall-static-inline-asm.ll
+++ b/llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-icall-static-inline-asm.ll
@@ -3,7 +3,7 @@
 
 target triple = "x86_64-unknown-linux-gnu"
 
-; CHECK: module asm ".lto_set_conditional a,a.[[HASH:[0-9a-f]+]]"
+; CHECK: @a.[[HASH:[0-9a-f]+]] = hidden alias
 
 define void @b() {
   %f = alloca ptr, align 8
@@ -14,7 +14,7 @@ define void @b() {
   ret void
 }
 
-; CHECK: define{{.*}} @a.[[HASH]](){{.*}} !type
+; CHECK: define internal void @a() {{.*}}!type
 define internal void @a() !type !0 {
   ret void
 }
diff --git a/llvm/test/Transforms/ThinLTOBitcodeWriter/comdat.ll b/llvm/test/Transforms/ThinLTOBitcodeWriter/comdat.ll
index f9545e2010c10..48d371531ed7b 100644
--- a/llvm/test/Transforms/ThinLTOBitcodeWriter/comdat.ll
+++ b/llvm/test/Transforms/ThinLTOBitcodeWriter/comdat.ll
@@ -25,9 +25,8 @@ $nt = comdat any
 ; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
 @lwt_aliasee = private unnamed_addr global [1 x ptr] [ptr null], comdat($lwt), !type !0
 
-; MERGED: {{@"?lwt_nl[^ ]+}} = hidden unnamed_addr global
-; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
-; THIN: {{@"?lwt_nl[^ ]+}} = external hidden
+; MERGED: @lwt_nl = internal unnamed_addr global i32 0, comdat(${{"?lwt[^ ]+}})
+; THIN: {{@"?lwt_nl\.[^ ]+}} = external hidden unnamed_addr global i32
 @lwt_nl = internal unnamed_addr global i32 0, comdat($lwt)
 
 ; MERGED: @nlwt_aliasee = private unnamed_addr global
@@ -47,12 +46,16 @@ $nt = comdat any
 ; THIN-SAME: comdat($nt)
 @nt_nl = internal unnamed_addr global i32 0, comdat($nt)
 
-; MERGED: {{@"?lwt[^ ]+}} = hidden unnamed_addr alias
-; THIN: {{@"?lwt[^ ]+}} = external hidden
+; MERGED: @lwt = internal unnamed_addr alias [1 x ptr], ptr @lwt_aliasee
+; MERGED: @nlwt_nl = internal unnamed_addr alias [1 x ptr], ptr @nlwt_aliasee
+; MERGED: {{@"?lwt_nl\.[^ ]+}} = hidden alias ptr, ptr @lwt_nl
+; MERGED: {{@"?lwt\.[^ ]+}} = hidden alias ptr, ptr @lwt
+; MERGED: {{@"?nlwt_nl\.[^ ]+}} = hidden alias ptr, ptr @nlwt_nl
+
+; THIN: {{@"?lwt\.[^ ]+}} = external hidden global [1 x ptr]
 @lwt = internal unnamed_addr alias [1 x ptr], ptr @lwt_aliasee
 
-; MERGED: {{@"?nlwt_nl[^ ]+}} = hidden unnamed_addr alias
-; THIN: {{@"?nlwt_nl[^ ]+}} = external hidden
+; THIN: {{@"?nlwt_nl\.[^ ]+}} = external hidden global [1 x ptr]
 @nlwt_nl = internal unnamed_addr alias [1 x ptr], ptr @nlwt_aliasee
 
 ; The functions below exist just to make sure the globals are used.
diff --git a/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal1.ll b/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal1.ll
index 1a6cbb7e7e84b..2c84ab86cac63 100644
--- a/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal1.ll
+++ b/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal1.ll
@@ -13,7 +13,8 @@
 ; BCA1-NOT: <GLOBALVAL_SUMMARY_BLOCK
 
 ; M0: @g.581d7631532fa146ba4061179da39272 = external hidden global i8
-; M1: @g.581d7631532fa146ba4061179da39272 = hidden global i8 42, !type !0
+; M1: @g = internal global i8 42, !type !0
+; M1: @g.581d7631532fa146ba4061179da39272 = hidden alias ptr, ptr @g
 @g = internal global i8 42, !type !0
 
 ; M0: define ptr @f()
diff --git a/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll b/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
index 1a7b1aba66576..d9d9f516015d9 100644
--- a/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
+++ b/llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
@@ -20,8 +20,9 @@ target triple = "x86_64-unknown-linux-gnu"
 ; M1: @g = global ptr @f.13757e0fb71915e385efa4dc9d1e08fd, !type !0
 @g = global ptr @f, !type !0
 
-; M0: define hidden void @f.13757e0fb71915e385efa4dc9d1e08fd()
-; M1: declare !guid !4 hidden void @f.13757e0fb71915e385efa4dc9d1e08fd()
+; M0: @f.13757e0fb71915e385efa4dc9d1e08fd = hidden alias ptr, ptr @f
+; M0: define internal void @f()
+; M1: declare !guid !{{[0-9]+}} hidden void @f.13757e0fb71915e385efa4dc9d1e08fd()
 define internal void @f() {
   call void @f2()
   ret void
diff --git a/llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll b/llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll
index 75932fd84d83e..1d17699be1c3c 100644
--- a/llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll
+++ b/llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll
@@ -10,12 +10,14 @@ define ptr @source() {
 }
 
 ; M0: @g.84f59439b469192440047efc8de357fb = external hidden constant [1 x ptr]
-; M1: @g.84f59439b469192440047efc8de357fb = hidden constant [1 x ptr] [ptr @ok.84f59439b469192440047efc8de357fb]
+; M1: @g = internal constant [1 x ptr] [ptr @ok.84f59439b469192440047efc8de357fb]
+; M1: @g.84f59439b469192440047efc8de357fb = hidden alias ptr, ptr @g
 @g = internal constant [1 x ptr] [
   ptr @ok
 ], !type !0
 
-; M0: define hidden i64 @ok.84f59439b469192440047efc8de357fb
+; M0: @ok.84f59439b469192440047efc8de357fb = hidden alias ptr, ptr @ok
+; M0: define internal i64 @ok(ptr %this)
 ; M1: define available_externally hidden i64 @ok.84f59439b469192440047efc8de357fb
 define internal i64 @ok(ptr %this) {
   ret i64 42

[teresajohnson]

lgtm