ld64.lld fails to link some against some hidden symbols during LTO

[glandium]

(This is a regression from 103e1d934a353ba233f854d992e5429106d3fbac and doesn't happen with ld64).

STR:

• Download and unpack https://drive.google.com/file/d/1CIF4JVMpEP1FDDsEIiMBDwbqPvmA5s80/view?usp=sharing
• cd testcase
• SDK=/path/to/MacOSX.sdk sh command

This currently fails with:

ld64.lld: error: undefined symbol: mozilla::ServoTraversalStatistics::sActive
>>> referenced by /tmp/lto.tmp:(symbol style::driver::traverse_dom::hea1ff5b66fd96c5e+0x2e)
>>> referenced by /tmp/lto.tmp:(symbol style::driver::should_report_statistics::hd9417f01a14da219+0x7)
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

What's peculiar here is that mozilla::ServoTraversalStatistics::sActive is defined as:

@_ZN7mozilla24ServoTraversalStatistics7sActiveE = hidden local_unnamed_addr global i8 0, align 1

in libxul.a(Unified_cpp_layout_style2.o) and

@"\01__ZN7mozilla24ServoTraversalStatistics7sActiveE" = external local_unnamed_addr global i8

in libgkrust.a(style-08fd5a26b9ae3025.3v30u7e8x2rxhb7t.rcgu.o).

[glandium]

It's worth noting that this doesn't happen with the _ZN7mozilla24ServoTraversalStatistics10sSingletonE symbol, which has similar definitions.

[llvmbot]

@llvm/issue-subscribers-lld-macho

[int3]

Interesting. I think 103e1d934a is doing the right thing, but maybe there is a bug in the LTO code itself.

It's worth noting that this doesn't happen with the _ZN7mozilla24ServoTraversalStatistics10sSingletonE symbol, which has similar definitions.

Yeah, both symbols should be externally visible at link time (though not exported in the final binary).

I ran the link with the -save-temps flag and it looks like the import step is responsible for converting the hidden to internal here. Next step is therefore to step through FunctionImport.cpp and see how exactly that happens.

[glandium]

In case that helps: it goes away if I change the code to use _ZN7mozilla24ServoTraversalStatistics7sActiveE instead of \01__ZN7mozilla24ServoTraversalStatistics7sActiveE (and only that one, all the other symbols that start with \01_ stay that way).

[int3]

Yeah, the \01 anti-mangling character seems to be the problem here.

Here's a small test case reproing the failure: https://gist.github.com/int3/e9581f9fd48fe8e69c9a5c29e0cc8f9a

I believe the reason why _ZN7mozilla24ServoTraversalStatistics10sSingletonE isn't affected is because it's not a constant. See this comment: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/FunctionImport.cpp#L1112-L1113

Also it looks like while __ZN7mozilla24ServoTraversalStatistics7sActiveE gets internalized in the import phase, it's the internalize phase that's responsible for tagging it with thinlto-internalize (see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp#L271) which the import phase later acts on.

[int3]

Got a good pointer from @cachemeifyoucan after posting https://discourse.llvm.org/t/thinlto-does-not-account-for-mangle-suppression-prefix-mach-o-issue/65686/. Apparently the old LTO backend faced a similar issue, and has a (hacky) fix. However, cross-module references are handled slightly differently in the new backend, so I can't just port the fix from the old backend. I'm probably going to punt on this for now. @glandium -- are there ways for you to work around this (by e.g. having the Rust compiler not use "\01_")?

[glandium]

The \01_ comes from https://github.com/rust-lang/rust-bindgen/pull/1063. I'm not sure we can easily work around that. @emilio what do you think?

[emilio]

We could try to de-mangle the name in bindgen, remove the \u{1} and hope it's correct / rustc doesn't tweak it or so, though it's not super-appealing (demangling is complex)...

FWIW, the mangled name comes straight from clang (clang_Cursor_getMangling / clang_Cursor_getCXXManglings). Maybe there's another API we could use to get right name for C++? I don't think that exists tho.

[cachemeifyoucan]

Usually when you get into this situation, it is easier if you can consistently get all the names to be emitted with \01_ prefix. You can do that in C languages using asm label (https://gcc.gnu.org/onlinedocs/gcc/Asm-Labels.html)

I don't how feasible it is since this is a building binding and you don't want to change original code to contain all the mangled names.

[glandium]

I'd be inclined to try to address the root issue (as per https://github.com/llvm/llvm-project/issues/57864#issuecomment-1267511067). I don't have the time right now, but I think that's something we could get @serge-sans-paille to look into when he joins Mozilla in a few days.

[serge-sans-paille]

@int3: can you confirm that passing -mllvm -enable-lto-internalization=0 to the linker fixes the issue? That would confirm the symbol prefixed by '\1' gets internalized and then discarded (probably because it has no obvious user)

[serge-sans-paille]

Another note: a quick workaround should be to flag that symbol as used, either through llvm.used or __attribute__((used))

[serge-sans-paille]

Potential patch, I'm still not 100% happy with it but it seems to work on my non-OSX setup:

diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 10ca98f..04ba62e 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -567,6 +567,13 @@ void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
       GlobalRes.IRName = std::string(Sym.getIRName());
     }

+    // If GlobalRes.IRName and Sym.getIRName differ, but we have the same Name,
+    // LLVM mangling escaping is involved, and we can't take the risk of missing
+    // a symbol because it's IR name turn it unused, so keep the symbol.
+    if (GlobalRes.IRName != Sym.getIRName()) {
+      GlobalRes.Partition = GlobalResolution::External;
+    }
+
     // Set the partition to external if we know it is re-defined by the linker
     // with -defsym or -wrap options, used elsewhere, e.g. it is visible to a
     // regular object, is referenced from llvm.compiler.used/llvm.used, or was

@glandium could you give it a try?

[int3]

can you confirm that passing -mllvm -enable-lto-internalization=0 to the linker fixes the issue?

It actually doesn't seem to fix the issue. But your proposed patch makes sense though

[serge-sans-paille]

It actually doesn't seem to fix the issue. But your proposed patch makes sense though

It makes sense but... does it work?

[int3]

Well, I was gonna let @glandium test it since you asked them, and since they were the ones who reported the bug :)

I can give it a go later though

[int3]

Hm I tried that patch and I still see the same error (on OS X, though I don't think platform should matter)

How are you testing it?

[glandium]

I see it too on Linux.

[serge-sans-paille]

My bad, I tested it without -module-summary which triggered the regular LTO path (which also had the bug). I indeed still have the bug with -module-summary which probably triggers the ThinLTO path.

to be continued

[serge-sans-paille]

What about his slight update to the patch?

diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 10ca98f..67f5b7c 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -567,6 +567,14 @@ void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
       GlobalRes.IRName = std::string(Sym.getIRName());
     }

+    // If GlobalRes.IRName and Sym.getIRName differ, but we have the same Name,
+    // LLVM mangling escaping is involved, and we can't take the risk of missing
+    // a symbol because it's IR name turn it unused, so keep the symbol.
+    if (GlobalRes.IRName != Sym.getIRName()) {
+      GlobalRes.Partition = GlobalResolution::External;
+      GlobalRes.VisibleOutsideSummary = true;
+    }
+
     // Set the partition to external if we know it is re-defined by the linker
     // with -defsym or -wrap options, used elsewhere, e.g. it is visible to a
     // regular object, is referenced from llvm.compiler.used/llvm.used, or was

[int3]

The updated patch works :)

