In D57694#1441561, @jmorse wrote:

In D57694#1441530, @srhines wrote:

Switching it to a MapVector makes it deterministic. I was just struggling late last night to try to reduce the .ll file down to something worth checking in! If you want to take over to minimize the test case, that would be much appreciated. Let me know so I can continue doing that if needed. Thanks.

Hmm, what kind of requirements are there for tests for nondeterminism? Just a high probability of triggering the existing fault/variation? This is interesting as unreliable compilation presumably means the test can't be reliable either.

A couple more style nits and LGTM.

In D59553#1438600, @echristo wrote:

In D59553#1437670, @probinson wrote:

@jhenderson and I did a prototype of DWARF-unit-per-function last year, and I was not favorably impressed by the numbers. But maybe he still has the actual data kicking around somewhere. We did not solve all the issues to our satisfaction before we ran out of time.
I've also had chats with @bd1976llvm about fragmenting .debug_info per-function without wrapping everything in units; you get into requiring some section-order things but it saves the unit-per-function overhead.

This is pretty much what I was talking about. You definitely don't need to do it as units if you're just including the concrete function. FWIW this is what I was talking about in the dwarf meeting where we talked about bringing units back into debug_info :)

Thanks for working on this! I knew I had left duplications behind, but the task to take care of them hadn't ever gotten to the top of the list.
Mostly style things to start with.

This is straying far enough from the original review that maybe a dev thread is a better place to discuss "how to strip per-function debug info along with the functions."

@jhenderson and I did a prototype of DWARF-unit-per-function last year, and I was not favorably impressed by the numbers. But maybe he still has the actual data kicking around somewhere. We did not solve all the issues to our satisfaction before we ran out of time.
I've also had chats with @bd1976llvm about fragmenting .debug_info per-function without wrapping everything in units; you get into requiring some section-order things but it saves the unit-per-function overhead.

Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.

I remember LLVM's DWARFContext being irritating to deal with when I was doing the DWARF 5 stuff. The exact details have been swapped out of my memory, but I suspect it had something to do with needing to know whether I was in DWO mode or normal mode in order to snag the right section for some sort of cross-section lookup. This is because DWARFContext is the dumping ground for all the sections, regardless of what kind of file we're looking at, and it's up to the user of its interfaces to know what mode it's in.
I will admit that having one dumping ground is convenient in some ways, especially when we were looking at object files that contained a mix of both normal and .dwo sections, but IIRC we don't emit mixed-mode object files anymore.

Filed PR41152 for the unobvious display of a zero-length location expression.

LGTM

Generating the test on the fly with Python seems plausible. Note you don't need 16K components, just enough components to make the location description exceed 16K bytes. If each component is (say) a 64-bit field with a max-int constant value, that's something like 11 bytes of description per field, so ~1500 fields ought to do it.

In D59417#1433403, @vsk wrote:

In D59417#1431496, @probinson wrote:

In D59417#1431369, @vsk wrote:

