Re: [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[Luis Machado <luis.machado@arm.com>]

On 4/5/22 16:15, will schmidt wrote:
> On Tue, 2022-04-05 at 09:36 +0100, Luis Machado wrote:
>> On 4/4/22 17:55, will schmidt wrote:
>>> On Thu, 2022-03-31 at 14:52 +0100, Luis Machado via Gdb-patches
>>> wrote:
>>>> v2:
>>>> - Check if both the line and symtab match for a particular line
>>>> table entry.
>>>>
>>>> --
>>>>
>>>> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also
>>>> spotted on
>>>> the ppc backend), I noticed some failures in gdb.reverse/solib-
>>>> precsave.exp
>>>> and gdb.reverse/solib-reverse.exp.
>>>>
>>>> The failure happens around the following code:
>>>>
>>>> 38  b[1] = shr2(17);		/* middle part two */
>>>> 40  b[0] = 6;   b[1] = 9;	/* generic statement, end part two */
>>>> 42  shr1 ("message 1\n");	/* shr1 one */
>>>>
>>>> Normal execution:
>>>>
>>>> - step from line 38 will land on line 40.
>>>> - step from line 40 will land on line 42.
>>>>
>>>> Reverse execution:
>>>>
>>>> - step from line 42 will land on line 40.
>>>> - step from line 40 will land on line 40.
>>>> - step from line 40 will land on line 38.
>>>>
>>>> The problem here is that line 40 contains two contiguous but
>>>> distinct
>>>> PC ranges in the line table, like so:
>>>>
>>>> Line 40 - [0x7ec ~ 0x7f4]
>>>> Line 40 - [0x7f4 ~ 0x7fc]
>>>
>>> <snip>
>>>
>>>
>>>> I'm not particularly happy with how we go back in the ranges
>>>> (using "pc - 1").
>>>> Feedback would be welcome.
>>>
>>> I suppose there could be a loop of some sort that iterates
>>> backwards to
>>> a valid line; though I'd think pc - 1 is sufficient?
>>>
>>
>> Yes, it seems to do the job.
>>
>>>> Validated on aarch64-linux/Ubuntu 20.04/18.04. Carl Love has
>>>> verified that it
>>>> does fix a similar issue on ppc.
>>>>
>>>> Ubuntu 18.04 doesn't actually run into these failures because the
>>>> compiler
>>>> doesn't generate distinct PC ranges for the same line.
>>>>
>>>> I see similar failures on x86_64 in the gdb.reverse tests
>>>> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp).
>>>> Those are
>>>> also fixed by this patch, although Carl's testcase doesn't fail
>>>> for x86_64.
>>>>
>>>> There seems to be a corner case where a line table has only one
>>>> instruction,
>>>> and while stepping that doesn't take the same path through
>>>> infrun. I think it
>>>> needs some more investigation. Therefore this is a tentative
>>>> patch for now.
>>>
>>> Are you (or Carl) continuing to pursue that edge case?
>>>
>>
>> Not at the moment unfortunately. I know that this needs to be handled
>> in
>> the fallthrough of process_event_stop_test. Carl's test doesn't seem
>> to
>> fail for x86_64 (at least for me). We need to polish the testcase a
>> bit
>> more so that we can cover that corner case as well.
>>
>> Also, this is more of a RFC at this point. You'll notice some debug
>> print statement in the patch. It would be nice to turn those into
>> permanent debug prints, as this code doesn't seem to print too much
>> detail about what it is doing.
> 
> OK,
> thanks.
> 
> I did notice the debug printfs.   Since they were actually using
> the infrun_debug_printf helper, versus actual printf calls,  I assumed
> they were deliberate and meant for the long term.  :-)
> I wasn't going to
> nit on the uppercase content of the debug printfs, I figure since they
> are actually debug for special circumstances the debug was fine as
> presented.  :-)

The upper case is so the text stands out from the rest of the debugging 
output. :-)

> I'm generally a fan of adding more debug output.

I think we should, in this case. Otherwise GDB will silently pick a 
different stepping range without making it clear why it did it.