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

[will schmidt <will_schmidt@vnet.ibm.com>]

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.  :-)
I'm generally a fan of adding more debug output.

Thanks
-Will