[lld/mac] Crash with thinlto and --start-lib / --end-lib

[nico]

https://drive.google.com/file/d/1ju1O-uXLu4JGq_FbVb3wu4ugiZ_MAWPR/view?usp=sharing

Assertion failed: (file.lazy), function extract, file InputFiles.cpp, line 2224.
Process 12725 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
    frame #4: 0x00000001007dae48 ld64.lld`lld::macho::extract(lld::macho::InputFile&, llvm::StringRef) + 300
ld64.lld`lld::macho::extract:
->  0x1007dae48 <+300>: bl     0x106b3f788               ; symbol stub for: __asan_handle_no_return
    0x1007dae4c <+304>: adrp   x0, 27829
    0x1007dae50 <+308>: add    x0, x0, #0xb80            ; __func__._ZN4llvm4castIN3lld5macho18ConcatInputSectionENS2_12InputSectionEEEDcPT0_
    0x1007dae54 <+312>: adrp   x1, 27829
Target 0: (ld64.lld) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
    frame #0: 0x0000000185632d98 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x0000000185667ee0 libsystem_pthread.dylib`pthread_kill + 288
    frame #2: 0x00000001855a2340 libsystem_c.dylib`abort + 168
    frame #3: 0x00000001855a1754 libsystem_c.dylib`__assert_rtn + 272
  * frame #4: 0x00000001007dae48 ld64.lld`lld::macho::extract(lld::macho::InputFile&, llvm::StringRef) + 300
    frame #5: 0x0000000100835dc0 ld64.lld`lld::macho::SymbolTable::addUndefined(llvm::StringRef, lld::macho::InputFile*, bool) + 1096
    frame #6: 0x00000001007da798 ld64.lld`lld::macho::BitcodeFile::parse() + 2056
    frame #7: 0x0000000100835dc0 ld64.lld`lld::macho::SymbolTable::addUndefined(llvm::StringRef, lld::macho::InputFile*, bool) + 1096
    frame #8: 0x00000001007da798 ld64.lld`lld::macho::BitcodeFile::parse() + 2056
    frame #9: 0x0000000100837168 ld64.lld`lld::macho::SymbolTable::addLazyObject(llvm::StringRef, lld::macho::InputFile&) + 592
    frame #10: 0x00000001007d9ecc ld64.lld`lld::macho::BitcodeFile::parseLazy() + 504
    frame #11: 0x00000001007d967c ld64.lld`lld::macho::BitcodeFile::BitcodeFile(llvm::MemoryBufferRef, llvm::StringRef, unsigned long long, bool, bool) + 3960
    frame #12: 0x000000010078109c ld64.lld`addFile(llvm::StringRef, LoadType, bool, bool, bool, bool) + 2464
    frame #13: 0x0000000100777da8 ld64.lld`lld::macho::link(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, bool, bool) + 64400
    frame #14: 0x00000001000022fc ld64.lld`lldMain(int, char const**, llvm::raw_ostream&, llvm::raw_ostream&, bool) + 3068
    frame #15: 0x00000001000013d0 ld64.lld`lld_main(int, char**) + 696
    frame #16: 0x000000010b1c108c dyld`start + 520

If you link with -t -why_load, it prints

...
obj/content/shell/content_shell_app/shell_crash_reporter_client.o
__ZN7content24ShellCrashReporterClientC1Ev forced load of obj/content/shell/content_shell_app/shell_crash_reporter_client.o
obj/content/shell/content_shell_app/shell_crash_reporter_client.o
__ZTVN7content24ShellCrashReporterClientE forced load of obj/content/shell/content_shell_app/shell_crash_reporter_client.o

So the file forces a load of itself!

We should definitely do bd448f0 in the MachO port (see also D106293), but doing just that isn't enough: We hit some other assert later on then.

Need to understand what's going on first and make a reduced repro.

[nico]

I'm not an LTO expert, but it looks surprising to me that shell_crash_reporter_client.o has __ZTVN7content24ShellCrashReporterClientE twice in its symbol table:

% ~/src/llvm-project/out/gn/bin/llvm-nm -m obj/content/shell/content_shell_app/shell_crash_reporter_client.o | rg __ZTVN7content24ShellCrashReporterClientE
---------------- (LTO,RODATA) private external __ZTVN7content24ShellCrashReporterClientE
                 (undefined) external __ZTVN7content24ShellCrashReporterClientE

[nico]

(On the third hand, this same link completes without problems when using thin archives instead of --start-lib / --end-lib.)

[nico]

adding @MaskRay since he implemented --start-lib in lld/MachO.

[llvmbot]

@llvm/issue-subscribers-lld-macho

[nico]

