On 8/21/24 10:45, Richard Biener wrote:
> On Wed, 21 Aug 2024, Richard Biener wrote:
>
>> On Tue, 20 Aug 2024, Bernd Edlinger wrote:
>>
>>> On 8/20/24 13:00, Richard Biener wrote:
>>>> On Fri, Aug 16, 2024 at 12:49 PM Bernd Edlinger
>>>> <[email protected]> wrote:
>>>>>
>>>>> While this already works correctly for the case when an inlined
>>>>> subroutine contains only one subrange, a redundant DW_TAG_lexical_block
>>>>> is still emitted when the subroutine has multiple blocks.
>>>>
>>>> Huh. The point is that the inline context is a single scope block with no
>>>> siblings - how did that get messed up? The patch unfortunately does not
>>>> contain a testcase.
>>>>
>>>
>>> Well, I became aware of this because I am working on a gdb patch,
>>> which improves the debug experience of optimized C code, and to my surprise
>>> the test case did not work with gcc-8, while gcc-9 and following were fine.
>>> Initially I did not see what is wrong, therefore I started to bisect when
>>> this changed, and so I found your patch, which removed some lexical blocks
>>> in the debug info of this gdb test case:
>>>
>>> from binutils-gdb/gdb/testsuite/gdb.cp/step-and-next-inline.cc
>>> in case you have the binutils-gdb already downloaded you can skip this:
>>> $ git clone git://sourceware.org/git/binutils-gdb
>>> $ cd binutils-gdb/gdb/testsuite/gdb.cp
>>> $ gcc -g -O2 step-and-next-inline.cc
>>>
>>> when you look at the debug info with readelf -w a.out
>>> you will see, that the function "tree_check"
>>> is inlined three times, one looks like this
>>> <2><86b>: Abbrev Number: 40 (DW_TAG_inlined_subroutine)
>>> <86c> DW_AT_abstract_origin: <0x95b>
>>> <870> DW_AT_entry_pc : 0x1175
>>> <878> DW_AT_GNU_entry_view: 0
>>> <879> DW_AT_ranges : 0x21
>>> <87d> DW_AT_call_file : 1
>>> <87e> DW_AT_call_line : 52
>>> <87f> DW_AT_call_column : 10
>>> <880> DW_AT_sibling : <0x8bf>
>>> <3><884>: Abbrev Number: 8 (DW_TAG_formal_parameter)
>>> <885> DW_AT_abstract_origin: <0x974>
>>> <889> DW_AT_location : 0x37 (location list)
>>> <88d> DW_AT_GNU_locviews: 0x35
>>> <3><891>: Abbrev Number: 8 (DW_TAG_formal_parameter)
>>> <892> DW_AT_abstract_origin: <0x96c>
>>> <896> DW_AT_location : 0x47 (location list)
>>> <89a> DW_AT_GNU_locviews: 0x45
>>> <3><89e>: Abbrev Number: 41 (DW_TAG_lexical_block)
>>> <89f> DW_AT_ranges : 0x21
>>>
>>> see the lexical block has the same DW_AT_ranges, as the
>>> inlined subroutine, but the other invocations do not
>>> have this lexical block, since your original fix removed
>>> those.
>>> And this lexical block triggered an unexpected issue
>>> in my gdb patch, which I owe you one, for helping me
>>> finding it :-)
>>>
>>> Before that I have never looked at these lexical blocks,
>>> but all I can say is that while compiling this test case,
>>> in the first invocation of gen_inlined_subroutine_die
>>> there are several SUBBLOCKS linked via BLOCK_CHAIN,
>>> and only the first one is used to emit the lexical_block,
>>> while the other siblings must be fully decoded, otherwise
>>> there is an internal error, that I found by try-and-error.
>>> I thought that is since the subroutine is split over several
>>> places, and therefore it appeared natural to me, that the
>>> subroutine is also using several SUBBLOCKS.
>>
>> OK, so the case in question looks like
>>
>> { Scope block #8 step-and-next-inline.cc:52 Originating from : static
>> struct tree * tree_check (struct tree *, int); Fragment chain : #16 #17
>> struct tree * t;
>> int i;
>>
>> { Scope block #9 Originating from :#0 Fragment chain : #10 #11
>> struct tree * x;
>>
>> }
>>
>> { Scope block #10 Originating from :#0 Fragment of : #9
>> struct tree * x;
>>
>> }
>>
>> { Scope block #11 Originating from :#0 Fragment of : #9
>> struct tree * x;
>>
>> }
>>
>> }
>>
>> so we have fragments here which we should ignore, but then fragments
>> are to collect multiple ranges which, when we do not emit a
>> lexical block for block #9 above, we will likely fail to emit and
>> which we instead should associate with block #8, the
>> DW_TAG_inlined_subroutine.
>>
>> Somehow it seems to "work" as to associate DW_AT_ranges with the
>> DW_TAG_inlined_subroutine.
>>
>> I've used the following - there's no need to process BLOCK_CHAIN
>> as fragments are ignored by gen_block_die.
>>
>> diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
>> index d5144714c6e..4e6ad2ab7e1 100644
>> --- a/gcc/dwarf2out.cc
>> +++ b/gcc/dwarf2out.cc
>> @@ -25194,8 +25194,13 @@ gen_inlined_subroutine_die (tree stmt, dw_die_ref
>> context_die)
>> Do that by doing the recursion to subblocks on the single subblock
>> of STMT. */
>> bool unwrap_one = false;
>> - if (BLOCK_SUBBLOCKS (stmt) && !BLOCK_CHAIN (BLOCK_SUBBLOCKS (stmt)))
>> + if (BLOCK_SUBBLOCKS (stmt))
>> {
>> + tree subblock = BLOCK_SUBBLOCKS (stmt);
>> + /* We should never elide that BLOCK, but we may have multiple
>> fragments.
>> + Assert that there's only a single real inline-scope block. */
>> + for (tree next = BLOCK_CHAIN (subblock); next; next = BLOCK_CHAIN
>> (next))
>> + gcc_checking_assert (BLOCK_FRAGMENT_ORIGIN (next) == subblock);
>> tree origin = block_ultimate_origin (BLOCK_SUBBLOCKS (stmt));
>> if (origin
>> && TREE_CODE (origin) == BLOCK
>>
>> I'm quite sure this will blow up, so the appropriate thing would be
>> to only unwrap the block if the assertion would hold.
>
> ICEs for example c-c++-common/torture/complex-sign-mixed-div.c
> and gcc.dg/torture/pr50823.c. The latter has
>
> { Scope block #24
> /space/rguenther/src/gcc/gcc/testsuite/gcc.dg/torture/pr50823.c:25
> Originating from : static void emit_pattern_after_noloc (int (*<T3dc>)
> (void)); Fragment chain : #31
> int (*<T3dc>) (void) make_raw;
>
> { Scope block #25
> /space/rguenther/src/gcc/gcc/testsuite/gcc.dg/torture/pr50823.c:29
> Originating from : static int make_insn_raw (void); Fragment chain : #28
>
> { Scope block #26 Originating from :#0 Fragment chain : #27
>
> }
>
> { Scope block #27 Originating from :#0 Fragment of : #26
>
> }
>
> }
>
> { Scope block #28
> /space/rguenther/src/gcc/gcc/testsuite/gcc.dg/torture/pr50823.c:29
> Originating from : static int make_insn_raw (void); Fragment of : #25
>
> }
>
> { Scope block #29
> /space/rguenther/src/gcc/gcc/testsuite/gcc.dg/torture/pr50823.c:30
> Originating from : static void add_insn_after (void);
>
> { Scope block #30 Originating from :#0
>
> }
>
> }
>
> }
>
> so that's for a function without any local vars, it does seem we
> have to check explicitly for the case with "proper" fragments and
> otherwise not unwrap.
>
> Can you update your patch accordingly?
>
Ah, okay,
so in terms of my current patch, you mean:
diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
index 79f97b5a55e..c30dad1513a 100644
--- a/gcc/dwarf2out.cc
+++ b/gcc/dwarf2out.cc
@@ -25183,6 +25183,10 @@ gen_inlined_subroutine_die (tree stmt, dw_die_ref
context_die)
&& TREE_CODE (origin) == BLOCK
&& BLOCK_SUPERCONTEXT (origin) == decl)
unwrap_one = true;
+ for (tree next = BLOCK_CHAIN (sub); unwrap_one && next;
+ next = BLOCK_CHAIN (next))
+ if (BLOCK_FRAGMENT_ORIGIN (next) != sub)
+ unwrap_one = false;
}
decls_for_scope (stmt, subr_die, !unwrap_one);
if (unwrap_one)
I will bootstrap this and post a new patch tomorrow.
Thanks
Bernd.
> Richard.
>
>> I'm testing the above.
>>
>> Richard.
>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>> Richard.
>>>>
>>>>> Fixes: ac02e5b75451 ("re PR debug/37801 (DWARF output for inlined
>>>>> functions
>>>>> doesn't always use DW_TAG_inlined_subroutine)")
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> PR debug/87440
>>>>> * dwarf2out.cc (gen_inlined_subroutine_die): Handle the case
>>>>> of multiple subranges correctly.
>>>>> ---
>>>>> some more context is here:
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87440#c5
>>>>> Bootstrapped and regression-tested on x86_64-pc-linux-gnu, OK for trunk?
>>>>>
>>>>> gcc/dwarf2out.cc | 11 ++++++++---
>>>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc
>>>>> index 357efaa5990..346feeb53c8 100644
>>>>> --- a/gcc/dwarf2out.cc
>>>>> +++ b/gcc/dwarf2out.cc
>>>>> @@ -25171,9 +25171,10 @@ gen_inlined_subroutine_die (tree stmt,
>>>>> dw_die_ref context_die)
>>>>> Do that by doing the recursion to subblocks on the single subblock
>>>>> of STMT. */
>>>>> bool unwrap_one = false;
>>>>> - if (BLOCK_SUBBLOCKS (stmt) && !BLOCK_CHAIN (BLOCK_SUBBLOCKS (stmt)))
>>>>> + tree sub = BLOCK_SUBBLOCKS (stmt);
>>>>> + if (sub)
>>>>> {
>>>>> - tree origin = block_ultimate_origin (BLOCK_SUBBLOCKS (stmt));
>>>>> + tree origin = block_ultimate_origin (sub);
>>>>> if (origin
>>>>> && TREE_CODE (origin) == BLOCK
>>>>> && BLOCK_SUPERCONTEXT (origin) == decl)
>>>>> @@ -25181,7 +25182,11 @@ gen_inlined_subroutine_die (tree stmt,
>>>>> dw_die_ref context_die)
>>>>> }
>>>>> decls_for_scope (stmt, subr_die, !unwrap_one);
>>>>> if (unwrap_one)
>>>>> - decls_for_scope (BLOCK_SUBBLOCKS (stmt), subr_die);
>>>>> + {
>>>>> + decls_for_scope (sub, subr_die);
>>>>> + for (sub = BLOCK_CHAIN (sub); sub; sub = BLOCK_CHAIN (sub))
>>>>> + gen_block_die (sub, subr_die);
>>>>> + }
>>>>> }
>>>>>
>>>>> /* Generate a DIE for a field in a record, or structure. CTX is
>>>>> required: see
>>>>> --
>>>>> 2.39.2
>>>>>
>>>
>>
>>
>
