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

[Luis Machado <luis.machado@linaro.org>]

On 1/22/20 11:26 AM, Alan Hayward wrote:
> 
> 
>> 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. >

Do you mean $pc_after_stepi as opposed to $next_insn_addr? 
$pc_after_stepi is what we get from the $pc register. $next_insn_addr is 
what we get from using the X command.

>>>   }
>>> @@ -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"
>>>   
>