[serge-sans-paille]

Great! I'll propose the patch on phabricator then.

[int3]

Hmm I'm taking another look at the condition GlobalRes.IRName != Sym.getIRName() and I think it's actually always going to return true, because every symbol by default will get mangled with a prepended underscore. We probably want to flag just the symbols that will not be mangled due to the leading "\01" character, since that breaks the 1:1 mapping between mangled and un-mangled names.

[serge-sans-paille]

I don't think so: the IRName doesn't get mangled. Let's discuss that in https://reviews.llvm.org/D135710

[speednoisemovement]

This still repros, but only if the prevailing symbol is processed first (see https://crbug.com/372055517). It's easy to repro by reversing the input files here: https://github.com/llvm/llvm-project/blob/e67442486d5efd48235f62b438557bc95193fc48/llvm/test/LTO/X86/hidden-escaped-symbols.ll#L12 When the prevailing symbol is first, the condition GlobalRes.IRName != Sym.getIRName() from https://reviews.llvm.org/D135710 doesn't hold.

The following blunt instrument fixes it by duplicating essentially duplicating the logic from further down.

+++ b/llvm/lib/LTO/LTO.cpp
@@ -645,7 +645,12 @@ void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
       assert(!GlobalRes.Prevailing &&
              "Multiple prevailing defs are not allowed");
       GlobalRes.Prevailing = true;
+      if (!GlobalRes.IRName.empty() && GlobalRes.IRName != Sym.getIRName()) {
+        GlobalRes.Partition = GlobalResolution::External;
+        GlobalRes.VisibleOutsideSummary = true;
+      }
       GlobalRes.IRName = std::string(Sym.getIRName());

But it feels like there should be something better. Is it absolutely necessary to have a data layout if we know we want Mach-O prefix mangling (by plumbing it through lto::InputFile for example)?