[PING] [PATCH] Harden gdb.base/step-over-syscall.exp
Alan Hayward
Alan.Hayward@arm.com
Wed Jan 22 14:45:00 GMT 2020
> 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"
>>
More information about the Gdb-patches
mailing list