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

Mon Oct 8 15:51:43 PDT 2018

kristina added a reviewer: dblaikie.
kristina added a comment.

> In https://reviews.llvm.org/D52952#1258114, @dblaikie wrote:
>  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)

Those you mentioned have separate compiler teams, some multiple of them, that occasionally contribute to upstream. The changes that land in downstream forks are usually cherry-picked from upstream, speaking from experience, so that has very little relevance when it comes to upstream modular builds breaking. Since it's not a big change I do not see why (after all I did it too) downstream forks would be in a hurry to get those changes upstreamed (The pre-cleanup version of this sat in my repository for weeks).

`Dwarf Version` is metadata stored in bitcode files, the conflicts like this is what happens when a module (a bitcode archive) is linked against an incompatible module (which could very well be a self-contained single bitcode file for a single TU since if I understand correctly, since LLVM7-8 have been very much module-oriented internally, even when a module is a single TU, which is extremely common) with a different version for the `Dwarf Version` metadata property. I never said anything about actual DWARF data being embedded in bitcode archives, only the metadata property. No DWARF data is emitted at any of those points. This merely pins down a conflict in metadata. Nothing more.

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

Considering it currently has zero coverage, adding a new set of tests that, also likely need to cover out of scope (for this patch) existing diagnostics for various other metadata attributes (there is quite a few of them) would likely belong in a separate diff addressing the lack of coverage in this area. Because of that I do not think it's a good idea to fuse diffs that add clusters of new tests for things that previously had zero coverage and changes like these.

> (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?)

It's the most common conflict I've come across, it's already part of the ABI and has a defined format. I do not believe there is a unified way to concisely represent all metadata property values especially when it may not even be human-friendly when it comes to reading it. This also implicates assumptions about further formats of metadata, naming of it, and layout of it, all of which is subject to change, while this particular property is part of the currently stable ABI on IR level. Therefore `Dwarf Version` is handled differently because it's known how such an attribute is handled and what it contains.

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

As previously mentioned, adding tests like this would likely be done in bulk for various other metadata attributes if you wanted to have coverage for those diagnostics, because it's a lot more convenient and is completely separate from any changes in code, since reiterating, //none of this currently has any coverage//.

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

More specifically actually emitting bitcode archives for libraries, ready for being pulled in for (Thin)LTO and static linking. The actual libraries can be considered modules in their own right.

I understand the desire for increasing test coverage and you may consider this a bad attitude but I fail to see how regression tests would make that much of a difference (a very very tiny increase in coverage for a single metadata attribute) for this specific case, unless it was done for `IRMover` as a whole, in which case yes that would provide a significant increase in coverage. I don't believe this patch is the place to start this process, however. (I should also mention that there are cases where there's a desire for increased coverage just for the sake of increased coverage, with tests that do not actually really test for any regressions).

However if you're unhappy with lack of bundling/starting of adding coverage tests for these particular diagnostics (ones coming from `IRMover`), I'm okay with abandoning this diff and keeping it downstream. It helped me so I figured it would help other people, in-scope, there is very little to test, it's simply adding additional information to existing diagnostics.

Repository:
  rL LLVM

https://reviews.llvm.org/D52952