[llvm-dwarfdump] Fix abstract origin vars location stats calculation

[djtodoro]

Summary

There are cases where a concrete DIE with DW_TAG_subprogram can have abstract_origin attribute, so we handle that situation as well.

[djtodoro]

@dblaikie I guess you have suggested renaming the IsInlinedFunction.

@aprantl We might need to update the Version as well?

[dblaikie]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–465

"IsPotentialAbstractOrigin"?

464–471

This name and the associated names (InlineFnsToBeProcessed, etc) may be incorrect now?

(or it looks like, if we're tracking "InlinedVars" Separately, perhaps this variable is the wrong one for tracking those - yeah, judging by the statistics that have changed - we probably shouldn't be grouping these non-inline concrete definitions under the "#inlined functions" or "#inlined functions with abstract origins" (also not sure why we have those two different statistics - inlined functions without abstract origins are non-DWARF-conforming, I think)

[djtodoro]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–465

Does this stand for the lines 465,466? (I guess it does)

464–471

Hmmm.... I am a bit confused. :)

If we have a concrete DW_TAG_subprogram with an abstract_origin (representing e.g. an always_inline function), are we counting it as an inlined function or as a concrete one?

If it is a concrete function, we should rename InlineFnsToBeProcessed, etc.

[dblaikie]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

I believe we should be counting it as a concrete one - are we not?

Looks like we are counting them as concrete.

Given this example:

__attribute__((nodebug)) void f1();
void f2() { f1(); }
void f3() { f2(); }

(compiled with -O3 to force the inlining, or use attributes to do the same)
Produces DWARF something like this:

0x0000000b: DW_TAG_compile_unit
              DW_AT_name        ("test.cpp")
0x0000002a:   DW_TAG_subprogram
                DW_AT_abstract_origin   (0x0000003d "_Z2f2v")
0x0000003d:   DW_TAG_subprogram
                DW_AT_name      ("f2")
0x00000049:   DW_TAG_subprogram
                DW_AT_name      ("f3")
0x00000062:     DW_TAG_inlined_subroutine
                  DW_AT_abstract_origin (0x0000003d "_Z2f2v")

The relevant stats are:

"#functions": 2,
"#functions with location": 2,
"#inlined functions": 1,
"#inlined functions with abstract origins": 1,

(I don't know why we have a separate stat for #inlined functions compared to #inlined functions with abstract origins - pretty sure the latter should just be a verifier error (& not one I'd expect to be common either) rather than a statistic)

So the two functions should me f2 and f3 (0x0000002a and 0x00000049, but not 0x0000003d).
The inlined function is the inlined version of f2 (0x00000062)

[djtodoro]

• Adding the test
• Addressing comments

[djtodoro]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

I believe we should be counting it as a concrete one - are we not?

Actually yes.

Looks like we are counting them as concrete.

Given this example:

__attribute__((nodebug)) void f1();
void f2() { f1(); }
void f3() { f2(); }

(compiled with -O3 to force the inlining, or use attributes to do the same)
Produces DWARF something like this:

0x0000000b: DW_TAG_compile_unit
              DW_AT_name        ("test.cpp")
0x0000002a:   DW_TAG_subprogram
                DW_AT_abstract_origin   (0x0000003d "_Z2f2v")
0x0000003d:   DW_TAG_subprogram
                DW_AT_name      ("f2")
0x00000049:   DW_TAG_subprogram
                DW_AT_name      ("f3")
0x00000062:     DW_TAG_inlined_subroutine
                  DW_AT_abstract_origin (0x0000003d "_Z2f2v")

The relevant stats are:

"#functions": 2,
"#functions with location": 2,
"#inlined functions": 1,
"#inlined functions with abstract origins": 1,

(I don't know why we have a separate stat for #inlined functions compared to #inlined functions with abstract origins - pretty sure the latter should just be a verifier error (& not one I'd expect to be common either) rather than a statistic)

Hmmm, I agree, there is no point of having inlined_subroutine with no abstract_origin, right?

So the two functions should me f2 and f3 (0x0000002a and 0x00000049, but not 0x0000003d).
The inlined function is the inlined version of f2 (0x00000062)

[dblaikie]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

I believe we should be counting it as a concrete one - are we not?

Actually yes.

