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

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

On 1/22/20 11:45 AM, Simon Marchi wrote:
> On 2020-01-15 3:36 p.m., Luis Machado wrote:
>> 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
> 
> I would suggest using -1 as the initial value, as 0 is (in theory) a valid address.
> 

Thanks. Fixed this as well as the other occurrences.

>>       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 {
>> +	-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
>> +	}
> 
> This probably fails with "make check-read1".  If the characters come in one
> by one, you'll get eventually get "display/i" in the buffer, which will match
> the second regexp.
> 

True. Let me think of a better way to handle this particular case.

>> +    }
>> +
>> +    # 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
>> +		}
>>   	    }
>>   	}
> 
> Maybe I'm worrying too much, but another way this could fail (or actually fail to catch
> a failure) is if the regexp misses that syscall instruction, but catches another syscall
> later, at some point during the 1000 stepi.  Would it be good to verify that we are at the
> syscall we expect, by by checking the syscall number?  That would require knowing the
> register name that holds the syscall number, and the expected syscall numbers for fork,
> vfork and exec, for each architecture.  Those things don't change over time, and we already
> have an architecture-specific definition ($syscall_insn), so I don't think it would be
> problematic to hardcode them in the test too.
>

I'll give this a try while at it.

>> +
>> +	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
>> +	}
> 
> For consistency, you might as well get the syscall instruction address from there too.
> 

Done.

>>       }
>>   
>> -    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"
>> +    }
> 
> Use gdb_assert, so that we get a PASS if it works.
> 
> gdb_assert {$next_insn_addr == $pc_after_stepi} \
>      "pc after stepi matches insn addr after syscall"
> 
Fixed now. Thanks!