RE: [PATCH] PowerPC, fix gdb.reverse/finish-reverse-bkpt.exp and gdb.reverse/next-reverse-bkpt-over-sr.exp

[Carl Love <cel@us.ibm.com>]

Bruno:

On Tue, 2022-11-29 at 09:52 +0100, Bruno Larsen wrote:

<snip> 
> 
> > 
> > This patch fixes two PowerPC test failures in each of the tests
> > gdb.reverse/finish-reverse-bkpt.exp and
> > gdb.reverse/next-reverse-bkpt-over-sr.exp.
> > 
> > Patch tested on PowerPC and Intel X86-64 with no regressions.
> 
> Hi Carl,
> 
> I also tested locally and verify that it adds no new regressions,
> and 
> your explanations make sense. I have some style nits inlined, but
> with 
> those fixed you can add my R-b tag:
> 
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> 
> > ---
> > 
<snip>

> > +   platforms.  See comments in finish-reverse-bkpt.exp.  */
> > +
> > +int void_test = 0;
> > +
> > +void void_func ()
> 
> GDB's coding style would say this should be:
> 
> void
> void_func ()
> {
Fixed
> 
> I know that a lot of .c files in the test suite aren't completely 
> following the coding standards, but we should avoid introducing new
> ones 
> if not necessary.

Yup, I know better.  Because I am used to doing it in line, it doesn't
jump out at me a wrong.  I have to actually stop and think about it to
see it.  Sigh hard to teach an "old dog" new tricks.  :-).  I fixed up
the four occurrences of the mistake.


> > +{
> > +  void_test = 1;		/* VOID FUNC */
> > +}
> > +
> > +int main (int argc, char **argv)
> same here.

Fixed. 
> > +{
> > +  int i;
> > +  void (*funp) (void) = void_func;
> > +
> > 
<snip>
> > set breakpoint on funp"
> > +gdb_continue_to_breakpoint "funp call" ".*$srcfile:$breakloc.*"
> > +
> > +# Start the test
> missing period at the end of the comment.

Yup, fixed.

> >   set breakloc [gdb_get_line_number "VOID FUNC" "$srcfile"]
> >   gdb_test "tbreak void_func" \
> > 
<snip>

> > t myglob = 0;
> > +
> > +int callee() {		/* ENTER CALLEE */
> Same comment about C coding style.

Fixed.

> > +  return myglob++;	/* ARRIVED IN CALLEE */
> > +}			/* RETURN FROM CALLEE */
> > +
> > +int main () {

Fixed

> > +   int (*funp) (void) = callee;
> > +
> > +   /* Test next-reverse-bkpt-over-sr.exp needs to call function
> > callee using
> > +      a function pointer to work correctly on PowerPC.  See
> > comments in
> > +      next-reverse-bkpt-over-sr.exp.  */
> > +   funp ();       /* FUNCTION PTR CALL TO CALLEE */
> > +
> > +   /* Test that "step" doesn't */
> > +   callee ();	/* STEP INTO CALL AFTER FUNP CALL */
> 
> I don't see the need for a callee call here. The test doesn't use
> the 
> second call at any point.

True, it is only used as a reference point to break on.  The exit (0)
statement could be used instead.  My first thought was to let leave it
as it was.  But after thinking about it for awhile, I decided it was
best to remove it since the comment "STEP INTO CALL AFTER FUNP CALL" is
technically misleading as the test doesn't actually step into callee().
So, in the end I decided to remove the callee (); line and fix the
expect file to break on the following exit (0) call.

Retested both test cases on Power to make sure I didn't have any typos.

> As I said before, these are all minor style comments, so I don't
> think a 
> new version is needed for these comments :)

Given the above functional change, I need to post the updated version. 
I did go ahead and add the reviewed by tag.

                        Carl