Sorry, bit confused by this answer. "Yes we are counting out of line definitions of inline functions as concrete" or "Yes we are not counting out of line definitions of inline functions as concrete"...

Hmmm, I agree, there is no point of having inlined_subroutine with no abstract_origin, right?

I mean, DWARF supports it syntactically, but I don't think it's supported semantically - because the DW_TAG_inlined_subroutine is in the location of the inlining, not in the location the function is defined. So it needs to use an abstract_origin (& the spec says it does /have/ to do this, I Think) to point to a DW_TAG_subprogram that's in the scope where the function was written by the programmer.

Might be worth roping in whoevre implemented these statistics to check what the thinking was behind them?

[djtodoro]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

I believe we should be counting it as a concrete one - are we not?

Actually yes.

Sorry, bit confused by this answer. "Yes we are counting out of line definitions of inline functions as concrete" or "Yes we are not counting out of line definitions of inline functions as concrete"...

Sorry for the confusion. Yes, we are counting these as concrete functions.

Hmmm, I agree, there is no point of having inlined_subroutine with no abstract_origin, right?

I mean, DWARF supports it syntactically, but I don't think it's supported semantically - because the DW_TAG_inlined_subroutine is in the location of the inlining, not in the location the function is defined. So it needs to use an abstract_origin (& the spec says it does /have/ to do this, I Think) to point to a DW_TAG_subprogram that's in the scope where the function was written by the programmer.

My understanding of this goes in the same direction.

Might be worth roping in whoevre implemented these statistics to check what the thinking was behind them?

The patch that has introduced this is D58849.

[djtodoro]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

I believe we should be counting it as a concrete one - are we not?

Actually yes.

Sorry, bit confused by this answer. "Yes we are counting out of line definitions of inline functions as concrete" or "Yes we are not counting out of line definitions of inline functions as concrete"...

Sorry for the confusion. Yes, we are counting these as concrete functions.

Hmmm, I agree, there is no point of having inlined_subroutine with no abstract_origin, right?

I mean, DWARF supports it syntactically, but I don't think it's supported semantically - because the DW_TAG_inlined_subroutine is in the location of the inlining, not in the location the function is defined. So it needs to use an abstract_origin (& the spec says it does /have/ to do this, I Think) to point to a DW_TAG_subprogram that's in the scope where the function was written by the programmer.

My understanding of this goes in the same direction.

Might be worth roping in whoevre implemented these statistics to check what the thinking was behind them?

The patch that has introduced this is D58849.

I think we can check it -- should we do that via llvm-dev mailing list?

[djtodoro]

• Bump the stats version

[dblaikie]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

I believe we should be counting it as a concrete one - are we not?

Actually yes.

Sorry, bit confused by this answer. "Yes we are counting out of line definitions of inline functions as concrete" or "Yes we are not counting out of line definitions of inline functions as concrete"...

Sorry for the confusion. Yes, we are counting these as concrete functions.

OK, so to unwind the conversational stack, we are correctly counting non-inline subprograms that have abstract origins as concrete (not inline) and we should name the variable the same - Ah, which I see you've done.

Maybe the right name would be "XxxxWithAbstractOrigin" as "AbstractOriginXxx" sounds like they are the abstract origins (the thing abstract_origin points to) rather than the thing that refers to an abstract origin?

Hmmm, I agree, there is no point of having inlined_subroutine with no abstract_origin, right?

I mean, DWARF supports it syntactically, but I don't think it's supported semantically - because the DW_TAG_inlined_subroutine is in the location of the inlining, not in the location the function is defined. So it needs to use an abstract_origin (& the spec says it does /have/ to do this, I Think) to point to a DW_TAG_subprogram that's in the scope where the function was written by the programmer.

My understanding of this goes in the same direction.

Might be worth roping in whoevre implemented these statistics to check what the thinking was behind them?

The patch that has introduced this is D58849.

I think we can check it -- should we do that via llvm-dev mailing list?

Yep, sounds good to me - cc/include me and the patch authors/reviewers (@cmtice and @aprantl).

[djtodoro]

llvm/tools/llvm-dwarfdump/Statistics.cpp

464–471

Maybe the right name would be "XxxxWithAbstractOrigin" as "AbstractOriginXxx" sounds like they are the abstract origins (the thing abstract_origin points to) rather than the thing that refers to an abstract origin?

