Thank everyone for reviewing this!
Committed as 68f464ac2ef5de8cb2e8beaeee43c435c536539e.

@djtodoro , yeah, it doesn't look good.
I also considered an option to rename the first bucket to smth like "No location info" or "Not covered" or something similar then the second can be safely named '0-9%'.
Or simply to use '0-9%' together with '0%' which might be a bit unclear, but as docs say 'The line 0% shows the number and the percentage of DIEs with no location information', it should not be confusing.

In D70548#1757299, @dblaikie wrote:

In D70548#1756279, @djtodoro wrote:

@krisb Thanks for working on this!

Let me summarize this quickly.

This will remove mixing between calculation against enclosing and adjusted scope. We have concluded the adjustment heuristics are confusing and wrong in many cases, so we are giving up on that. The alternative is to generate the DW_AT_start_scope attribute, that represents exactly the adjusted scope, and having that we will be able to calculate the debug location statistics for the pieces of code where the variable is alive.

WDYT how "easy" will be to implement the DW_AT_start_scope attribute? I know that it might not be used anywhere else, but I think that having location statistics against variable's life will be very useful thing.

See my reply from D69027 - though thinking more closely, I'm not sure start_scope would really answer the question as it seems like it has many of the same issues as the adjusted scope we're removing - it assumes the code is laid out in order. Any use of DW_AT_ranges would invalidate it (& I /think/ many & perhaps most scopes are fragmented into ranges in optimized code which is where any of these stats are interesting) & basic block reordering, etc, is sort of the nail in the coffin.

In D70548#1756279, @djtodoro wrote:

@krisb Thanks for working on this!

Let me summarize this quickly.

This will remove mixing between calculation against enclosing and adjusted scope. We have concluded the adjustment heuristics are confusing and wrong in many cases, so we are giving up on that. The alternative is to generate the DW_AT_start_scope attribute, that represents exactly the adjusted scope, and having that we will be able to calculate the debug location statistics for the pieces of code where the variable is alive.

Refactored llvm/utils/llvm-locstats/llvm-locstats.py variable's names to avoid mentioning 'first def'.

Abandon the revision as I submitted another one https://reviews.llvm.org/D70548 that removes OffsetToFirstDefinition instead of trying to fix it, as suggested by @avl.

When I started to work on the statistics, I also thought that the percentage buckets are to show completeness of the debug info. Unfortunately, due to BB reordering this information, sometimes not just imprecise, but rather terribly wrong. And in general case, we can't even guarantee any precision. I agree with @dblaikie; we need a way to calculate adjusted scope correctly regardless of basic block reordering to justify the notion of the adjusted scope itself. Otherwise, I'd prefer to reduce maintenance costs and to calculate debug info coverage against the full enclosing scope. In such a case, only changes in percentages would make sense, but at least this information won't be misleading. So, I tend to abandon this patch and proceed with another one that changes all the calculations against an adjusted scope to calculations against a full enclosing scope (if there are no strong objections).

Currently (in the current master), we have the following statistics related to the discussion:

1. The total number of bytes covered by DW_AT_location over all variables. We also have separate statistics for local vars only, params only and entry values for vars and params.
2. The total number of bytes in variable's scopes over all variables. Variable's scope means a scope adjusted to the lowerest address in variables' DW_AT_location. Plus, separate statistics for local vars only and params only.
3. The number of variables in each coverage bucket, where the coverage bucket calculated as 'the number of bytes covered by a variable's DW_AT_location' / 'the number of bytes in variable's enclosing scope'. Variable's enclosing scope means a full scope (e.g. full subprogram, lexical block, etc.) without any adjustments. There are also separate statistics for local vars and params, and all the three where entry values are excluded.

In D69027#1729302, @dblaikie wrote:

In D69027#1728152, @aprantl wrote:

I see the primary value of this statistic to allow us to see change — then a human has to make judgement what that change means. Regardless which choice we make, we may get a false signal from a compiler change that reorders basic blocks, but we'll get valuable signal when we're testing the ramifications of a debug-info-only change to the compiler. As long as we document what the statistics measure, I think we just need to pick something and run with it.

