This is small enough that I'd prefer to see it incorporated into the patch with the "real" warning, rather than have this temporary placeholder diagnostic.

Regarding the bit about environment variables, probably the right thing to do is add a lit.local.cfg to the FileCheck test directory, that zaps the environment variables. Then most individual FileCheck tests can assume a default environment, and tests for specific non-defaults can set the environment variables directly in the test file.
It would mean you can't run the FileCheck tests *from lit* with an environment variable set. Unless we invented another env var that lit.local.cfg would look for... but that's for another patch, at this point.

In D54597#1301275, @aprantl wrote:

I think this looks good now.

It really seems like DiagList and AnnotationList ought to be vectors, not lists. They are append-only, and AnnotationList gets converted to a vector anyway to sort it. The code doesn't depend on the element-pointer stability guarantee of a list, except in one place noted below which can be fixed.
It's quite possible a vector would perform less well, in the face of many diags/annotations, but as the diags/annotations are the unusual case, performance is not really a big consideration.

Delete unneeded ctor. Clang-format.

Remove new .def file, move content into DebugInfoFlags.def.
Fix one oversight in Clang part, lit tests now all pass.

In D54597#1300354, @aprantl wrote:

Just to make sure I'm reading this correctly: This patch does not change the bitcode format in any way?

My experiments with clang-format show that it is aware of this convention. It will change /*Prefix*/nullptr to /*Prefix*/ nullptr (inserting a space) and will also change /*Prefix=*/ nullptr to /*Prefix=*/nullptr (removing the space, if the comment ends with =).

Okay, clearly I have not been paying attention. It's not that lit -v enables the warning, it is simply not suppressing the message. This is why lit without -v appears to turn it "off." I get it now. As far as FileCheck itself is concerned, this is always-on.
Have you tried running the entire suite with lit -v to see how often the diagnostic comes up? Now that you don't emit a diagnostic for the multiple-prefix case.

I don't really see a need for a new directive, it's convenient but does nothing you couldn't do with existing directives.
It does have some downside, in that it's supported only by the LLVM integrated assembler.

Make DwarfDebug responsible for picking which section to use.

I hadn't thought about -v; I don't think the new diag is currently under -v? The first test isn't using that option...

Because there is nothing to indicate that a line contains a FileCheck directive, other than that it pattern-matches a user-specified string with an optional suffix, we can't reliably report all typos. So for example:

CHECK-LABEL: A
CHECK: B
CHEKC: C

the third line can't be diagnosed by FileCheck under any reasonable heuristic. So, anything we do is a best-effort, and ideally produces no false positives by default.

In D54217#1291828, @clayborg wrote:

Looks like I can piggy back on llvm/test/tools/llvm-dwarfdump/X86/statistics.ll

Does anyone know how to run only this test from command line?

In D53710#1290345, @jdenny wrote:

In D53710#1289937, @SjoerdMeijer wrote:

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Yep. Briefly looked at AMDGPU warnings. The very first 4 warnings, in tests basic-loop.ll, basic-call-return.ll, bug-vopc-commute.ll, call-return-types.ll, shows I think that they are dodgy tests, but then I see indeed the intentional use of the shared label prefix.

You two seem to be saying that the concept of a "shared label prefix" justifies this use case. I'm sorry, but I don't know what that phrase means. Could someone please explain? In other words, couldn't these tests just use "CHECK:" (or "GCN:" or whatever prefix they prefer) without "-LABEL"? I believe that's Paul's position.

In D54187#1290340, @gbedwell wrote:

(Dexter) will step through every line of the program it can, collecting info about each step until it reaches the program exit. It won't currently produce a pass/fail, but rather a score. That is, if it observes that 'x' is visible and has the value 42 it'll get a perfect score.

Seems like it would easy enough to base pass/fail on getting a perfect score, for the purpose of debuginfo-tests.

In D54187#1290294, @zturner wrote:

In D54187#1290282, @probinson wrote:

+gbedwell

Just to throw the idea out there, why not abandon debuginfo-tests entirely and try using Dexter for this. Dexter's design center is debug-info quality, not correctness, but it already knows how to drive several different debuggers on multiple platforms.
Dexter would have to become an LLVM project tool, and I'm not sure how Sony management feels about that, but I think this would be an awesome use-case.

Depends where we draw the distinction between quality and correctness. We specifically want a way to test that when we fix a correctness bug, it's actually fixed against Microsoft debuggers.