Oh yes...it makes sense to me! Thanks! I'll update the change in order to differentiate these two cases.

[djtodoro]

• make distinguishing between "XxxxWithAbstractOrigin" and "AbstractOriginXxx"

[dblaikie]

Sorry, I know this is annoying - but could you commit the renaming as a separate preliminary patch (feel free to commit that without review)? It'd make it much easier to understand the semantic change in this patch with that separated.

llvm/tools/llvm-dwarfdump/Statistics.cpp

230–231

Side note: why are we passing around std::strings by value? Should these be being passed by const ref?

[djtodoro]

In D101025#2727098, @dblaikie wrote:

Sorry, I know this is annoying - but could you commit the renaming as a separate preliminary patch (feel free to commit that without review)? It'd make it much easier to understand the semantic change in this patch with that separated.

(Sorry for delay, I was on PTO.)

It makes sense to me, I'll do that ASAP.

llvm/tools/llvm-dwarfdump/Statistics.cpp

230–231

I think we should be using const ref instead.
But it is existing code, and the change for that should be a separate commit, right?

[djtodoro]

• Rebase on top of the rG9ad9f0c73170

[dblaikie]

Looks good, thanks!

llvm/tools/llvm-dwarfdump/Statistics.cpp

230–231

oh, sure - easier to see this is untouched now that the renaming is separated, thanks!

461–462

maybe more accurately named HasAbstractOrigin? (IsPotentialAbstractOrigin sounds like this DIE /is/ the one referenced by an abstract_origin, rather than being the DIE with the abstract_origin)

[djtodoro]

@dblaikie Thanks!

llvm/tools/llvm-dwarfdump/Statistics.cpp

230–231

But I can still commit the patch for turning it into const ref, as a separate commit, right? (I guess no need to review that one)

461–462

Sure. (originally, it was HasAbstractOrigin xD)

[dblaikie]

llvm/tools/llvm-dwarfdump/Statistics.cpp

230–231

Yep!

llvm/test/tools/llvm-dwarfdump/X86/locstats-for-absctract-origin-vars.yaml

 ##     DW_AT_inline    (DW_INL_inlined)
 ##       DW_TAG_formal_parameter <--(0x000000f4)
 ##         DW_AT_decl_file       (0x01)
 ##         DW_AT_decl_line       (1)
 ##       DW_TAG_lexical_block
 ##         DW_TAG_variable <--(0x000000f8)
 ##           DW_AT_decl_file     (0x01)
 ##           DW_AT_decl_line     (1)
+##    DW_TAG_subprogram
+##        DW_AT_abstract_origin (0x000000f0)
 
-# CHECK: "version": 7,
-# CHECK: "#variables processed by location statistics": 15,
-# CHECK: "#variables with 0% of parent scope covered by DW_AT_location": 11,
+# CHECK: "version": 8,
+# CHECK: "#variables processed by location statistics": 17,
+# CHECK: "#variables with 0% of parent scope covered by DW_AT_location": 13,
 # CHECK: "#variables with 100% of parent scope covered by DW_AT_location": 4,
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
- Table:
           - Attribute: DW_AT_abstract_origin
             Form: DW_FORM_ref4
       - Code: 16
         Tag: DW_TAG_variable
         Children: DW_CHILDREN_no
         Attributes:
           - Attribute: DW_AT_abstract_origin
             Form: DW_FORM_ref4
+      - Code: 17
+        Tag: DW_TAG_subprogram
+        Children: DW_CHILDREN_no
+        Attributes:
+          - Attribute: DW_AT_abstract_origin
+            Form: DW_FORM_ref4
   debug_info:
     - Version:    4
       AbbrOffset: 0x00
       Entries:
         - AbbrCode: 1 ## DW_TAG_compile_unit
           Values:
             - Value: 0x00 ## DW_AT_producer
         - AbbrCode: 2 ## DW_TAG_subprogram
- Version:    4
             - Value: 1 ## DW_AT_decl_line
         - AbbrCode: 13 ## DW_TAG_lexical_block
         - AbbrCode: 5 ## DW_TAG_variable
           Values:
             - Value: 1 ## DW_AT_decl_file
             - Value: 1 ## DW_AT_decl_line
         - AbbrCode: 0 ## NULL
         - AbbrCode: 0 ## NULL