Sure - then I'd suggest that a good statistic to choose would be "bytes covered by location lists" (which is already there) and maybe, if you want a % one, "bytes covered by location lists" / "bytes of enclosing scopes" (counting each scope once for each variable within it - or doing the loc-list-intersects-with-enclosing-scope earlier & just tracking the relative numbers from that point).

@dblaikie thanks, David! This is definitely needed to be kept in mind.

In D69027#1723448, @avl wrote:

If the other metric is valuable, I suggest implementing it in a separate patch. But could you give an example when calculating coverage against full scope provides additional knowledge which does not follow from currently existing metrics?

suppose we have such case with incomplete debug info:

DW_TAG_lexical_block

DW_AT_ranges  (0x00000000
   [0x000000000000010, 0x0000000000000197))

DW_TAG_variable
  DW_AT_location  (0x0000018e
     [0x0000000000000183,  0x0000000000000197): DW_OP_reg0 RAX)

And then will create a fix which improves debug info in such a way:

DW_TAG_lexical_block

DW_AT_ranges  (0x00000000
   [0x000000000000010, 0x0000000000000197))

DW_TAG_variable
  DW_AT_location  (0x0000018e
     [0x0000000000000100,  0x0000000000000183): DW_OP_reg1 RBX)
     [0x0000000000000183,  0x0000000000000197): DW_OP_reg0 RAX)

In both cases reported coverage there would be 100%.
That hides original low coverage for that variable and makes invisible performed improvement.
Change in coverage looks like this: 100%->100%. If it instead would be 20%->30% then result of changes would be seen.

Doesn't the absolute number of covered bytes (i.e. ScopeBytesCovered) suite this purpose? Yes, we will see no change in coverage, but the absolute number will be changed from 14 to 97 bytes, which may be considered as an improvement. Does it make sense to you?

In D69027#1718400, @avl wrote:

Speaking of variable's enclosing scope: it is necessary to take into account not only OffsetFromFirstDefinition, which points into the start of the variable's enclosing scope. The end of the variable's address ranges should also be encountered.

Bumped version 4, updated more tests, added comment for adjustScopeToFirstDef().

@avl, thanks for the comment.

In D69027#1711682, @dblaikie wrote:

I'm confused - given the presence of noncontiguous ranges, why would the "first def" be an important property?

The existence of this operation is "iffy" to me - I'd expect a general solution that handles all the noncontiguous range chunks equally, so there wouldn't be a special case for the "first def".

@dnsampaio, many thanks for committing this!

Rebased

Applied the comment: enable 'sha2' and 'aes' only for armv8 A profile, report warning and disable 'crypto' for other archs. Add more tests.

I see. But this doesn't seem like a valid case, does it? Shouldn't we somehow diagnose it to not to silently ignore user-specified options?

@dnsampaio thanks for reviewing this! Could I ask you to commit the patch (since I don't have commit access yet)?

Added 'CPU.empty()' check back.

@dnsampaio, thanks! This is definitely better. I also added ARMV8_5A as a test case.

Applied the comment & rebased.
Also changed the patch to take into account only the latest 'crypto' in the Features vector, to avoid enabling 'sha2' and 'aes' when 'crypto' was finally disabled.

@dnsampaio, thanks, this looks better.

Applied the comment.

Fixed broken test.

@asl thanks for catching this! Looks like I forgot to amend a fix for the test after testing :( sorry for that.

@aaron.ballman yes and yes. Thanks!

@aaron.ballman Thanks! Could I ask you to commit the patch? I don't have commit access yet.

Applied the comment.

Addressed feedback.

Applied comments and extended condition with 'ShiftAmount == 8'

Applied the comments

Reverted unintentional changes again. Sorry for that

@teemperor, thanks! All comments are applied.

In D43186#1020762, @arsenm wrote:

This is too aggressive. I have D20758 to do this for noon. Otherwise it should fail for the target to do something about it

In D43186#1020602, @yaxunl wrote:

I think the lowering of addrspacecast is target dependent.

Reverted unintentional changes.

Updated one more test where attributes became mismatched.

Ping