This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by djtodoro on Apr 22 2021, 12:18 AM.

Details

Summary

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

Diff Detail

Event Timeline

djtodoro created this revision.Apr 22 2021, 12:18 AM
djtodoro requested review of this revision.Apr 22 2021, 12:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 12:18 AM

@dblaikie I guess you have suggested renaming the IsInlinedFunction.

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

dblaikie added inline comments.Apr 22 2021, 8:08 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–466

"IsPotentialAbstractOrigin"?

465–472

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 added inline comments.Apr 23 2021, 8:41 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–466

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

465–472

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 added inline comments.Apr 23 2021, 1:39 PM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 updated this revision to Diff 340521.Apr 26 2021, 7:26 AM
  • Adding the test
  • Addressing comments
djtodoro added inline comments.Apr 26 2021, 7:29 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 added inline comments.Apr 26 2021, 10:32 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 added inline comments.Apr 26 2021, 11:56 PM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 added inline comments.Apr 27 2021, 12:11 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 updated this revision to Diff 340737.Apr 27 2021, 12:23 AM
  • Bump the stats version
dblaikie added inline comments.Apr 27 2021, 11:44 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 added inline comments.Apr 28 2021, 3:45 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
465–472

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 updated this revision to Diff 341131.Apr 28 2021, 3:52 AM
  • make distinguishing between "XxxxWithAbstractOrigin" and "AbstractOriginXxx"
djtodoro retitled this revision from [llvm-dwarfdump] Fix inline function stats calculation to [llvm-dwarfdump] Fix abstract origin vars location stats calculation.Apr 28 2021, 3:56 AM

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?

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 updated this revision to Diff 344038.May 10 2021, 6:10 AM
dblaikie accepted this revision.May 10 2021, 8:55 AM

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)

This revision is now accepted and ready to land.May 10 2021, 8:55 AM

@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)

This revision was landed with ongoing or failed builds.May 11 2021, 1:05 AM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.May 11 2021, 2:15 PM
llvm/tools/llvm-dwarfdump/Statistics.cpp
230–231

Yep!