+        - AbbrCode: 17 ## DW_TAG_subprogram
+          Values:
+            - Value: 0xf0 ## DW_AT_abstract_origin
         - AbbrCode: 0 ## NULL

llvm/test/tools/llvm-dwarfdump/X86/statistics-dwo.test

 #   printf ("fibonacci(9) = %d\n", result);
 #   result = fib(10);
 #   printf ("fibonacci(10) = %d\n", result);
 # 
 #   return 0;
 # }
 #
 
-CHECK:      "version": 7,
+CHECK:      "version": 8,
 CHECK:      "#functions": 3,
 CHECK:      "#functions with location": 3,
 CHECK:      "#inlined functions": 7,
 CHECK:      "#inlined functions with abstract origins": 7,
 CHECK:      "#unique source variables": 9,
 CHECK:      "#source variables": 30,
 
 # Ideally the value below would be 33 but currently it's not.

llvm/test/tools/llvm-dwarfdump/X86/statistics-v3.test

 #   printf ("fibonacci(9) = %d\n", result);
 #   result = fib(10);
 #   printf ("fibonacci(10) = %d\n", result);
 # 
 #   return 0;
 # }
 #
 
-CHECK:      "version": 7,
+CHECK:      "version": 8,
 CHECK:      "#functions": 3,
 CHECK:      "#functions with location": 3,
 CHECK:      "#inlined functions": 8,
 CHECK:      "#inlined functions with abstract origins": 8,
 CHECK:      "#unique source variables": 9,
 CHECK:      "#source variables": 33,
 
 # Ideally the value below would be 33 but currently it's not.

llvm/test/tools/llvm-dwarfdump/X86/statistics.ll

 ; RUN: llc -O0 %s -o - -filetype=obj \
 ; RUN:   | llvm-dwarfdump -statistics - | FileCheck %s
-; CHECK: "version": 7,
+; CHECK: "version": 8,
 
 ; namespace test {
 ;  extern int a;
 ; }
 ; using test::a;
 ;
 ; int GlobalConst = 42;
 ; int Global;

llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.yaml

 ##
 ##       // #bytes in parent scope: 6
 ##       // #bytes in any scope covered by DW_AT_location: 2
 ##       // #bytes in parent scope covered by DW_AT_location: 0
 ##       DW_TAG_variable
 ##         DW_AT_location (0x00000023:
 ##            [0x0000000000000003, 0x0000000000000005): DW_OP_reg2 RCX)
 
-# CHECK: "version": 7,
+# CHECK: "version": 8,
 # CHECK: "sum_all_variables(#bytes in parent scope)": 12,
 # CHECK: "sum_all_variables(#bytes in any scope covered by DW_AT_location)": 8
 # CHECK: "sum_all_variables(#bytes in parent scope covered by DW_AT_location)": 4
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB

