Differential D143229
[AutoUpgrade] Add flag to disable autoupgrading debug info Authored by aeubanks on Feb 2 2023, 4:14 PM.
Details Auto-upgrade can be expensive, especially UpgradeDebugInfo() since it runs the verifier. With this patch, we can specify that the imported bitcode is built with the same revision of LLVM, meaning there's no need to run any auto-upgrading. For now, limit this to just debug info since projects like Rust want to support multiple versions of LLVM at the same time (at compiler build time, not at Rust source code build time) and run a subset of the autoupgrade functionality for simplicity.
Diff Detail
Unit Tests Event TimelineComment Actions
I am pretty sure ThinLTO was designed without this restriction, where is it coming from? Comment Actions Hmm I thought I remembered this being true, I may be wrong though. Hopefully others can comment. Comment Actions A flag seems a bit ad-hoc to me, can we detect from the bitcode itself if this is necessary? Comment Actions Yeah, my understanding's with @mehdi_amini too - that ThinLTO should be/is currently compatible with archived bitcode (Apple folks would be the ones most likely to speak to that, perhaps @tejohnson knows who's best to rope in on their side these days), it's just that our (Google) internal use doesn't need this flexibility and is paying a penalty for it. The cost of running the IR verifier for debug info might interest @aprantl and @JDevlieghere in case they've got ideas about other ways to address this or might just be interested in knowing it's hot and might be worth improvements, even if Google still ends up turning it off entirely. Comment Actions Not really that I know of - if it's untrusted bitcode, everything's got to be validated/can't just check a version flag and assume it's good. Having some generic flag for "this bitcode is trusted" might be a nice general purpose thing to have for users that can ensure that. Comment Actions Right, it is not a general requirement for ThinLTO that the input bitcode must be built with the same revision. Inside our Google builds this does end up being the case, however, because of the way the build system works. So we would need to put it under a flag. Comment Actions The description in this revision isn't about "trust" but about "matching revision", can you elaborate what you have in mind as "trust" issue here? Comment Actions I don't think this restriction works for Apple. Compatibility is important for users that needs external dependencies that they do not control. If the debug info upgrader is expensive for you, we can probably add a fast check to skip that, or just give user a flag to control that behavior. Comment Actions my assumption was wrong then what about checking if the llvm.idents (embedded by the frontend) of the modules match? although that wouldn't work for cross-language LTO, but it's a start Comment Actions If my source module is bitcode upgraded, it won't get a new llvm.ident right? Then the upgrade pass will be wrongly skipped. I think we need a LLVM code generator version somewhere in the IR Comment Actions it the bitcode was upgraded, then during the upgrade it should already have had UpgradeDebugInfo called on it (with the expected LLVM revision) right? Comment Actions I don't think you can / should trust llvm.ident. It shouldn't be given a semantic meaning Comment Actions Plus this isn't even a single field, it's a list of many versions (unfortunately it's not deduplicated either) Comment Actions I am talking about A.bc imports function from B.bc. Both of bitcode files are out of date, so A.bc gets upgraded when loaded, then A tries to import B, they have the same ident so the upgrade from B is skipped. Comment Actions In the sense that if I write some handcrafted IR, LLVM should verify it before moving forward because other things interacting with the IR will assume it's valid & may crash if it isn't. So there's a trust boundary, generally - like in memory IR usually isn't verified, it's assumed the producer produced something valid by construction & it's a bug on the producer side if it didn't. But generally once we're reading things off disk we assume it might have come from anywhere & to be somewhat stable/not so crashy, we usually verify it on input (at least that was/is my rough understanding). So if you happen to be willing to sacrifice that extra stability guarantee because because you can guarantee that outside the system outside the program will ensure that only valid IR is passed in - then maybe having a flag to communicate that guarantee would be appropriate. Comment Actions I guess it depends what the contract is for UpgradeDebugInfo, because from the name it didn't seem to me like a "verifier"? Comment Actions Oh I misread the verifier code, yes you're right. I see, yes you're right. At this point I agree that a flag would be what I'd try for now. UpgradeDebugInfo runs the verifier, and the verifier has a mode that doesn't fail if the debug info is invalid, but rather reports it separately. We could separate out the debug info portion of the verifier into a separate DIVerifier and use that instead. But we're still doing unnecessary work verifying debug info that we can assume to be valid in our build environments. Comment Actions Looking at the implementation: /// Check the debug info version number, if it is out-dated, drop the debug
/// info. Return true if module is modified.
bool llvm::UpgradeDebugInfo(Module &M) {
unsigned Version = getDebugMetadataVersionFromModule(M);
if (Version == DEBUG_METADATA_VERSION) {
bool BrokenDebugInfo = false;
if (verifyModule(M, &llvm::errs(), &BrokenDebugInfo))
report_fatal_error("Broken module found, compilation aborted!");
if (!BrokenDebugInfo)
// Everything is ok.
return false;
else {
// Diagnose malformed debug info.
DiagnosticInfoIgnoringInvalidDebugMetadata Diag(M);
M.getContext().diagnose(Diag);
}
}
...
So are we re-running the IR verifier independently? And if we don't what kind of inconsistency would we be setting up? Comment Actions We don't typically run the verifier in assert builds because it's expensive. We assume that input bitcode passes the verifier.
Comment Actions The name "upgrade" comes from a time when regularly bumped DEBUG_METADATA_VERSION. Back then it "upgraded" older debug info formats by dropping the outdated metadata. In the more recent past we are trying to make debug info upgradeable in the bit code reader. Unfortunately, because debug info metadata is a cyclic graph we keep finding bugs where the metadata produced by older versions of LLVM and/or 3rd party frontends is malformed in a way that causes problems (crashes, infinite loops, ...) with newer version of LLVM. Whenever we find such a problem we improve the IR Verifier to detect the malformed IR. Because of this, the upgrade function insists on running the Verifier. If it finds malformed debug info it will perform an "upgrade" by dropping the malformed debug info.
Comment Actions It seems reasonable to me to provide a way to skip this for situations where the build system guarantees that the bitcode is always generated by the same compiler, e.g. in a distributed ThinLTO build where any change in compiler will force a rebuild anyway, which is the case @aeubanks is looking at. @aeubanks are we also invoking UpgradeDebugInfo, or the Verifier separately, on the main input bitcode in the distributed ThinLTO backend clang invocation (e.g. the clang -x ir input file)? For the same reason, we should be able to skip that. So I wonder if it makes sense then for the disabling mechanism (e.g. flag or whatever) to be in UpgradeDebugInfo or the Verifier itself? Comment Actions
I'm having a hard time imagining how a build system would be able to guarantee this, especially if multiple compiler frontends are involved. The only scenario where this makes sense if you are only doing clean world builds with only clang as the compiler. Maybe that's what you are doing, but IMHO that is such a niche use-case that I'd rather see such a flag being implemented downstream and not in the main llvm-project. As @mehdi_amini pointed out, it complicates the qualification matrix quite a lot. Comment Actions The compiler version being used is centrally controlled. And we're not always doing clean world builds, but Bazel's build cache is queried by a hash of all action inputs, including the compiler bits, so if anything changes it forces a rebuild. We use stock clang/LLVM, so adding an option downstream is not possible. Taking a step back, though, the issue here is that we are verifying the same module multiple times - once when it is read to be compiled through its own ThinLTO backend (either via the linker for in-process ThinLTO, or via clang for distributed ThinLTO as we are doing), and then once per every other ThinLTO backend that imports anything from it. If the debug info is possibly out of date or untrusted then this is unavoidable. But in any build system where the inputs are expected to be from the same trusted source compilers it would be nice to have a mode whereby when the bitcode is read for compiling through its own ThinLTO backend we perform the UpgradeDebugInfo and if the verifier there fails, it is a hard error. Then skip the upgrading during the function importing process. This doesn't seem particularly niche to me. You are guaranteed that the build will simply fail if the assumption is wrong. This patch afaict only does the latter part of this, by skipping the upgrade/verification during function importing, so would need a bit more work to tie that to a guarantee that we do the upgrade/verification with a hard error during the read for each module's ThinLTO backend. Comment Actions So your build system guarantees that all bitcode that is being consumed was generated by compilers linked against the same version of LLVM that is used in the LTO plugin and if that version were to serialize malformed metadata, you wouldn't be any worse off than a non-LTO build, which also doesn't verify between passes. Based on that assumption, this option does make sense to me. As others pointed out, it would be nice if the version number could be encoded in the bitcode itself, however, that doesn't fix the aforementioned trust boundary issue, so you're probably no better off than with this flag. Considering all this, this patch gets an unenthusiastic LGTM from my side :-) Comment Actions In chrome, we add a clang -D flag containing the toolchain revision for caching/determinism reasons so we can also guarantee that bitcode inputs will always match. I may be wrong, but I believe rust does inter-crate (Thin)LTO, all bitcode produced from the same rustc frontend, which could also use this. Yeah there is some time spent verifying the main input bitcode (although much less than function importing). Moved the flag to autoupgrade.
Comment Actions
Conceptually yes, we don't need verification for compiler-driven LTO (which is the default). We wouldn't be able to use it as implemented though, because we do need to auto-upgrade intrinsics in the initial IR, while this effectively "breaks" UpgradeCallsToIntrinsic(). Comment Actions Perhaps then it should go back to the original version that just skipped the UpgradeDebugInfo during cross-module importing - would that work for you? Comment Actions Rust upgrades intrinsics after initial IR generation (i.e. before the pre-link pipeline). The pre-link and post-link pipeline are part of the same process, so they can't use different opt options. Comment Actions Then the next question is, why does Rust need to upgrade intrinsics after initial IR generation, rather than constructing initially up-to-date intrinsic calls? Comment Actions std::arch defines bindings to LLVM target intrinsics, and these need to work across multiple supported LLVM versions.
Comment Actions I've only skimmed the discussion, but FTR, Sony licensees are also in a single-compiler environment, which is enforced in a couple of ways. In particular we have a downstream patch to insert the compiler version into the bitcode (modeled on similar code for Darwin IIRC) and the linker will complain if it sees a different version, before starting the LTO machinery. I'm thinking we should have (a downstream patch to make) the linker set this flag when it invokes the LTO machinery? because it has already done a check? | ||||||||||||||||||||||||||||||||||||||||||
I suppose this text is no longer accurate?