@djtodoro @aprantl thanks for reviewing this whole set of patches!

@aprantl thanks for the review! Are you okay with this test?

Refactor checks for constant members a bit.

Added a test.

@djtodoro thanks! Are you okay with this test case?

Add a test.

@djtodoro those two binaries with 99% of availability just have some variables w/o locations (mostly artificial 'this' parameters of member functions).
But this change reveals one more interesting case.
For code like this:

int foo() {
	if (cond1) {
		static const int a = 1;
		...
	}
	if (cond2) {
		static const int a = 1;
		...
	}
}

clang (unlike gcc) generates both a variables in the function scope, not in a corresponded DW_TAG_lexical_block (as gcc does).
I'm not sure clang is right here, cause a - despite it is static - isn't visible outside the if.
What do you think about this?

Use declaration info only to identify functions, global variables, and members.

In D72797#1829031, @djtodoro wrote:

So, this increases number of 'total vars' because these 'a' and 'b' variables counted twice.

I see... Can we do something to avoid the additional calculation? Somehow to recognize we already observed the a and b, although they have different DIE ID.

I also was a bit optimistic about how this patch(es) affects the performance of the statistics, it actually became two times slower than before the patches.

What patch from the stack adds the overhead? This one?

Yeah, this one mostly causes the overhead.
Actually, adding declaration info for ID is only necessary for functions, global variables, and members (because we need to handle static/inline functions/variables, and don't respect namespaces, classes, and structures). For local variables and parameters, Prefix + Name should be enough to distinguish all the cases (because their prefix already guaranteed uniqueness, for parameters the prefix is going to be added in D73003).
This approach shows better results:

@aprantl @djtodoro thanks for the comments!

Applied the comments.

Yeah, the rest of the code around SelectionDAG doesn't do a good job of keeping location intrinsics in order; I think it's acceptable to suffer a little bit more re-ordering here for an increase in available locations, for now.

Okay, I'll commit the patch as is, then. This issue with 'IROrder' in SelectionDAG is quite tricky, so I'll submit further changes in separate patches.

Rebased and reduced the test a bit more.

@spatel thanks for reporting this!
Could you, please, check that llvm-locstat wasn't cached in the build/install directory? llvm-locstat should look for a variables with (0%,10%) of its scope covered as was changed by the patch, so I guess it didn't get updated for some reason.

Updated the doc, rebased on D71366.

Fixed one more misprint.

Fixed a misprint in llvm/docs/CommandGuide/llvm-locstats.rst.

@djtodoro , Ah, missed this one. Thank you so much!

@djtodoro , @dblaikie thanks for your opinions! I created a separate review for this change https://reviews.llvm.org/D71366. Please, take a look if you have a chance.

Fix indentation in llvm-locstats output.

@aprantl, @djtodoro thanks! I reduced the test a bit more.

Reduced the test.

@dblaikie , @djtodoro sorry for paying so much attention to details here, but does this one work with you?

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.