From llvm-modextract -n 0 -o - obj/content/shell/content_shell_app/shell_crash_reporter_client.o | llvm-dis -o -:

@_ZTVN7content24ShellCrashReporterClientE = external hidden unnamed_addr constant { [13 x ptr] }, align 8

From llvm-modextract -n 1 -o - obj/content/shell/content_shell_app/shell_crash_reporter_client.o | llvm-dis -o -:

@_ZTVN7content24ShellCrashReporterClientE = hidden unnamed_addr constant { [13 x ptr] } { [13 x ptr] [ptr null, ...

As far as I know, -n 0 is the actual bitcode, while -n 1 is the thinlto index of the file.

So the vtable is an undef external in the actual bitcode, but it's defined in the index (?)

That seems surprising, but maybe it's normal? (@pcc too because thinlto)

Anyways, llvm-nm uses SymbolicFile and IRObjectFile just gets the symbols from all the file's modules, which explains why llvm-nm prints the symbol twice, once defined and once not.

[nico]

llvm-lto2 dump-symtab obj/content/shell/content_shell_app/shell_crash_reporter_client.o uses the same codepath for dumping the symbol table as the one that lld uses:

% llvm-lto2 dump-symtab obj/content/shell/content_shell_app/shell_crash_reporter_client.o | rg __ZTVN7content24ShellCrashReporterClientE
HU------ __ZTVN7content24ShellCrashReporterClientE
H------- __ZTVN7content24ShellCrashReporterClientE

Other symbols in that file are also present twice, but for others the defined symbol comes before the undefined one (i.e. the definition is in the main bitcode file, instead of in the index.)

[nico]

Note to self: clang calls llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h from clang/lib/CodeGen/BackendUtil.cpp to write a thinlto'd bitcode output file, with separate index module.

lld calls into llvm/lib/LTO/ThinLTOCodeGenerator.cpp to link those thin bitcodes.

[nico]

Looks like it's normal that the vtable def goes in the index:

% cat vtable.cc                                                           
struct S {
  S();

  virtual void f();
};

S::S() {}

void S::f() {}
% clang -c vtable.cc -flto=thin -fsplit-lto-unit

% out/gn/bin/llvm-modextract -n 0 -o - vtable.o | out/gn/bin/llvm-dis -o - | rg _ZTV1S
@_ZTV1S = external unnamed_addr constant { [3 x i8*] }, align 8
  store i32 (...)** bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTV1S, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 (...)*** %4, align 8

% out/gn/bin/llvm-modextract -n 1 -o - vtable.o | out/gn/bin/llvm-dis -o - | rg _ZTV1S
@_ZTV1S = unnamed_addr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* bitcast ({ i8*, i8* }* @_ZTI1S to i8*), i8* bitcast (void ()* @_ZN1S1fEv to i8*)] }, align 8, !type !0, !type !1

[nico]

Here's a reduced repro:

% cat vtable.cc 
struct S {
  S();

  virtual void f();
};

S::S() {}

void S::f() {}

% cat vtable_use.cc 
struct S {
  S();

  virtual void f();
};

int main() {
  S s;
}

% out/gn/bin/clang -c vtable_use.cc vtable.cc -flto=thin -fsplit-lto-unit          

% out/gn/bin/clang++ -Wl,--start-lib vtable.o -Wl,--end-lib vtable_use.o -isysroot $(xcrun -show-sdk-path) -fuse-ld=lld -fsplit-lto-unit                            
Assertion failed: (file.lazy), function extract, file InputFiles.cpp, line 2224.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /Users/thakis/src/llvm-project/out/gn/bin/ld64.lld -demangle -dynamic -arch arm64 -platform_version macos 12.0.0 13.0 -syslibroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -o a.out --start-lib vtable.o --end-lib vtable_use.o -lc++ -lSystem
clang: error: unable to execute command: Abort trap: 6
clang: error: linker command failed due to signal (use -v to see invocation)

[nico]

This is almost certainly the right fix:

% git diff
diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index bedc273c5283..1af42b337065 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -2212,8 +2212,10 @@ void BitcodeFile::parseLazy() {
 }
 
 void macho::extract(InputFile &file, StringRef reason) {
+  if (!file.lazy)
+    return;
+
   printArchiveMemberLoad(reason, &file);
-  assert(file.lazy);
   file.lazy = false;
   if (auto *bitcode = dyn_cast<BitcodeFile>(&file)) {
     bitcode->parse();

[nico]

https://reviews.llvm.org/D139199 is that plus a test. The test asserts with out the fix but not with it, so that's good.

However, porting bd448f0 instead of that fix also makes it pass, but as mentioned above, porting bd448f0 isn't enough to make my larger, unreduced repro work.

So we should add a more involved test for that too at some point.