Re: [PINGv2][PATCH] gdb: Guarantee that an SAL's end is right before the next statement

[Guinevere Larsen <blarsen@redhat.com>]

Ping!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers
On 21/11/2023 18:22, Guinevere Larsen wrote:
> Ping!
>
> -- 
> Cheers,
> Guinevere Larsen
> She/Her/Hers
> On 14/11/2023 11:50, Guinevere Larsen wrote:
>> Ping!
>>
>> -- 
>> Cheers,
>> Guinevere Larsen
>> She/Her/Hers
>>
>>
>> On 27/10/2023 10:57, Guinevere Larsen wrote:
>>> When examining a failure that happens when testing
>>> gdb.python/py-symtab.c with clang, I noticed that it was going wrong
>>> because the test assumed that whenever we get an SAL, its end would
>>> always be right before statement in the line table. This is true for 
>>> GCC
>>> compiled binaries, since gcc only adds statements to the line table, 
>>> but
>>> not true for clang compiled binaries.
>>>
>>> This is the second time I run into a problem where GDB doesn't handle
>>> non-statement line table entries correctly. The other was eventually
>>> committed as 9ab50efc463ff723b8e9102f1f68a6983d320517: "gdb: fix until
>>> behavior with trailing !is_stmt lines", but that commit only changes 
>>> the
>>> behavior for the 'until' command. In this patch I propose a more 
>>> general
>>> solution, making it so every time we generate the SAL for a given 
>>> pc, we
>>> set the end of the SAL to before the next statement or the first
>>> instruciton in the next line, instead of naively assuming that to be 
>>> the
>>> case.
>>>
>>> With this new change, the edge case is removed from the processing of
>>> the 'until' command without regressing the accompanying test case, and
>>> no other regressions were observed in the testsuite.
>>>
>>> Regression tested on fedora 37 with GCC and clang.
>>>
>>> ---
>>>   gdb/infcmd.c | 39 ---------------------------------------
>>>   gdb/symtab.c | 10 ++++++++--
>>>   2 files changed, 8 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>>> index cf8cd527955..72dc8231523 100644
>>> --- a/gdb/infcmd.c
>>> +++ b/gdb/infcmd.c
>>> @@ -1363,45 +1363,6 @@ until_next_command (int from_tty)
>>>           tp->control.step_range_start = func->value_block 
>>> ()->entry_pc ();
>>>         tp->control.step_range_end = sal.end;
>>> -
>>> -      /* By setting the step_range_end based on the current pc, we are
>>> -     assuming that the last line table entry for any given source line
>>> -     will have is_stmt set to true.  This is not necessarily the case,
>>> -     there may be additional entries for the same source line with
>>> -     is_stmt set false.  Consider the following code:
>>> -
>>> -     for (int i = 0; i < 10; i++)
>>> -       loop_body ();
>>> -
>>> -     Clang-13, will generate multiple line table entries at the end of
>>> -     the loop all associated with the 'for' line.  The first of these
>>> -     entries is marked is_stmt true, but the other entries are is_stmt
>>> -     false.
>>> -
>>> -     If we only use the values in SAL, then our stepping range may not
>>> -     extend to the end of the loop. The until command will reach the
>>> -     end of the range, find a non is_stmt instruction, and step to the
>>> -     next is_stmt instruction. This stopping point, however, will be
>>> -     inside the loop, which is not what we wanted.
>>> -
>>> -     Instead, we now check any subsequent line table entries to see if
>>> -     they are for the same line.  If they are, and they are marked
>>> -     is_stmt false, then we extend the end of our stepping range.
>>> -
>>> -     When we finish this process the end of the stepping range will
>>> -     point either to a line with a different line number, or, will
>>> -     point at an address for the same line number that is marked as a
>>> -     statement.  */
>>> -
>>> -      struct symtab_and_line final_sal
>>> -    = find_pc_line (tp->control.step_range_end, 0);
>>> -
>>> -      while (final_sal.line == sal.line && final_sal.symtab == 
>>> sal.symtab
>>> -         && !final_sal.is_stmt)
>>> -    {
>>> -      tp->control.step_range_end = final_sal.end;
>>> -      final_sal = find_pc_line (final_sal.end, 0);
>>> -    }
>>>       }
>>>     tp->control.may_range_step = 1;
>>>   diff --git a/gdb/symtab.c b/gdb/symtab.c
>>> index 5ec56f4f2af..090f6415af6 100644
>>> --- a/gdb/symtab.c
>>> +++ b/gdb/symtab.c
>>> @@ -3207,10 +3207,16 @@ find_pc_sect_line (CORE_ADDR pc, struct 
>>> obj_section *section, int notcurrent)
>>>              unrelocated_addr (pc - objfile->text_section_offset ()),
>>>              pc_compare));
>>>         if (item != first)
>>> -    prev = item - 1;        /* Found a matching item.  */
>>> +    {
>>> +      prev = item - 1;        /* Found a matching item.  */
>>> +      /* At this point, prev is a line whose address is <= pc.  
>>> However, we
>>> +         don't know if ITEM is pointing to the same statement or 
>>> not.  */
>>> +      while (item != last && prev->line == item->line && 
>>> !item->is_stmt)
>>> +        item++;
>>> +    }
>>>           /* At this point, prev points at the line whose start addr 
>>> is <= pc, and
>>> -     item points at the next line.  If we ran off the end of the 
>>> linetable
>>> +     item points at the next statement.  If we ran off the end of 
>>> the linetable
>>>        (pc >= start of the last line), then prev == item. If pc < 
>>> start of
>>>        the first line, prev will not be set.  */ 


-- 
Cheers,
Guinevere Larsen
She/Her/Hers