[PATCH] D143229: [FunctionImporter] Add flag to disable upgrading debug info

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D143229#4126345 <https://reviews.llvm.org/D143229#4126345>, @aprantl wrote:

>> 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.
>
> 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.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143229/new/

https://reviews.llvm.org/D143229