LTO pipeline crashes when incompatible bitcode gets used in an LTO build

[ilovepi]

A while back, our toolchain shortly moved the Rust compiler ahead of our clang distribution. As a result LTO bitcode from rustc was used in an older LLD. While we don't expect this to be supported (its definitely wrong and we've taken steps to ensure that doesn't happen again), it did crash LLD in the LTO pipeline and trigger an assert in a asserts enabled builds. Our sole contention is that we don't think LLVM should crash because the bitcode was incompatible. Fatal errors or some other failure mechanism would be preferable.

#147260 introduced the bitcode change that caused the incompatibility. In our opinion this isn't a problem w/ their patch, but rather the failure mechanism in the LTO pipeline and/or the detection mechanisms available for incompatible IR versioning.

Error:

ld.lld: /usr/local/google/home/paulkirth/llvm-fork/llvm/lib/Bitcode/Reader/MetadataLoader.cpp:349: (anonymous namespace)::(anonymous namespace)::PlaceholderQueue::~PlaceholderQueue(): Assertion `empty() && "PlaceholderQueue hasn't bee
n flushed before being destroyed"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
#0 0x00005615294ee7b8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/paulkirth/llvm-fork/llvm/lib/Support/Unix/Signals.inc:804:13
[1]    4027137 IOT instruction (core dumped)  ~/llvm-fork/build/bin/ld.lld @response.txt

Linker reproducer: https://g-issues.fuchsia.dev/action/issues/430381743/attachments/68029508?download=true

Looking at the assertion failure, and stepping through this in a debugger, the "fix" could just to be throw a fatal error in the destructor of PlaceHolderQueue if it isn't empty (turn the assertion into an error). That doesn't sound to me like a great solution, but would probably work for cases like this, where there is new metadata and it won't be handled here.

Does anyone have an idea of a better way to detect this class of problem? or at least prevent this kind of crash the next time bitcode formatting has an update? To be clear I don't think we should go out of our way to try and support this situation, I just don't think out toolchain should crash if we can avoid it. This is an attempt my attempt to try and avoid it, but I'm fine if we think this is too much work.

CC: @MaskRay @nikic

[nikic]

Looking at the assertion failure, and stepping through this in a debugger, the "fix" could just to be throw a fatal error in the destructor of PlaceHolderQueue if it isn't empty (turn the assertion into an error). That doesn't sound to me like a great solution, but would probably work for cases like this, where there is new metadata and it won't be handled here.

Most likely that assertion needs to be removed altogether. If I understand correctly, it will hit whenever one of the error return paths is hit while there are placeholders in the queue. We either need to drop the assert or explicitly clear the queue on all error returns with the queue in scope.

---

Generally, bitcode parsing is supposed to never crash, but the parser is not hardened against this. If you run the llvm-dis fuzzer, it'll take about 1ms to find a new crash :) These issues get fixed on a best effort basis.

[cachemeifyoucan]

I would love to see bitcode parser is more rigid so that I can be run as a library without taking down the host application (that means fatal_error is also not an option) but we are very far from it.

We do have a version encoded bitcode (IDENTIFICATION_BLOCK_ID) which can be used to reject bitcode versions known to be incompatible but it is not currently used (BITCODE_CURRENT_EPOCH is never bumped).

[bd1976bris]

For PlayStation toolchains, we rev-lock the bitcode. The mechanism for this involves adding additional version information using the BitCodeWrapperFormat. Perhaps such a scheme could be useful?