[PATCH] D96045: [llvm-dwarfdump][locstats] Unify handling of inlined vars with no loc
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 21 10:16:29 PDT 2021
dblaikie added a comment.
In D96045#2704967 <https://reviews.llvm.org/D96045#2704967>, @djtodoro wrote:
> In D96045#2703754 <https://reviews.llvm.org/D96045#2703754>, @dblaikie wrote:
>
>> In D96045#2688579 <https://reviews.llvm.org/D96045#2688579>, @djtodoro wrote:
>>
>>> In D96045#2688025 <https://reviews.llvm.org/D96045#2688025>, @dblaikie wrote:
>>>
>>>> In D96045#2685000 <https://reviews.llvm.org/D96045#2685000>, @djtodoro wrote:
>>>>
>>>>> In D96045#2684570 <https://reviews.llvm.org/D96045#2684570>, @dblaikie wrote:
>>>>>
>>>>>> In D96045#2678117 <https://reviews.llvm.org/D96045#2678117>, @rdhindsa wrote:
>>>>>>
>>>>>>> After moving the variable InlinedFnsToBeProcessed to inside the for loop, I noticed a difference in two dwarfdump statistics for a quite large binary with fission=yes:
>>>>>>>
>>>>>>> #variables processed by location statistics
>>>>>>> #variables with 0% of parent scope covered by DW_AT_location
>>>>>>>
>>>>>>> Since I am not very familiar with this code, I would request if you could investigate and apply the patch.
>>>>>>
>>>>>> Hmm, looking at this trying to reproduce the behavior makes me think this wouldn't be the right fix & the change in numbers is probably reflective of a bug in the fix - specifically in LTO situations (where the abstract origin of the function may be in a different CU than the one it's referenced from/inlined into).
>>>>>>
>>>>>> But there might be a reasonable alternative fix/improvement - pruning the contents of the list once it has been handled. Possibly sorting the list so it's easy to just visit the elements that need to.
>>>>>
>>>>> I have taken a look into this and come up with the similar conclusion.
>>>>> This could be reproduced with the Split DWARF -- when building e.g. GDB project with -gsplit-dwarf option enabled, it triggers the issue. I was struggling to make a simple test case (from a high level) that meets all the requirements, but it is very hard. That is why I've sent the email: https://lists.llvm.org/pipermail/llvm-dev/2021-April/149735.html.
>>>>
>>>> Ah, hmm - why would this come up in Split DWARF, do you think? Split DWARF doesn't have any cross-CU references (the format has no way to encode them) so I'd expect no DIEs to be added to the InlinedFnsToBeProcessed list?
>>>
>>> The example I had in my mind is a bit different (I haven't looked into cross-cu support yet, but we should have covered it).
>>
>> Hmm, OK. If the intent is that this change isn't meant to work for/fix the LTO case - then changing the scope sounds like something worth doing immediately. (& the discussion below about Split DWARF sounds like it'd be a place where the current buggy code would exhibit different (incorrect) behavior). I can probably try to come up with a test case that demonstrates that buggy behavior.
>>
>>> Let me first explain two main variables here:
>>>
>>> `GlobalInlinedFnInfo` -- keeps track of `DW_TAG_subprogram` copies with `DW_AT_inline`, so when we face a `DW_TAG_inlined_subroutine` we know for how many variables we have zero % location coverage
>>
>> As an aside: This issue can come up for the concrete DW_TAG_subprogram that refers to the abstract origin, not only for the DW_TAG_inlined_subroutines that refer to the abstract origin. ie:
>>
>> $ cat test.cpp
>> void f1();
>> __attribute__((always_inline)) void f2() {
>> int i;
>> f1();
>> }
>> void f3() {
>> f2();
>> }
>> blaikie at blaikie-linux2:~/dev/scratch$ clang++-tot -O3 test.cpp -c -g && llvm-dwarfdump-tot test.o | grep "DW_TAG\|abstract_origin\|DW_AT_name"
>> 0x0000000b: DW_TAG_compile_unit
>> DW_AT_name ("test.cpp")
>> 0x0000002a: DW_TAG_subprogram
>> DW_AT_abstract_origin (0x0000005b "_Z2f2v")
>> 0x0000003d: DW_TAG_variable
>> DW_AT_abstract_origin (0x00000067 "i")
>> 0x00000042: DW_TAG_GNU_call_site
>> DW_AT_abstract_origin (0x00000050 "_Z2f1v")
>> 0x00000050: DW_TAG_subprogram
>> DW_AT_name ("f1")
>> 0x0000005b: DW_TAG_subprogram
>> DW_AT_name ("f2")
>> 0x00000067: DW_TAG_variable
>> DW_AT_name ("i")
>> 0x00000073: DW_TAG_base_type
>> DW_AT_name ("int")
>> 0x0000007a: DW_TAG_subprogram
>> DW_AT_name ("f3")
>> 0x00000093: DW_TAG_inlined_subroutine
>> DW_AT_abstract_origin (0x0000005b "_Z2f2v")
>> 0x000000a7: DW_TAG_GNU_call_site
>> DW_AT_abstract_origin (0x00000050 "_Z2f1v")
>>
>> So currently LLVM still has the "emitting a DIE with only an abstract origin for variables that are optimized away" issue in the concrete out of line definition (0x2a/0x3d above) though it's been fixed in the inlined case (0x93 above). And this patch for statistics addresses the latter case (the inlined_subprogram) but doesn't address the former case (if LLVM were improved not to produce that basically dead concrete variable DIE).
>
> Oh, yes, I think the D54217 <https://reviews.llvm.org/D54217> have introduced a bug here (since we should consider dw_subprogram with abstract_origin as inlined function) and I guess we should make something like this:
>
> diff --git a/llvm/tools/llvm-dwarfdump/Statistics.cpp b/llvm/tools/llvm-dwarfdump/Statistics.cpp
> index 3758a56da7a2..4d0cf6b5fc8d 100644
> --- a/llvm/tools/llvm-dwarfdump/Statistics.cpp
> +++ b/llvm/tools/llvm-dwarfdump/Statistics.cpp
> @@ -462,9 +462,15 @@ static void collectStatsRecursive(DWARFDie Die, std::string FnPrefix,
> return;
>
> // Handle any kind of lexical scope.
> - const bool IsFunction = Tag == dwarf::DW_TAG_subprogram;
> + const bool HasAbstractOriginAttr =
> + Die.find(dwarf::DW_AT_abstract_origin) != None;
> + const bool IsFunction =
> + ((Tag == dwarf::DW_TAG_subprogram) && !HasAbstractOriginAttr);
> const bool IsBlock = Tag == dwarf::DW_TAG_lexical_block;
> - const bool IsInlinedFunction = Tag == dwarf::DW_TAG_inlined_subroutine;
> + const bool IsInlinedFunction =
> + (Tag == dwarf::DW_TAG_inlined_subroutine ||
> + (Tag == dwarf::DW_TAG_subprogram && HasAbstractOriginAttr));
> +
> InlinedVarsTy InlinedVars;
> // Get the vars of the inlined fn, so the locstats
> // reports the missing vars (with coverage 0%).
More or less - I think we'll need to find another name other than "inlined" for these "concrete and inlined instances with abstract origins", otherwise it's liable to be confusing to future readers/maintainers.
>>> `InlinedFnsToBeProcessed` -- doing recursive approach if we can firsly meet a `DW_TAG_inlined_subroutine` referencing a `DW_TAG_subprogram` copy with `DW_AT_inline` (since DWARF standard does not propose the order), this variable keeps the `DW_TAG_inlined_subroutine` DIEs that should be processed after we are finished with the dwarf tree
>>>
>>> Let us know consider a big project (let it be GDB) compiled with -gsplit-dwarf. Main executable will have debug_info section:
>>>
>>> $ llvm-dwarfdump gdb
>>> 0x0000000b: DW_TAG_compile_unit
>>> DW_AT_stmt_list (0x00000000)
>>> DW_AT_comp_dir ("/gdb")
>>> DW_AT_GNU_pubnames (true)
>>> DW_AT_GNU_dwo_name ("gdb.dwo")
>>> DW_AT_GNU_dwo_id (0xb8ed71b1bb823706)
>>> DW_AT_low_pc (0x0000000000482370)
>>> DW_AT_high_pc (0x000000000048239a)
>>> DW_AT_GNU_addr_base (0x00000000)
>>> 0x00000030: Compile Unit: length = 0x00000030, format = DWARF32, version = 0x0004, abbr_offset = 0x001a, addr_size = 0x08 (next unit at 0x00000064)
>>>
>>> 0x0000003b: DW_TAG_compile_unit
>>> DW_AT_stmt_list (0x00000069)
>>> DW_AT_comp_dir ("/gdb")
>>> DW_AT_GNU_pubnames (true)
>>> DW_AT_GNU_dwo_name ("amd64-tdep.dwo")
>>> DW_AT_GNU_dwo_id (0x97c5a9e4ce1a43bf)
>>> DW_AT_GNU_ranges_base (0x00000000)
>>> DW_AT_low_pc (0x00000000004823a0)
>>> DW_AT_high_pc (0x000000000048e982)
>>> DW_AT_GNU_addr_base (0x00000018)
>>> 0x00000064: Compile Unit: length = 0x0000002c, format = DWARF32, version = 0x0004, abbr_offset = 0x0037, addr_size = 0x08 (next unit at 0x00000094)
>>>
>>> 0x0000006f: DW_TAG_compile_unit
>>> ....
>>>
>>> We will keep filling both of these variables (GlobalInlinedFnInfo and InlinedFnsToBeProcessed) for all the CUs, but there could be a situation where we have inlined_subroutine referencing a subprogram, but we accidently use variables info from subprogram from previous CU with the same DIE offset -- that is why we have different numbers for this case.
>>
>> Yep, that sounds reasonably buggy & in the absence of LTO/if this code isn't intended to handle the LTO case (which it seems it isn't) then moving the variables to the inner scope for now - pending work to address LTO - sounds right to me.
>>
>> Here's a test case for the scope change fix/bug with split-dwarf:
>> a.cpp:
>>
>> __attribute__((optnone)) static void x() {
>> }
>> __attribute__((always_inline)) static void y() {
>> int var;
>> x();
>> }
>> void f1() {
>> y();
>> }
>>
>> b.cpp:
>>
>> __attribute__((optnone)) static void x() {
>> }
>> __attribute__((always_inline)) static void y() {
>> int var;
>> x();
>> }
>> void f2() {
>> y();
>> }
>>
>> c.cpp:
>>
>> void f1();
>> void f2();
>> int main() {
>> f1();
>> f2();
>> }
>>
>> $ clang++ a.cpp b.cpp c.cpp -g -gsplit-dwarf -O1
>> $ llvm-dwarfdump --statistics a.out
>>
>> (you could compile c.cpp without debug info, or put main in a or b.cpp at the end, I think it'd still work).
>>
>> With the current coed, this reports:
>>
>> "#variables processed by location statistics": 3,
>> "#variables with 0% of parent scope covered by DW_AT_location": 3,
>> ...
>> "#local vars processed by location statistics": 3,
>> "#local vars with 0% of parent scope covered by DW_AT_location": 3,
>>
>> Moving the variables into the nested scope causes these to all change to `2`, which is correct.
>
> Great! Thanks a lot!
> This is the patch: https://reviews.llvm.org/D100951.
Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96045/new/
https://reviews.llvm.org/D96045
More information about the llvm-commits
mailing list