[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
Tue Apr 20 18:40:05 PDT 2021
dblaikie added a comment.
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).
> `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.
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