Re: [PATCH v2 2/2] ppc: recognize all program traps

[Jan Vrany <jan.vrany@labware.com>]

On Tue, 2021-11-30 at 12:17 +0000, Pedro Alves wrote:
> On 2021-11-24 13:09, Jan Vrany via Gdb-patches wrote:
> > +# Number of expected SIGTRAP's to get.  This needs to be kept in
> > sync
> > +# with the source file.
> > +set expected_traps 3
> > +set keep_going 1
> > +set count 0
> > +
> > +# Make sure we have a lower timeout in case GDB doesn't support a
> > particular
> > +# instruction.  Such instruction will cause GDB to loop
> > infinitely.
> > +while {$keep_going} {
> > +    # Continue to next program breakpoint instruction.
> > +    gdb_test_multiple "continue" "trap instruction $count causes
> > SIGTRAP" {
> > +       -re "Program received signal SIGTRAP, Trace/breakpoint
> > trap.*$gdb_prompt $" {
> > +           pass $gdb_test_name
> > +
> > +           # Advance PC to next instruction
> > +           gdb_test "set \$pc = \$pc + 4" "" "advance past trap
> > instruction $count"
> > +
> > +           incr count
> > +       }
> > +       # We've reached the end of the test.
> > +       -re "exited with code 01.*$gdb_prompt $" {
> > +           set keep_going 0
> > +       }
> > +       timeout {
> > +           fail $gdb_test_name
> > +           set keep_going 0
> > +       }
> > +    }
> > +}
> > +
> 
> 
> This while loop will iterate forever if gdb_test_multiple trips on
> some internal match, like
> some unexpected text out of gdb that ends with a prompt, or an
> internal error.  The logic of
> "keep_going" should be reversed so that the loop breaks if anything
> goes wrong.   Also, the
> last continue to "exited with code 01" doesn't itself issue a pass
> unlike the other continues,
> which seems inconsistent.  BTW, if we flip the logic, we no longer
> need the timeout check.
> 
> Something like this (completely untested):
> 
> ~~~~~~~~~~~~
> # Number of expected SIGTRAP's to get.  This needs to be kept in sync
> # with the source file.
> set expected_traps 3
> set count 0
> set keep_going 1
> 
> # Make sure we have a lower timeout in case GDB doesn't support a
> particular
> # instruction.  Such instruction will cause GDB to loop infinitely.
> while {$keep_going} {
> 
>     set keep_going 0
> 
>     # Continue to next program breakpoint instruction.
>     gdb_test_multiple "continue" "trap instruction $count causes
> SIGTRAP" {
>         -re "Program received signal SIGTRAP, Trace/breakpoint
> trap.*$gdb_prompt $" {
>             pass $gdb_test_name
> 
>             # Advance PC to next instruction
>             gdb_test "set \$pc = \$pc + 4" "" "advance past trap
> instruction $count"
> 
>             incr count
>             if {$count < $expected_traps} {
>                set keep_going 1
>             }
>         }
>     }
> 
> # Verify we stopped at the expected number of SIGTRAP's.
> gdb_assert {$count == $expected_traps} "all trap instructions
> triggered"
> 
> # One last continue to reach the end of the test, to make sure we
> don't get
> # another SIGTRAP.
> gdb_test "continue" "exited with code 01.*" "continue to end"
> ~~~~~~~~~~~~
> 
> 
> Some thing in the other file.  Or you could merge the files, and make
> the exp file
> set a different .S filename and different "expected_traps" depending
> on arch, to
> avoid duplication.


Good point. I'll do as suggested in next version. 

> 
> 
> BTW, I don't understand what the "Make sure we have a lower timeout"
> comment is referring
> to -- I don't see anything changing the timeout.

Neither I do, to be honest. These tests are shameless hacked copies of
existing gdb.arch/aarch64-brk-patterns.exp test so this and the 
comment above also apply to it as well. 

I prefer not to change aarch64 test now since I do not have aarch64 
test environment set up yet.

> 
> BTW², seems strange to expect that the program exits with exit code 1
> instead of 0 on a success run.
> I can't write PPC assembly to save my life, but I don't see any "1",
> or writing to r3 in in
> the assembly, so I guess that is assuming the value is already 1 on
> entry.  I wonder whether that's a
> good assumption in all environments.

This works because main gets "int argc" as first parameter and test
runs program with no aguments so argc is always 1. 
AFAIK, in all calling conventions used on PPC first argument is passed
in r3 which is also a return register. 

But I agree this is little to hacky. I'll change it. 

Thanks! Jan