[llvm] [ThinLTO] Record prevailing GUIDs by linker symbol name (PR #226911)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lto

Author: Fabian Meumertzheim (fmeum)

<details>
<summary>Changes</summary>

`LTO::addThinLTO` updates `GlobalResolutions` using the IR name from a summary, but that map is keyed by mangled linker symbol names. On Mach-O, the IR name `_foo` can therefore match the resolution for `foo` (whose linker name is `_foo`). This assigns `_foo`'s GUID to `foo` and can incorrectly internalize `foo` even when another module references it, producing an undefined-symbol error.

Move the GUID assignment into the symbol loop, where `Sym.getName()` identifies the correct resolution and the GUID has already been obtained using `Sym.getIRName()`. Preserve the prevailing-definition check and the existing fallback for bitcode without a GUID table.

This fixes a regression introduced by 39dcb0ff91b312bb269334168d5e32be78b60417 (#<!-- -->201849). It surfaced when linking mimalloc 3.5.3 into a PGO/ThinLTO build of LLVM on macOS. The standalone reproducer below needs only Clang, LLD, and the macOS SDK.

Reproducer on macOS: set `LLVM_BIN` to the absolute path of the LLVM `bin` directory, then run:

```sh
set -eu
: "${LLVM_BIN:?Set LLVM_BIN to the directory containing clang and ld64.lld}"
repro_dir=$(mktemp -d)
cd "$repro_dir"

cat > defs.c <<'C'
volatile int v;
__attribute__((noinline)) int foo(void) { return v; }
__attribute__((noinline)) int _foo(void) { return v + 1; }
C
cat > main.c <<'C'
int foo(void);
int _foo(void);
int main(void) { return foo() + _foo() != 1; }
C

"$LLVM_BIN/clang" -O2 -flto=thin -fuse-ld=lld \
  --ld-path="$LLVM_BIN/ld64.lld" \
  -isysroot "$(xcrun --show-sdk-path)" \
  -Wl,-mllvm,-import-instr-limit=0 defs.c main.c -o repro
./repro
```

With unpatched LLVM 23.1.2, the link fails:

```text
ld64.lld: error: undefined symbol: foo
>>> did you mean: _foo
```

With the patch, it links successfully and `./repro` exits with status 0. Disabling ThinLTO importing keeps the cross-module references visible in this small example.

The added two-module `llvm-lto2` regression checks that both `foo` and `_foo` remain externally defined after importing. Before the fix, `foo` incorrectly becomes `internal`.

Assisted-by: OpenAI Codex


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


2 Files Affected:

- (modified) llvm/lib/LTO/LTO.cpp (+1-5) 
- (added) llvm/test/ThinLTO/X86/macho-guid-collision.ll (+41) 


``````````diff
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 50232bf87d257..a8d4bed429593 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -1199,11 +1199,6 @@ LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
             auto IT = IRSpecifiedGUIDs.insert({VI.name(), VI.getGUID()});
             (void)IT;
             assert(IT.second);
-            if (auto GRIt = GlobalResolutions->find(VI.name());
-                GRIt != GlobalResolutions->end() &&
-                Prevailing.count(VI.name())) {
-              GRIt->second.setGUID(VI.getGUID());
-            }
           }))
     return Err;
   LLVM_DEBUG(dbgs() << "Module " << BMID << "\n");
@@ -1224,6 +1219,7 @@ LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
     if (!Sym.getIRName().empty() &&
         (R.Prevailing || R.FinalDefinitionInLinkageUnit)) {
       if (R.Prevailing) {
+        (*GlobalResolutions)[Sym.getName()].setGUID(GUID);
         ThinLTO.setPrevailingModuleForGUID(GUID, BMID);
         // For linker redefined symbols (via --wrap or --defsym) we want to
         // switch the linkage to `weak` to prevent IPOs from happening.
diff --git a/llvm/test/ThinLTO/X86/macho-guid-collision.ll b/llvm/test/ThinLTO/X86/macho-guid-collision.ll
new file mode 100644
index 0000000000000..ced1651fa42cd
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/macho-guid-collision.ll
@@ -0,0 +1,41 @@
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/defs.ll -o %t/defs.bc
+; RUN: opt -module-summary %t/main.ll -o %t/main.bc
+; RUN: llvm-lto2 run %t/defs.bc %t/main.bc -o %t/out -save-temps \
+; RUN:   -import-instr-limit=0 \
+; RUN:   -r=%t/defs.bc,_foo,pl -r=%t/defs.bc,__foo,pl \
+; RUN:   -r=%t/main.bc,_main,plx -r=%t/main.bc,_foo,l \
+; RUN:   -r=%t/main.bc,__foo,l
+; RUN: llvm-dis %t/out.1.3.import.bc -o - | FileCheck %s
+
+;; Global resolutions use mangled symbol names, whereas the summary records IR
+;; names. Looking up the IR name _foo in the resolutions for Mach-O incorrectly
+;; assigns its GUID to foo and allows foo to be internalized.
+; CHECK: define dso_local i32 @foo()
+; CHECK: define dso_local i32 @_foo()
+
+;--- defs.ll
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.15.0"
+
+define i32 @foo() noinline {
+  ret i32 1
+}
+
+define i32 @_foo() noinline {
+  ret i32 2
+}
+
+;--- main.ll
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.15.0"
+
+declare i32 @foo()
+declare i32 @_foo()
+
+define i32 @main() {
+  %a = call i32 @foo()
+  %b = call i32 @_foo()
+  %sum = add i32 %a, %b
+  ret i32 %sum
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/226911