Just to throw the idea out there, why not abandon debuginfo-tests entirely and try using Dexter for this. Dexter's design center is debug-info quality, not correctness, but it already knows how to drive several different debuggers on multiple platforms.
Dexter would have to become an LLVM project tool, and I'm not sure how Sony management feels about that, but I think this would be an awesome use-case.

I think what we've determined so far is that some uses of CHECK-LABEL simply misunderstand the LABEL suffix, which IMO is because the suffix is very poorly named. Renaming it to something harder to misunderstand would be a lot of churn but ultimately benefit the project's tests. The process of updating all tests with LABEL would necessarily find all the dodgy cases. And with a better-understood FileCheck feature, ultimately I think there would be fewer cases of bad tests.
(It would also help if people responsibly made sure their tests failed when their patch was NOT applied, but we have no process for this.)

LGTM.
A patch to report an error on multiple -D of the same variable is probably a good thing, particularly since it doesn't work properly.

We already do this for X86, would it be better to make the check in non-target-specific code? (Just asking, not expressing an opinion.)

Even when DWARF adds something in response to the needs of a specific language, it doesn't use language names in the tags or attributes. So, I think putting "Fortran" in the symbol names here would be inconsistent with that. It's fine to describe it as a Fortran attribute in commentary.
As a hypothetical example, C++ has the concept of a "pure" virtual method declaration, and somebody might come up with a reason to mark those with the new Pure flag.
Which is another reason not to start overloading the existing flags based on assumptions about which languages use which flags. I've started sketching out what it would take to add a separate DISPFlags type/field in DISubprogram.

Ping. For my next trick I was going to do the patch that emits type units in .debug_info instead of .debug_types, for DWARF v5. Would it help to have that up first?

Procedural point: When you upload a diff to Phabricator, please include more context (-U999999 is typical).

In D46061#1285431, @dblaikie wrote:

Looks like Eric still had some questions here? Or at least hadn't commented
about whether the answers/changes addressed his concerns?

Please review all comments to ensure proper punctuation. There are a lot of missing full-stops.

Presumably this addresses the bad debugging experience for the sample from the PR; does it have any other good/bad effect on the Dexter corpus?

LGTM

In D52296#1283691, @grimar wrote:

Nice :)
So seems the last unresolved question left is the naming of the new option?
If we do not want to go with -gsingle-file-split-dwarf then what it should be?

What about -fdebug-fission as an alias for -gsplit-dwarf.
And -fsingle-file-debug-fission for the new option?

Or, may be:

-fdebug-fission for the new option and then
-fdebug-fission -fdebug-split-dwo would work together as -gsplit-dwarf?

In D52296#1282589, @dblaikie wrote:

In D52296#1282587, @probinson wrote:

Any distributed build has to make this work, so the paths in the line table are usable. Not clear what you're thinking might be the problem with the split-in-.o case.

Fair point - same sort of problem, but might need distinct handling (ie: might not work "for free" with existing tools, but need more support), etc, depending on how it's solved?

Mostly just wondering whether Clang would need extra flags to support this - to get a sense of the total/overall solution/surface area of the feature, etc.

In D52296#1282458, @dblaikie wrote:

I guess in that case your distributed build system would have a constraint that it always ships all the object files back to the primary machine (where you'd be running the debugger)? (perhaps it just always runs the link locally - even though it distributes the compilations - I guess that's probably how things like distcc work, where they only inject themselves into the compilation command, not the linking command maybe?)

I reordered the units in the test file, because llvm-dwarfdump will dump all the .debug_info bits before going on to the .debug_types bits. The order of the sections in the file doesn't matter, but the order of the CHECKs does, and this was the simplest way to deal with it.

Does this lose debug information? I do very little with optimization passes so I don't know, but it would be nice to keep it in mind.

In D53710#1277353, @jdenny wrote:

In D53710#1277094, @SjoerdMeijer wrote:

Thanks for looking at this! Answering this one first:

Have you run existing test suites to see how many warnings you get?

I just did, and it came back with some interesting results. For example test/CodeGen/ARM/Windows/mangling.ll:

; RUN: llc -mtriple=thumbv7-windows -mcpu=cortex-a9 -o - %s | FileCheck %s
define void @function() nounwind {
entry:
  ret void
}
; CHECK-LABEL: function

