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

[glandium]

(This is a regression from 103e1d9 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 103e1d9 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 rust-lang/rust-bindgen#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 #57864 (comment)). 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)