As for instructions at the start of the block - I'm pretty sure that a while back @probinson made a change to ensure that cascade locations couldn't occur at the start of a block (that we'd set to line zero down in the AsmPrinter/DwarfDebug if that occurred) so we shouldn't end up with super weird/layout-based cascade locations for instructions generated from phis at the start of blocks.

That makes sense. What if a block has multiple phis? I.e. if the first phi in a block has a location but the second phi doesn't, is that a bug, or do we expect the location to cascade?

Locations cascade, with one exception: If the first instruction after a label does not have an explicit source location, we emit line 0.

That sounds like a sensible rule. Presumably that line 0 location has the correct scope set. Shall I relax the debugify verifier so that it only diagnoses non-phi instructions without locations? I.e. are phis special in some way, or would the relaxed check still be too strict (& any 'no location' diagnostic could be noisy)?

In D59417#1431369, @vsk wrote:

As for instructions at the start of the block - I'm pretty sure that a while back @probinson made a change to ensure that cascade locations couldn't occur at the start of a block (that we'd set to line zero down in the AsmPrinter/DwarfDebug if that occurred) so we shouldn't end up with super weird/layout-based cascade locations for instructions generated from phis at the start of blocks.

That makes sense. What if a block has multiple phis? I.e. if the first phi in a block has a location but the second phi doesn't, is that a bug, or do we expect the location to cascade?

Aren't phis *always* at the start of a block?
If that's the case, I'm tempted to say phis should be allowed to have no source location, and we should fix the debugify verifier accordingly. Sometimes a phi does have an appropriate source location, which is fine. I supposed sometimes a phi ought to have a real location, but doesn't, and the verifier will stop helping us to find those cases; but it will save a bit of memory not to attach source locations to every phi in the world, and have no practical effect on the final line table (because those will turn into line-0 anyway).

I believe this is a patch to fix PR37964?

I'd preserve the trivial test cases and show that the NonTrivial flag is *not* present. I can suggest ways to achieve this if you like (note there is no CHECK-DAG-NOT combined directive).

In D59235#1425443, @zturner wrote:

In D59235#1425436, @clayborg wrote:

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

That's why for now I'm just doing very small preliminary steps to get the two interfaces to be closer to each other and simplify the problem space. We can worry about the asserts and all of that when we actually start moving pieces of LLDB to use LLVM's classes (which isn't in this patch).

I am convinced that the asserts are wrong, and that we currently get diags for NEXT/SAME/EMPTY at the top. So LGTM to remove the asserts.

In D58784#1424305, @thopre wrote:

I think I can be convinced that it is ok to make an initial CHECK-NEXT match the first line of input if an explicit mention to this case is added to the existing documentation for CHECK-NEXT and perhaps also reword the current text slightly to indicate that CHECK-NEXT checks for a match in the next line of input (so that the explicit mention of CHECK-NEXT as first directive feels natural to whoever is reading).

I'd be inclined to add the serialize/deserialize test to an existing test file, rather than a new test.
Similarly for the flag->attribute test.

OK sounds like we should invent a CHECK-FIRSTLINE directive. (Let's not call it CHECK-FIRST because people could think it means "do these first before other checks.") It must be the first directive, and its pattern must match on the first line of input (before the first newline).
Does that satisfy the requirements?

I haven't verified it renders correctly, but eyeballing it, looks right.

In D58952#1420777, @dblaikie wrote:

In D58952#1420776, @probinson wrote:

I'm not sure I follow this - the line table maps instructions to source locations. If there are no instructions there's nothing to map

That's the intent. However, in practice it maps instruction *addresses* to source locations.

Well, in practice it's unspecified what it means - so we can look at this and come up with different interpretations. Mine is to interpret these as half open intervals - in which case [100, 100) is empty and you keep searching, then you find [100, 104) and that contains the address you're looking for.

LGTM. It would be nice if we picked a width to print based on the width we read in, but this is fine.

I'm not sure I follow this - the line table maps instructions to source locations. If there are no instructions there's nothing to map

I think there is value in having a way to check the very first line of input. Once we've done that, everything else ought to fall out naturally, if the model is defined well enough.

A few wording notes. Haven't worked through the examples yet.

In D58784#1414574, @jdenny wrote:

Here's how I see it, but I realize it's subjective. If we say that an initial CHECK-SAME matches the first line, then, to be consistent with the relationship CHECK-SAME and CHECK-NEXT have everywhere else, an initial CHECK-NEXT should skip a new line and thus match the second line. That means that, in order to match consecutive lines starting with the first line, we have:

CHECK-SAME: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

In D58784#1414321, @jdenny wrote:

In D58784#1414106, @probinson wrote:

FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

This directive looks for the next empty line, even if that's not the first line. Interestingly, when it's not the first directive, its search range starts after the previous match, so if the remainder of the previous match's line is empty (even if the full line is not empty), it'll match there.

Address review comments & rebase.

So we didn't have any tests that verified the assertion? That was an oversight.
FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

Drive-by nit.

Some initial comments.

In D58453#1409691, @jmorse wrote:

Absolutely true -- and constant-valued dbg.values (including undef, technically) are frequently the source of bug reports (ex: PR40795) as their lifetime isn't associated with anything else in the IR (see also PR40628).

Hmmm. It appears that you are detaching the notion of here-the-value-is-computed from here-the-value-becomes-associated-with-this-variable. Usually these are concurrent in an "assignment" to the variable.
Separating these concerns means that if a value computation is hoisted, it might not be appropriate to hoist the association with the variable. That is, the "assignment" effect maybe should stay where it was, even if there's no specific instruction left in that position that implements that assignment.
I could see this situation arising in a variety of circumstances. CSE, various sorts of value-propagation (constant or not), passing a value to an inlined function parameter, yada yada.

In D57896#1406412, @MyDeveloperDay wrote:

In D57896#1406407, @zturner wrote:

If I read the post correctly, it was actually agreeing with me (because it said "to reinforce your point...". Meaning that something such as lowerCaseCamel would be the third style being referred to

Correct! just acknowledging your point from a different perspective.

In D57896#1406353, @MyDeveloperDay wrote:

I can't argue with anything you say.. but I guess to reinforce your point introducing what is effectively a 3rd style would likely cause even more jarring...

There's a lot going on in the explanation, and partly I'm handicapped by lack of familiarity with the picky details of LLVM's location-list construction. Given all the attention to where dbg.value instructions are placed, it seems like placement is a significant factor in that construction. Intuitively, placing the dbg.value near the instruction that defines the value (assuming there is such an instruction) allows the location-list to better describe the instruction stream that is actually emitted. And, that's really what I want from a location list.
I would caution against focusing too much on the coverage statistics. As you say, what we really want to know is "whether the variable locations are defined over instructions in the program that make sense" and the statistics don't tell us that.
So, what instructions "make sense" in a location list? Well, the ones where the variable has an identifiable machine location or constant value. If optimization causes a value to be defined "before" the variable is declared, that does not really worry me. The goal here is to produce a correct description of the instructions-actually-emitted.
"One example problem is that conditional variable assignments can be hoisted and appear to debuggers unconditionally." Are the values actually defined unconditionally? Then it is correct to describe them that way to the debugger. I assume this is referring to PR38754. If the instructions to compute the value are hoisted, my assertion is that the location-list should describe the variable with that value when it actually occurs.
In my view, the real problem as stated in the PR appears to be that the value later gets *lost*, sadly before the instructions directly related to the switch-case where the variable is used; the real problem is *not* that the value's description starts too soon.

Please add a test.

+ @ABataev re the question whether NVPTX runs into the situation described in this review.

In D56587#1393556, @dblaikie wrote:

I don't think anyone mentioned it, that's why I was bringing it up - there's always an option to not render any location if it's not possible/worth the work. That's all I was asking - is it worth the complexity? (I wasn't sure anyone needed it - but sounds like Sony does, reckon it's worth the tradeoff in complexity in LLVM compared to the work required to support this in the Sony debugger?)

There has been some progress recently on better FileCheck diagnosis of likely test-writing issues, although to date it mostly requires the human to ask "what is going on here?" explicitly. I can see adding a check to detect the missing-colon-on-otherwise-valid-directive case (including the usual suffixes), and my guess is that isn't too likely to cause excessive churn. The deeper we go into typo-detection the more likely we are to trip over false positives, but this small step seems like a worthy change.
Happy to review such a patch, or somebody can file a PR and CC me.

In D56587#1391461, @dblaikie wrote:

In D56587#1391435, @aprantl wrote:

In D56587#1390564, @dstenb wrote:

How should DW_OP_convert be handled when targeting DWARF versions earlier than 5? There is the GNU extension DW_OP_GNU_convert, which GDB seems to have had support for since 2011. The operation seems to be the identical to the final version that got into DWARFv5, so LLDB should be able to handle the two variants transparently. Can we emit that GNU extension (under some limitations)?

That seems fine for debugger-tuning=gdb (and eventually lldb once we implemented support for it). Is emitting the byzantine shift/mask expression that this review started with in all other cases an option for a reasonably large subset of the uses?

If we have something nice for GDB and LLDB, and Paul's OK with one of those for Sony's stuff - then who would we be maintaining this extra code for? I'd be OK saying that backwards/sidewards/unspecified compatibility might not be worth it? If someone comes with a need, it could be resurrected/implemented - and until then, we could emit no location at all, potentially.

In D56587#1390564, @dstenb wrote:

How should DW_OP_convert be handled when targeting DWARF versions earlier than 5? There is the GNU extension DW_OP_GNU_convert, which GDB seems to have had support for since 2011. The operation seems to be the identical to the final version that got into DWARFv5, so LLDB should be able to handle the two variants transparently. Can we emit that GNU extension (under some limitations)?

In D57896#1389067, @lebedev.ri wrote:

2. It might be best to give this more visibility, by submitting a mail to llvm-dev, with a noticeable subject, like "RFC: changing variable naming rules in LLVM codebase"

r353328. (Typo'd the link in the commit message.)

Ping.

LGTM. I'll trust you on the actual address-class values.

The revision title and summary contradict each other. The title says to allow something, but the summary says that already works; then the summary says CHECK-DAG can't be first, while the check-dag.txt test clearly already has CHECK-DAG as the first directive.
Please fix up the title and summary so that they correctly describe what you are trying to accomplish. This will help reviewers to make sure the patch does what you want.

Thanks! I'm not surprised I didn't get it completely right; it was such a maze of twisty passages all different.

LGTM although I do wonder at the usefulness of the wildcards in the implicit-not checks.

LGTM

I'm happy.

Nice work on the prefix tests. One comment nit and LGTM.

A couple of comment phrasing nits.

Sorry for not noticing this sooner. The TL;DR is, the current patch does not affect PS4, so go ahead; but see the long version for other considerations.

In D54769#1317228, @jdenny wrote:

Another problem would be if lit substitutions make FileCheck commands or their prefixes unrecognizable. I haven't looked for that usage yet.

A few more comments.

Thanks for the ping. The code change looks straightforward, but a handful of questions on the tests.

In D56297#1346801, @andrewrk wrote:

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?