[RemoveDIs] Load into new debug info format by default in llvm-link

[OCHyams]

Directly load all bitcode into the new debug info format in llvm-link. This means that new-mode bitcode no longer round-trips back to old-mode after parsing, and that old-mode bitcode gets auto-upgraded to new-mode debug info (which is the current in-memory default in LLVM).

[jryans]

Seems sensible to do this for llvm-link, thanks!

[jryans]

Ultra-nit: Maybe add a blank above this to separate it from the "default" parsing step.

[jsji]

Looks like this might cause assert failure for some scenarios.

Simple reproducer on x86 Linux:

# Build clang/llvm with -DLLVM_ENABLE_ASSERTIONS=ON

$cat t.cpp
int __attribute__((always_inline)) func(int x) {
   return x * 2;
 }
$ clang++ -g -c -emit-llvm t.cpp
$ llvm-link t.bc

Unknown function for CallBase upgrade.
UNREACHABLE executed at .../llvm-project/llvm/lib/IR/AutoUpgrade.cpp:4268!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: bin/llvm-link t.bc
 #0 0x00007f06b58af4b1 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) .../llvm-project/build/bin/../lib/libLLVMSupport.so.19.0git+0x1cd4b1)
 #1 0x00007f06b58aca14 SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f06b51f8db0 __restore_rt (/lib64/libc.so.6+0x59db0)
 #3 0x00007f06b524542c __pthread_kill_implementation (/lib64/libc.so.6+0xa642c)
 #4 0x00007f06b51f8d06 gsignal (/lib64/libc.so.6+0x59d06)
 #5 0x00007f06b51cb7d3 abort (/lib64/libc.so.6+0x2c7d3)
 #6 0x00007f06b57ceada (.../llvm-project/build/bin/../lib/libLLVMSupport.so.19.0git+0xecada)
 #7 0x00007f06b5ae5126 llvm::UpgradeIntrinsicCall(llvm::CallBase*, llvm::Function*) (.../llvm-project/build/bin/../lib/libLLVMCore.so.19.0git+0x13c126)
 #8 0x00007f06b5fb9e38 (anonymous namespace)::BitcodeReader::materialize(llvm::GlobalValue*) BitcodeReader.cpp:0:0
 #9 0x00007f06b5c91de7 llvm::Module::materialize(llvm::GlobalValue*) (.../llvm-project/build/bin/../lib/libLLVMCore.so.19.0git+0x2e8de7)
#10 0x00007f06b5bf6d81 llvm::GlobalValue::materialize() (.../llvm-project/build/bin/../lib/libLLVMCore.so.19.0git+0x24dd81)
#11 0x00007f06b65c0330 (anonymous namespace)::IRLinker::materialize(llvm::Value*, bool) IRMover.cpp:0:0
#12 0x00007f06b64b35ad (anonymous namespace)::Mapper::mapValue(llvm::Value const*) ValueMapper.cpp:0:0
#13 0x00007f06b64b8989 llvm::ValueMapper::mapValue(llvm::Value const&) (.../llvm-project/build/bin/../lib/libLLVMTransformUtils.so.19.0git+0x2c2989)
#14 0x00007f06b65c7416 llvm::IRMover::move(std::unique_ptr<llvm::Module, std::default_delete<llvm::Module>>, llvm::ArrayRef<llvm::GlobalValue*>, llvm::unique_function<void (llvm::GlobalValue&, std::function<void (llvm::GlobalValue&)>)>, bool) (.../llvm-project/build/bin/../lib/libLLVMLinker.so.19.0git+0x15416)
#15 0x00007f06b65d1392 (anonymous namespace)::ModuleLinker::run() LinkModules.cpp:0:0
#16 0x00007f06b65d177c llvm::Linker::linkInModule(std::unique_ptr<llvm::Module, std::default_delete<llvm::Module>>, unsigned int, std::function<void (llvm::Module&, llvm::StringSet<llvm::MallocAllocator> const&)>) (.../llvm-project/build/bin/../lib/libLLVMLinker.so.19.0git+0x1f77c)
#17 0x000000000040df9d linkFiles(char const*, llvm::LLVMContext&, llvm::Linker&, llvm::cl::list<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, llvm::cl::parser<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, unsigned int) llvm-link.cpp:0:0
#18 0x000000000040659b main (bin/llvm-link+0x40659b)
#19 0x00007f06b51e3e50 __libc_start_call_main (/lib64/libc.so.6+0x44e50)
#20 0x00007f06b51e3efc __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x44efc)
#21 0x0000000000407ae5 _start (bin/llvm-link+0x407ae5)
Aborted (core dumped)

[SLTozer]

Looks like this might cause assert failure for some scenarios.

I think I see the issue, I think it's incidentally fixed by a PR that I have open - the rough outline is that the bitcode loading process for the new debug info format can hit this assertion if we lazy load, and change the debug info format of the module in between parsing the module and materializing its functions; in more detail:

• We tell the autoupgrader during the parse step that all debug intrinsics should be upgraded to debug records because we desire the new format.
• Then, we change the module to be in the old debug info format (IRMover.cpp:1552).
• Then, we materialize the function, converting the inserted debug records to debug intrinsics due to the parent module using the old debug info format.
• Then, we try to upgrade intrinsics, at which point the module is still in the old debug info format, but we have already requested that debug intrinsics be upgraded; we thus fall into the llvm_unreachable here:

    } else if (IsDbg && CI->getModule()->IsNewDbgInfoFormat) {
      upgradeDbgIntrinsicToDbgRecord(Name, CI);
    } else {
      llvm_unreachable("Unknown function for CallBase upgrade.");
    }

A simple immediate fix for this would be to avoid catching IsNewDbgInfoFormat in the clause above, but instead fold it as a separate check:

    } else if (IsDbg) {
      if (CI->getModule()->IsNewDbgInfoFormat)
        upgradeDbgIntrinsicToDbgRecord(Name, CI);
    } else {
      llvm_unreachable("Unknown function for CallBase upgrade.");
    }

It's not an ideal solution, but it only needs to be temporary since a patch that fixes it is already ready.

[OCHyams]

Thanks for the report and analysis.

I think I see the issue, I think it's incidentally fixed by a PR that I have open

Do you have a link to it?

A simple immediate fix for this would be to avoid...

We could also revert this till your patch lands if it fixes the issue.

[SLTozer]

My mistake, my existing patch doesn't fix the issue (the reproducer needed ToT clang as well as llvm-link), but the fix is straightforward enough - I've added one here, which has a slight overlap with my other review (which adds a new flag). @jsji does the linked patch fix all your crashes if applied?

[jsji]

My mistake, my existing patch doesn't fix the issue (the reproducer needed ToT clang as well as llvm-link), but the fix is straightforward enough - I've added one here, which has a slight overlap with my other review (which adds a new flag). @jsji does the linked patch fix all your crashes if applied?

Yes, looks like #87494 fixed the crashes. Thanks.