Re: [PING] [PATCH] Harden gdb.base/step-over-syscall.exp

[Alan Hayward <Alan.Hayward@arm.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 9216 bytes --]



> On 22 Jan 2020, at 13:30, Luis Machado <luis.machado@linaro.org> wrote:
> 
> ping?
> 
> https://sourceware.org/ml/gdb-patches/2020-01/msg00432.html
> 
> On 1/15/20 5:36 PM, Luis Machado wrote:
>> 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. I don't
>> feel strongly about this, so i'm OK with changing how we compare PC's for
>> the entire test if folks decide it is reasonable.

I’m happy with leaving it this way too.

>> 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
>> I've queued a test run on the buildbot to make sure things are sane for other
>> architectures, and i've tested it on aarch64-linux-gnu.
>> gdb/testsuite/ChangeLog:
>> 2020-01-15  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, 66 insertions(+), 25 deletions(-)
>> diff --git a/gdb/testsuite/gdb.base/step-over-syscall.exp b/gdb/testsuite/gdb.base/step-over-syscall.exp
>> index b373c169c0..4d9488b1d4 100644
>> --- a/gdb/testsuite/gdb.base/step-over-syscall.exp
>> +++ b/gdb/testsuite/gdb.base/step-over-syscall.exp
>> @@ -46,7 +46,8 @@ proc_with_prefix check_pc_after_cross_syscall { syscall syscall_insn_next_addr }
>>    proc setup { syscall } {
>>      global gdb_prompt syscall_insn
>> -
>> +    global hex
>> +    set next_insn_addr 0
>>      set testfile "step-over-$syscall"
>>        clean_restart $testfile
>> @@ -62,7 +63,7 @@ proc setup { syscall } {
>>      gdb_test_no_output "set displaced-stepping off" \
>>  	"set displaced-stepping off during test setup"
>>  -    gdb_test "break $syscall" "Breakpoint \[0-9\]* at .*"
>> +    gdb_test "break \*$syscall" "Breakpoint \[0-9\]* at .*"
>>        gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, (.* in |__libc_|)$syscall \\(\\).*" \
>>  	"continue to $syscall (1st time)"
>> @@ -75,37 +76,72 @@ 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 0
>> +    set test "fetch first stop pc"
>> +    gdb_test_multiple "display/i \$pc" $test {

I don’t like the use of display/i here because it will be displaying the pc after
every command, so could break other tests. Alternatively, you could stepi then x/i $pc.
However, the test already worked this way, so I’m ok with it.

>> +	-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 "display/i.*" {
>> +	    pass $test
>> +	}
>> +    }
>> +
>> +    # If we are not at the syscall instruction yet, keep looking for it with
>> +    # stepi commands.
>> +    if {$syscall_insn_addr == 0} {
>> +	# 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
>> +	    }
>> +	    -re "x/i .*=>.*\r\n$gdb_prompt $" {
>> +		incr steps
>> +		if {$steps == $max_steps} {
>> +		    fail $msg
>> +		} else {
>> +		    send_gdb "stepi\n"
>> +		    exp_continue
>> +		}
>>  	    }
>>  	}
>> +
>> +	if {$steps == $max_steps} {
>> +	    return { -1, -1 }
>> +	}
>> +
>> +	set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0" \
>> +				  "pc before stepi"]
>>      }
>>  -    if {$steps == $max_steps} {
>> -	return { -1, -1 }
>> +    # We have found the syscall instruction.  Now record the next instruction.
>> +    # Use the X command instead of stepi since we can't guarantee
>> +    # stepi is working properly.
>> +    set test "pc after syscall instruction"
>> +    gdb_test_multiple "x/2i \$pc" $test {
>> +	-re "x/2i .*=> $hex .*:.*$syscall_insn.* ($hex) .*:.*$gdb_prompt $" {
>> +	    set next_insn_addr $expect_out(2,string)
>> +	    pass $test
>> +	}
>>      }
>>  -    set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0" \
>> -			       "pc before stepi"]
>>      if {[gdb_test "stepi" "x/i .*=>.*" "stepi $syscall insn"] != 0} {
>>  	return { -1, -1 }
>>      }
>> +
>> +    set pc_after_stepi [get_hexadecimal_valueof "\$pc" "0" \
>> +			    "pc after stepi with x command"]
>> +
>> +    if {$next_insn_addr != $pc_after_stepi} {
>> +      fail "pc after stepi matches insn addr after syscall"
>> +    }
>> +
>>      return [list $syscall_insn_addr [get_hexadecimal_valueof "\$pc" \
>>  					 "0" "pc after stepi"]]

Minor nit. In the return, you could just use $next_insn_addr instead of
calling get_hexadecimal_valueof again.

>>  }
>> @@ -156,8 +192,13 @@ proc step_over_syscall { syscall } {
>>  		}
>>  	    }
>>  -	    gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, .*" \
>> -		"continue to syscall insn $syscall"
>> +	    # Check if the syscall breakpoint is at the syscall instruction
>> +	    # address.  If so, no need to continue, otherwise we will run the
>> +	    # inferior to completion.
>> +	    if {$syscall_insn_addr != [get_hexadecimal_valueof "\$pc" "0"]} {
>> +		gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, .*" \
>> +		    "continue to syscall insn $syscall"
>> +	    }
>>    	    gdb_test_no_output "set displaced-stepping $displaced"
>>  

\x16º&Öéj×!zÊÞ¶êç×­÷ëYb²Ö«r\x18\x1dn–­r\x17¬