public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb: Guarantee that an SAL's end is right before the next statement
@ 2023-10-27  8:57 Guinevere Larsen
  2023-11-14 10:50 ` [PING][PATCH] " Guinevere Larsen
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Guinevere Larsen @ 2023-10-27  8:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Guinevere Larsen

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.  */
 
-- 
2.41.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PING][PATCH] gdb: Guarantee that an SAL's end is right before the next statement
  2023-10-27  8:57 [PATCH] gdb: Guarantee that an SAL's end is right before the next statement Guinevere Larsen
@ 2023-11-14 10:50 ` Guinevere Larsen
  2023-11-21 17:22   ` [PINGv2][PATCH] " Guinevere Larsen
  2023-12-07 17:13 ` [PINGv4][PATCH] " Guinevere Larsen
  2023-12-07 18:56 ` [PATCH] " Tom Tromey
  2 siblings, 1 reply; 7+ messages in thread
From: Guinevere Larsen @ 2023-11-14 10:50 UTC (permalink / raw)
  To: Guinevere Larsen, Gdb-patches

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.  */
>   


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PINGv2][PATCH] gdb: Guarantee that an SAL's end is right before the next statement
  2023-11-14 10:50 ` [PING][PATCH] " Guinevere Larsen
@ 2023-11-21 17:22   ` Guinevere Larsen
  2023-11-28  9:17     ` Guinevere Larsen
  0 siblings, 1 reply; 7+ messages in thread
From: Guinevere Larsen @ 2023-11-21 17:22 UTC (permalink / raw)
  To: Guinevere Larsen, Gdb-patches

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.  */


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PINGv2][PATCH] gdb: Guarantee that an SAL's end is right before the next statement
  2023-11-21 17:22   ` [PINGv2][PATCH] " Guinevere Larsen
@ 2023-11-28  9:17     ` Guinevere Larsen
  0 siblings, 0 replies; 7+ messages in thread
From: Guinevere Larsen @ 2023-11-28  9:17 UTC (permalink / raw)
  To: Guinevere Larsen, Gdb-patches

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PINGv4][PATCH] gdb: Guarantee that an SAL's end is right before the next statement
  2023-10-27  8:57 [PATCH] gdb: Guarantee that an SAL's end is right before the next statement Guinevere Larsen
  2023-11-14 10:50 ` [PING][PATCH] " Guinevere Larsen
@ 2023-12-07 17:13 ` Guinevere Larsen
  2023-12-07 18:56 ` [PATCH] " Tom Tromey
  2 siblings, 0 replies; 7+ messages in thread
From: Guinevere Larsen @ 2023-12-07 17:13 UTC (permalink / raw)
  To: Guinevere Larsen, Gdb-patches

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.  */
>   


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] gdb: Guarantee that an SAL's end is right before the next statement
  2023-10-27  8:57 [PATCH] gdb: Guarantee that an SAL's end is right before the next statement Guinevere Larsen
  2023-11-14 10:50 ` [PING][PATCH] " Guinevere Larsen
  2023-12-07 17:13 ` [PINGv4][PATCH] " Guinevere Larsen
@ 2023-12-07 18:56 ` Tom Tromey
  2023-12-08  9:09   ` Guinevere Larsen
  2 siblings, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2023-12-07 18:56 UTC (permalink / raw)
  To: Guinevere Larsen; +Cc: gdb-patches

>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:

Guinevere> With this new change, the edge case is removed from the processing of
Guinevere> the 'until' command without regressing the accompanying test case, and
Guinevere> no other regressions were observed in the testsuite.

Guinevere> Regression tested on fedora 37 with GCC and clang.

Thanks for the patch.

It's pretty hard to reason about this kind of thing.  SALs are sort of a
grab bag of behavior.

Anyway, I read the explanation and it made sense; and you tested it.  So
I think we should try it out.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] gdb: Guarantee that an SAL's end is right before the next statement
  2023-12-07 18:56 ` [PATCH] " Tom Tromey
@ 2023-12-08  9:09   ` Guinevere Larsen
  0 siblings, 0 replies; 7+ messages in thread
From: Guinevere Larsen @ 2023-12-08  9:09 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 07/12/2023 19:56, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:
> Guinevere> With this new change, the edge case is removed from the processing of
> Guinevere> the 'until' command without regressing the accompanying test case, and
> Guinevere> no other regressions were observed in the testsuite.
>
> Guinevere> Regression tested on fedora 37 with GCC and clang.
>
> Thanks for the patch.
>
> It's pretty hard to reason about this kind of thing.  SALs are sort of a
> grab bag of behavior.
>
> Anyway, I read the explanation and it made sense; and you tested it.  So
> I think we should try it out.
>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom
>
Thank you! I've just pushed this

-- 
Cheers,
Guinevere Larsen
She/Her/Hers


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-12-08  9:09 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-27  8:57 [PATCH] gdb: Guarantee that an SAL's end is right before the next statement Guinevere Larsen
2023-11-14 10:50 ` [PING][PATCH] " Guinevere Larsen
2023-11-21 17:22   ` [PINGv2][PATCH] " Guinevere Larsen
2023-11-28  9:17     ` Guinevere Larsen
2023-12-07 17:13 ` [PINGv4][PATCH] " Guinevere Larsen
2023-12-07 18:56 ` [PATCH] " Tom Tromey
2023-12-08  9:09   ` Guinevere Larsen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).