llvm/tools/llvm-dwarfdump/Statistics.cpp

 /// Collect debug info quality metrics for one DIE.
 static void collectStatsForDie(DWARFDie Die, std::string FnPrefix,
                                std::string VarPrefix, uint64_t BytesInScope,
                                uint32_t InlineDepth,
                                StringMap<PerFunctionStats> &FnStatMap,
                                GlobalStats &GlobalStats,
                                LocationStats &LocStats,
                                AbstractOriginVarsTy *AbstractOriginVariables) {
   const dwarf::Tag Tag = Die.getTag();
   // Skip CU node.
   if (Tag == dwarf::DW_TAG_compile_unit)
     return;
 
   bool HasLoc = false;
   bool HasSrcLoc = false;
   bool HasType = false;
   uint64_t TotalBytesCovered = 0;
   uint64_t ScopeBytesCovered = 0;
if (Die.isNULL())
     return;
 
   const dwarf::Tag Tag = Die.getTag();
   // Skip function types.
   if (Tag == dwarf::DW_TAG_subroutine_type)
     return;
 
   // Handle any kind of lexical scope.
+  const bool HasAbstractOrigin = Die.find(dwarf::DW_AT_abstract_origin) != None;
   const bool IsFunction = Tag == dwarf::DW_TAG_subprogram;
   const bool IsBlock = Tag == dwarf::DW_TAG_lexical_block;
   const bool IsInlinedFunction = Tag == dwarf::DW_TAG_inlined_subroutine;
+  // We want to know how many variables (with abstract_origin) don't have
+  // location info.
+  const bool IsCandidateForZeroLocCovTracking =
+      (IsInlinedFunction || (IsFunction && HasAbstractOrigin));
+
   AbstractOriginVarsTy AbstractOriginVars;
 
   // Get the vars of the inlined fn, so the locstats
   // reports the missing vars (with coverage 0%).
-  if (IsInlinedFunction) {
+  if (IsCandidateForZeroLocCovTracking) {
     auto OffsetFn = Die.find(dwarf::DW_AT_abstract_origin);
     if (OffsetFn) {
       uint64_t OffsetOfInlineFnCopy = (*OffsetFn).getRawUValue();
       if (GlobalAbstractOriginFnInfo.count(OffsetOfInlineFnCopy)) {
         AbstractOriginVars = GlobalAbstractOriginFnInfo[OffsetOfInlineFnCopy];
         AbstractOriginVarsPtr = &AbstractOriginVars;
       } else {
         // This means that the DW_AT_inline fn copy is out of order,
while (Child) {
 
     collectStatsRecursive(
         Child, FnPrefix, ChildVarPrefix, BytesInScope, InlineDepth, FnStatMap,
         GlobalStats, LocStats, GlobalAbstractOriginFnInfo,
         FnsWithAbstractOriginToBeProcessed, AbstractOriginVarsPtr);
     Child = Child.getSibling();
   }
 
-  if (!IsInlinedFunction)
+  if (!IsCandidateForZeroLocCovTracking)
     return;
 
-  // After we have processed all vars of the inlined function,
-  // we want to know how many variables have no location.
+  // After we have processed all vars of the inlined function (or function with
+  // an abstract_origin), we want to know how many variables have no location.
   for (auto Offset : AbstractOriginVars) {
     LocStats.NumVarParam++;
     LocStats.VarParamLocStats[ZeroCoverageBucket]++;
     auto FnDie = Die.getDwarfUnit()->getDIEForOffset(Offset);
     if (!FnDie)
       continue;
     auto Tag = FnDie.getTag();
     if (Tag == dwarf::DW_TAG_formal_parameter) {
while (Child) {
     } else if (ChildTag == dwarf::DW_TAG_lexical_block)
       updateVarsWithAbstractOriginLocCovInfo(Child, AbstractOriginVars);
     Child = Child.getSibling();
   }
 }
 
 /// Collect zero location coverage for inlined variables which refer to
 /// a DW_AT_inline copy of subprogram that is out of order in the DWARF.
+/// Also cover the variables of a concrete function (represented with
+/// the DW_TAG_subprogram) with an abstract_origin attribute.
 static void collectZeroLocCovForVarsWithAbstractOrigin(
     DWARFUnit *DwUnit, GlobalStats &GlobalStats, LocationStats &LocStats,
     AbstractOriginVarsTyMap &GlobalAbstractOriginFnInfo,
     FunctionsWithAbstractOriginTy &FnsWithAbstractOriginToBeProcessed) {
   for (auto FnOffset : FnsWithAbstractOriginToBeProcessed) {
     DWARFDie FnDieWithAbstractOrigin = DwUnit->getDIEForOffset(FnOffset);
     auto FnCopy = FnDieWithAbstractOrigin.find(dwarf::DW_AT_abstract_origin);
     AbstractOriginVarsTy AbstractOriginVars;
bool dwarfdump::collectStatsForObjectFile(ObjectFile &Obj, DWARFContext &DICtx,
 
   /// Collect the sizes of debug sections.
   SectionSizes Sizes;
   calculateSectionSizes(Obj, Sizes, Filename);
 
   /// The version number should be increased every time the algorithm is changed
   /// (including bug fixes). New metrics may be added without increasing the
   /// version.
-  unsigned Version = 7;
+  unsigned Version = 8;
   unsigned VarParamTotal = 0;
   unsigned VarParamUnique = 0;
   unsigned VarParamWithLoc = 0;
   unsigned NumFunctions = 0;
   unsigned NumInlinedFunctions = 0;
   unsigned NumFuncsWithSrcLoc = 0;
   unsigned NumAbstractOrigins = 0;
   unsigned ParamTotal = 0;