[v2] Harden gdb.base/step-over-syscall.exp

New in v2:

- Set initial values to -1 instead of 0.
- Rewrote RE to prevent unexpected matching when parsing one character at a
  time.
- Used gdb_assert for an additional check.
- Validated with check-read1

Simon,

I did some research on checking the syscall numbers to make sure we're calling
the right syscall, but there seems to be considerable variation in terms of
what registers are used to pass the syscall number and also the syscall
number itself.

For example, aarch64 seems to use clone for fork/vfork, but arm does not. The
syscall number comes in through w0, but it also gets passed via x8 if it goes
into the kernel.

Given the added complexity and the fact that the test is already breaking into
fork/vfork/clone, i think we can be reasonably sure that we are invoking the
right syscall. What do you think? Any other ideas?

Luis

---

There are a couple problems with this test.

First
--

gdb.base/step-over-syscall.exp records the address of a syscall instruction
within fork/vfork/clone functions and also the address of the instruction
after that syscall instruction.

It uses these couples addresses to make sure we stepped over a syscall
instruction (fork/vfork/clone events) correctly.

The way the test fetches the addresses of the instructions is by stepi-ing
its way through the fork/vfork/clone functions until it finds a match for
a syscall. Then it stepi's once again to get the address of the next
instruction.

This assumes that stepi-ing over a syscall is working correctly and landing
in the right PC. This is not the case for AArch64/Linux, where we're
landing a couple instructions after the syscall in some cases.

The following patch lets the test execute as before, but adds a new instruction
address check using the x command as opposed to stepi.

I didn't want to change how the test works since we may also be
interested in checking if stepi-ing over the syscall under different
conditions (displaced stepping on/off) yields the same results.

Second
--

FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: continue to vfork (3rd time) (the program exited)
FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: continue to syscall insn vfork (the program is no longer running)
FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: single step over vfork (the program is no longer running)

Depending on the glibc version we may have different code generated for the
fork/vfork/clone functions.

I ran into the situation where vfork for newer glibc's on AArch64/Linux is
very short, so "break vfork" will put a breakpoint right at the syscall
instruction, which is something the testcase isn't expecting (a off-by-1
of sorts).

The patch adds extra code to handle this case. If the test detects we're
already sitting at a syscall instruction, it records the address and moves
on to record the address after that particular instruction.

Another measure is to "break *$syscall" instead of "break $syscall". That
guarantees we're stopping at the first instruction of the syscall function,
if it ever happens that the syscall instruction is the first instruction of
those functions.

With these changes i can fix some failures for aarch64-linux-gnu and also
expose the problems i've reported here:

https://sourceware.org/ml/gdb-patches/2019-12/msg01071.html

These tests now fail for aarch64-linux-gnu (patch for this is going through
reviews):

FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=off: pc after stepi matches insn addr after syscall
FAIL: gdb.base/step-over-syscall.exp: vfork: displaced=on: pc after stepi matches insn addr after syscall

gdb/testsuite/ChangeLog:

2020-01-24  Luis Machado  <luis.machado@linaro.org>

	* gdb.base/step-over-syscall.exp (setup): Check if we're already
	sitting at a syscall instruction when we hit the syscall function's
	breakpoint.
	Check PC against one obtained with the x command.
	(step_over_syscall): Don't continue to the syscall instruction if
	we're already there.
---
 gdb/testsuite/gdb.base/step-over-syscall.exp | 91 ++++++++++++++------
 1 file changed, 64 insertions(+), 27 deletions(-)

-- 
2.17.1

On 2020-01-24 11:35 a.m., Luis Machado wrote:
> New in v2:

> 

> - Set initial values to -1 instead of 0.

> - Rewrote RE to prevent unexpected matching when parsing one character at a

>   time.

> - Used gdb_assert for an additional check.

> - Validated with check-read1

> 

> Simon,

> 

> I did some research on checking the syscall numbers to make sure we're calling

> the right syscall, but there seems to be considerable variation in terms of

> what registers are used to pass the syscall number and also the syscall

> number itself.


I would expect it to have exactly one register and one syscall number per architecture.

> For example, aarch64 seems to use clone for fork/vfork, but arm does not. The

> syscall number comes in through w0, but it also gets passed via x8 if it goes

> into the kernel.


So when we are stopped about to execute the svc instruction, the value should
be in x8, that's the one we would care about.

> Given the added complexity and the fact that the test is already breaking into

> fork/vfork/clone, i think we can be reasonably sure that we are invoking the

> right syscall. What do you think? Any other ideas?


Yeah, it's probably overkill, forget about that.

> @@ -75,39 +76,70 @@ proc setup { syscall } {

>      # Hit the breakpoint on $syscall for the second time.  In this time,

>      # the address of syscall insn and next insn of syscall are recorded.

>  

> -    gdb_test "display/i \$pc" ".*"

> -

> -    # Single step until we see a syscall insn or we reach the

> -    # upper bound of loop iterations.

> -    set msg "find syscall insn in $syscall"

> -    set steps 0

> -    set max_steps 1000

> -    gdb_test_multiple "stepi" $msg {

> -	-re ".*$syscall_insn.*$gdb_prompt $" {

> -	    pass $msg

> +    # Check if the first instruction we stopped at is the syscall one.

> +    set syscall_insn_addr -1

> +    set test "fetch first stop pc"

> +    gdb_test_multiple "display/i \$pc" $test {

> +	-re "display/i .*: x/i .*=> ($hex) .*:.*$syscall_insn.*$gdb_prompt $" {

> +	    set syscall_insn_addr $expect_out(1,string)

> +	    pass $test

>  	}

> -	-re "x/i .*=>.*\r\n$gdb_prompt $" {

> -	    incr steps

> -	    if {$steps == $max_steps} {

> -		fail $msg

> -	    } else {

> -		send_gdb "stepi\n"

> -		exp_continue

> +	-re ".*$gdb_prompt $" {

> +	    pass $test

> +	}

> +    }


Instead of setting the test variable, you can now just pass the test name to
gdb_test_multiple and access it inside the callbacks as $gdb_test_name.  See
commit:

gdb/testsuite: Add gdb_test_name variable
3d63690a0316d92cf248542ee12a3fc8b30152ea

The patch LGTM with that fixed.

Simon