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

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

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.