[PATCH] D52952: [IR][Modules] Improve quality of diagnostic messages for non-fatal warnings during module linking.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D52952#1258139, @kristina wrote:

> In https://reviews.llvm.org/D52952#1258114, @dblaikie wrote:
>
> > Could this be tested with two .ll files with mismatched DWARF Versions, lto'd together like other LTO tests (in, for example, lld/test/ELF/lto/)
>
>
> This would require introducing a category of tests for IR linker diagnostics,


I'm not sure what you mean by a new category of tests - lit tests are fairly general purpose. Inputs and expected output.

Here's one I created for this code change:

  test/Linker/metadata-mismatch.test:
    ; RUN: llvm-link %p/Inputs/metadata-mismatch-a.ll %p/Inputs/metadata-mismatch-b.ll -S 2>&1 | FileCheck %s
    ; CHECK: warning: linking conflicting DWARF versions: v5 from {{.*}}/metadata-mismatch-b.ll to v4 in llvm-link
  test/Linker/Inputs/metadata-mismatch-a.ll:
    !llvm.module.flags = !{!1}
    !1 = !{i32 2, !"Dwarf Version", i32 4}
  test/Linker/Inputs/metadata-mismatch-b.ll:
    !llvm.module.flags = !{!1}
    !1 = !{i32 2, !"Dwarf Version", i32 5}

>   which currently do not exist, likely due to their relative insignificance and a somewhat (from my perspective, given how often modulemaps are forgotten and not updated and it not being noticed for a while) low number of module users overall, 

C++/clang modules, you mean? This diagnostic is independent of that, right? It could hit anyone using LTO (at least Apple, Google, and Sony use it extensively, so far as I know?) where a build passed different -gN flags to different compiles (pretty uncommon - which is why people probably aren't too worried aobut these diagnostic experiences)

> especially for projects outside of LLVM-related scope

LLVM-related scope?

> (and as I said, the actual diagnostics emitted and their validity/suitability have zero coverage,

Yeah, pity about the lack of coverage - which is why it's great to try to fix that when the opportunity arises, like now.

> hence my remark about it being something for another time as it involves a fairly significant bulk of changes, if I understand it correctly).

I'd love to better understand what large changes this might entail/why it looks like it might require a lot of work - would be great to address that problem (either whatever info lead to the misunderstanding - or an actual high cost for such work) - hopefully teh example I gave above demonstrates how this can be tested without too much work.

>   A regression in things like metadata naming on the other hand (like renaming `Dwarf Version` to something else) would be ABI breaking in terms of IR as well so if it does get renamed, a ton of other tests would fail as well.
>    
> 
> This is just a quality-of-life improvement for dealing with migrating to DWARF5 as well as general migration of bitcode based builds forward over to LLVM 8 and finding stale bitcode archives easier for people who use modular builds involving linking against a variety of cached pre-built bitcode archives, which saves time when trying to work out which caches need purging. (The original patch was a rougher version of this that I added to my local fork to try to hunt down sources of these warnings, this is just a slightly more refined version of something I've been using for a while now in an upstream-friendly form)

Also super curious about how C++/Clang modules buidls relate to this - they don't cache bitcode, do they? Sounds like you're dealing with a ThinLTO/incremental LTO build? That's separate from Clang Modules. (Clang modules with LLDB and -gmodules can cache debug info in the .pcm files - but that's stored as actual object code DWARF, not LLVM bitcode/IR debug info).

(also, maybe we can avoid a special case for DWARF here? And have the diagnostic print both values of whatever the metadata field is - a more general solution?)

Thanks for the fix/improvements!


Repository:
  rL LLVM

https://reviews.llvm.org/D52952