This shows that just checking the label is actually a use-case, which is why I thought a warning would be better; looks like we can't promote it to an error (which is okay I think).

Why isn't that just CHECK? Is that a legitimate use case?

In D47811#1273876, @vvlevchenko wrote:

In D47811#1273052, @probinson wrote:

I've put a note out to the DWARF committee to see if we can streamline the assignment of language codes. As long as there is a source of truth for these, we shouldn't have to wait for DWARF 6 to be published.
The idea has come up before, and now with a motivating case I think it's likely to happen. So if you could hold off a little longer (probably a few days) I can have an answer for you on that point.

Ok I'll wait as long as required, for now I've updated diff regarding my understanding of our yesterday conversation. Thank you for cooperation.

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

I've put a note out to the DWARF committee to see if we can streamline the assignment of language codes. As long as there is a source of truth for these, we shouldn't have to wait for DWARF 6 to be published.
The idea has come up before, and now with a motivating case I think it's likely to happen. So if you could hold off a little longer (probably a few days) I can have an answer for you on that point.

Following up from the BoF and the corresponding email thread:
I think we want to distinguish the general guideline/intent (support N years/releases) from the firm statement "we are actually going to require *this* minimum version starting on *this* date." There seems to be some resistance to having the latter happen on a strict timeline.
Having separated out those two things, we should specify a minimum lead time for the firm statement to go from a warning to an error. I threw out 3 months as a starting point for discussion; I don't think it can reasonably be any less than that. Upgrades like this need to be tested, deployed to lots of people and bots, issues found and corrected. These things take time.

+wolfgang who has been working in this area also

+echristo re defining an LLVM range of language codes.

It would be safer if we came up with an LLVM extension range for language constants (assuming the DWARF committee doesn't change the process for assigning language constants). It will be years before DWARF v6 comes out and there is a very reasonable chance that the numbers won't match.
I'd rather we don't ship any LLVM that could emit an incorrect language code.

In D52444#1253212, @dblaikie wrote:

(also, I did go back and read the spec - and you're right that call_ref uses an address-width-sized value, and that it could be used in debug_loclists.dwo.

In D52295#1251426, @Chirag wrote:

Yes, with this patch, only decimal scale is supported and i will add binary scale as well. (will modify this patch), and related changes too.
and about the front-end, we have a pl1/cobol llvm based front-end (in development) which generates this info.

What I meant by "use case" is, do you have a front end that will make use of these new features?

clang-format-diff is your friend.

Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win.

LGTM

In D51976#1237392, @vsk wrote:

I expect that extending dbg.value in this way will be fairly involved. There are a couple questions worth thinking about, imo:

• Would the extra SSA values need to be constants (or live in registers)? If not, how will their locations be represented in a TAG_variable entry?
• What's the expected memory size overhead of adding an extra operand to dbg.value? Does it need to be counterbalanced by introducing dbg.value.simple() [for the case where there is an empty DIExpression/extended SSA stack]?

In D51976#1236852, @CarlosAlbertoEnciso wrote:

In D51976#1235508, @aprantl wrote:

Having read the real testcase now, I understand why you are dropping the debug info now. I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

That is a very good idea and I am interested in implementing it. A good opportunity to improve my knowledge in that area. I can see the associated benefits.

In D51908#1233588, @dblaikie wrote:

Perhaps we could implement this behavior (pr: i.e., omitting the label if it has been optimized away) only if we're tuning for GDB & otherwise produce a label without a low_pc? Not sure how other folks (@aprantl @echristo @probinson @JDevlieghere) feel about this - apply the workaround generally, because it's not super important to have a label for name-lookup porpoises? or is it worth carrying the two codepaths to ensure that slightly more accurate debug info where it's possible (presuming LLDB can cope with it)?

Can you preserved the declaration DIE (at 0x0000007c) while omitting the definition DIE (at 0x00000063)? Does that still cause a problem for GDB?

+cfe-commits

Awesome thanks!

The indentation in the test looks a little odd. Please verify there are no hard tabs.
Otherwise LGTM.

I'm abandoning this minor tweak as I am convinced that there is something more fundamentally wrong here.
llvm-dwarfdump doesn't call the right API sequence to show the bug; it requires a DWP, looking up a specific unit by hash via the DWP index, and then scanning all the units. I can envision how to do a unittest for that, but the unittest infrastructure doesn't currently support building a DWP. I can't take the time right now to address this, but I'll keep it on The List.