| Message ID | 20200115203645.26360-1-luis.machado@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Harden gdb.base/step-over-syscall.exp | expand |
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. > > 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 { > + -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"]] > } > @@ -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" > >
> 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" >>
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. > 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. > + } > + > + # 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. > + > + 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. > } > > - 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" Simon
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" >>> >
> On 22 Jan 2020, at 15:20, Luis Machado <luis.machado@linaro.org> wrote: > > 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: >>>> >>>> + >>>> + 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. Yes, $pc_after_stepi :) > >>>> } >>>> @@ -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"
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!
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 { + -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"]] } @@ -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"