[PATCH] D101025: [llvm-dwarfdump] Fix inline function stats calculation

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:470-472
+  const bool IsInlinedFunction =
+      (Tag == dwarf::DW_TAG_inlined_subroutine ||
+       (Tag == dwarf::DW_TAG_subprogram && HasAbstractOriginAttr));
----------------
dblaikie wrote:
> djtodoro wrote:
> > dblaikie wrote:
> > > djtodoro wrote:
> > > > dblaikie wrote:
> > > > > 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)
> > > > 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.
> > > 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`)
> > > 
> > > 
> > > 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`)
> > > 
> > 
> > 
> > > 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?
> > > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101025/new/

https://reviews.llvm.org/D101025