* [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 @ 2021-01-26 18:03 Tom de Vries 2021-01-28 15:03 ` Simon Marchi via Gdb-patches 0 siblings, 1 reply; 7+ messages in thread From: Tom de Vries @ 2021-01-26 18:03 UTC (permalink / raw) To: gdb-patches Hi, When running test-case gdb.opt/solib-intra-step.exp with target board unix/-m32 and gcc-10, I run into: ... (gdb) step^M __x86.get_pc_thunk.bx () at ../sysdeps/i386/crti.S:68^M 68 ../sysdeps/i386/crti.S: No such file or directory.^M (gdb) step^M shlib_second (dummy=0) at solib-intra-step-lib.c:23^M 23 abort (); /* second-hit */^M (gdb) FAIL: gdb.opt/solib-intra-step.exp: second-hit ... The problem is that the test-case expects to step past the retry line, which is optional. Fix this by make the retry line step optional, both for shlib_first and shlib_second. Any comments? Thanks, - Tom [gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 gdb/testsuite/ChangeLog: 2021-01-26 Tom de Vries <tdevries@suse.de> * gdb.opt/solib-intra-step.exp: Make step into retry line optional. --- gdb/testsuite/gdb.opt/solib-intra-step.exp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/testsuite/gdb.opt/solib-intra-step.exp b/gdb/testsuite/gdb.opt/solib-intra-step.exp index ad19895d890..13e0cf04c7a 100644 --- a/gdb/testsuite/gdb.opt/solib-intra-step.exp +++ b/gdb/testsuite/gdb.opt/solib-intra-step.exp @@ -63,7 +63,7 @@ gdb_test_multiple "step" $test { exp_continue } -re -wrap "get_pc_thunk.*" { - if { $state != 1 } { + if { $state != 0 && $state != 1 } { set state -1 } else { set state 2 @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { exp_continue } -re -wrap "get_pc_thunk.*" { - if { $state != 1 } { + if { $state != 0 && $state != 1 } { set state -1 } else { set state 2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 2021-01-26 18:03 [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 Tom de Vries @ 2021-01-28 15:03 ` Simon Marchi via Gdb-patches 2021-01-28 17:50 ` Tom de Vries 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi via Gdb-patches @ 2021-01-28 15:03 UTC (permalink / raw) To: Tom de Vries, gdb-patches On 2021-01-26 1:03 p.m., Tom de Vries wrote: > Hi, > > When running test-case gdb.opt/solib-intra-step.exp with target board > unix/-m32 and gcc-10, I run into: > ... > (gdb) step^M > __x86.get_pc_thunk.bx () at ../sysdeps/i386/crti.S:68^M > 68 ../sysdeps/i386/crti.S: No such file or directory.^M > (gdb) step^M > shlib_second (dummy=0) at solib-intra-step-lib.c:23^M > 23 abort (); /* second-hit */^M > (gdb) FAIL: gdb.opt/solib-intra-step.exp: second-hit > ... > > The problem is that the test-case expects to step past the retry line, > which is optional. > > Fix this by make the retry line step optional, both for shlib_first and > shlib_second. > > Any comments? > > Thanks, > - Tom > > [gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 > > gdb/testsuite/ChangeLog: > > 2021-01-26 Tom de Vries <tdevries@suse.de> > > * gdb.opt/solib-intra-step.exp: Make step into retry line optional. > > --- > gdb/testsuite/gdb.opt/solib-intra-step.exp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdb/testsuite/gdb.opt/solib-intra-step.exp b/gdb/testsuite/gdb.opt/solib-intra-step.exp > index ad19895d890..13e0cf04c7a 100644 > --- a/gdb/testsuite/gdb.opt/solib-intra-step.exp > +++ b/gdb/testsuite/gdb.opt/solib-intra-step.exp > @@ -63,7 +63,7 @@ gdb_test_multiple "step" $test { > exp_continue > } > -re -wrap "get_pc_thunk.*" { > - if { $state != 1 } { > + if { $state != 0 && $state != 1 } { > set state -1 > } else { > set state 2 > @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { > exp_continue > } > -re -wrap "get_pc_thunk.*" { > - if { $state != 1 } { > + if { $state != 0 && $state != 1 } { > set state -1 > } else { > set state 2 > I don't really understand what happens here, what state value means what. A bit of commenting would help. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 2021-01-28 15:03 ` Simon Marchi via Gdb-patches @ 2021-01-28 17:50 ` Tom de Vries 2021-01-28 18:04 ` Simon Marchi via Gdb-patches 0 siblings, 1 reply; 7+ messages in thread From: Tom de Vries @ 2021-01-28 17:50 UTC (permalink / raw) To: Simon Marchi, gdb-patches [-- Attachment #1: Type: text/plain, Size: 2269 bytes --] On 1/28/21 4:03 PM, Simon Marchi wrote: > > > On 2021-01-26 1:03 p.m., Tom de Vries wrote: >> Hi, >> >> When running test-case gdb.opt/solib-intra-step.exp with target board >> unix/-m32 and gcc-10, I run into: >> ... >> (gdb) step^M >> __x86.get_pc_thunk.bx () at ../sysdeps/i386/crti.S:68^M >> 68 ../sysdeps/i386/crti.S: No such file or directory.^M >> (gdb) step^M >> shlib_second (dummy=0) at solib-intra-step-lib.c:23^M >> 23 abort (); /* second-hit */^M >> (gdb) FAIL: gdb.opt/solib-intra-step.exp: second-hit >> ... >> >> The problem is that the test-case expects to step past the retry line, >> which is optional. >> >> Fix this by make the retry line step optional, both for shlib_first and >> shlib_second. >> >> Any comments? >> >> Thanks, >> - Tom >> >> [gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 >> >> gdb/testsuite/ChangeLog: >> >> 2021-01-26 Tom de Vries <tdevries@suse.de> >> >> * gdb.opt/solib-intra-step.exp: Make step into retry line optional. >> >> --- >> gdb/testsuite/gdb.opt/solib-intra-step.exp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/testsuite/gdb.opt/solib-intra-step.exp b/gdb/testsuite/gdb.opt/solib-intra-step.exp >> index ad19895d890..13e0cf04c7a 100644 >> --- a/gdb/testsuite/gdb.opt/solib-intra-step.exp >> +++ b/gdb/testsuite/gdb.opt/solib-intra-step.exp >> @@ -63,7 +63,7 @@ gdb_test_multiple "step" $test { >> exp_continue >> } >> -re -wrap "get_pc_thunk.*" { >> - if { $state != 1 } { >> + if { $state != 0 && $state != 1 } { >> set state -1 >> } else { >> set state 2 >> @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { >> exp_continue >> } >> -re -wrap "get_pc_thunk.*" { >> - if { $state != 1 } { >> + if { $state != 0 && $state != 1 } { >> set state -1 >> } else { >> set state 2 >> > > I don't really understand what happens here, what state value means what. > > A bit of commenting would help. I tried to add comments but didn't manage to come up with something sensible. Instead, I simplified gdb_test_multiple to just track the order of events, and then added a few asserts about order of events. I hope this clarifies what the test is trying to do. WDYT? Thanks, - Tom [-- Attachment #2: 0005-gdb-testsuite-Fix-gdb.opt-solib-intra-step.exp-with-m32-and-gcc-10.patch --] [-- Type: text/x-patch, Size: 3285 bytes --] [gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 When running test-case gdb.opt/solib-intra-step.exp with target board unix/-m32 and gcc-10, I run into: ... (gdb) step^M __x86.get_pc_thunk.bx () at ../sysdeps/i386/crti.S:68^M 68 ../sysdeps/i386/crti.S: No such file or directory.^M (gdb) step^M shlib_second (dummy=0) at solib-intra-step-lib.c:23^M 23 abort (); /* second-hit */^M (gdb) FAIL: gdb.opt/solib-intra-step.exp: second-hit ... The problem is that the test-case expects to step past the retry line, which is optional. Fix this by make the retry line step optional, both for shlib_first and shlib_second. gdb/testsuite/ChangeLog: 2021-01-26 Tom de Vries <tdevries@suse.de> * gdb.opt/solib-intra-step.exp: Make step into retry line optional. Rewrite state machine to make it clear what order conditions need checking. --- gdb/testsuite/gdb.opt/solib-intra-step.exp | 63 +++++++++++++++++++----------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/gdb/testsuite/gdb.opt/solib-intra-step.exp b/gdb/testsuite/gdb.opt/solib-intra-step.exp index ad19895d890..5c78e07772e 100644 --- a/gdb/testsuite/gdb.opt/solib-intra-step.exp +++ b/gdb/testsuite/gdb.opt/solib-intra-step.exp @@ -47,58 +47,77 @@ if ![runto_main] then { return 0 } +proc check_before { a b } { + eval global $a $b + eval set val_a $$a + eval set val_b $$b + if { $val_a == -1 || $val_b == -1 } { + return + } + gdb_assert { $val_a < $val_b } "$a before $b" +} + set test "first-hit" +set hit_state -1 +set retry_state -1 +set thunk_state -1 set state 0 gdb_test_multiple "step" $test { -re -wrap " first-hit .*" { - gdb_assert { $state != -1 } $test + set hit_state $state + pass $gdb_test_name } -re -wrap " first-retry .*" { - if { $state != 0 } { - set state -1 - } else { - set state 1 - } + set retry_state $state + incr state send_gdb "step\n" exp_continue } -re -wrap "get_pc_thunk.*" { - if { $state != 1 } { - set state -1 - } else { - set state 2 - } + set thunk_state $state + incr state send_gdb "step\n" exp_continue } } +with_test_prefix $test { + check_before retry_state hit_state + check_before thunk_state hit_state + check_before retry_state thunk_state +} + set test "second-hit" +set hit_state -1 +set retry_state -1 +set thunk_state -1 +set state 0 set state 0 gdb_test_multiple "step" $test { -re -wrap " second-hit .*" { - gdb_assert { $state != -1 } $test + set hit_state $state + pass $gdb_test_name } -re -wrap " second-retry .*" { - if { $state != 0 } { - set state -1 - } else { - set state 1 - } + set retry_state $state + incr state send_gdb "step\n" exp_continue } -re -wrap "get_pc_thunk.*" { - if { $state != 1 } { - set state -1 - } else { - set state 2 - } + set thunk_state $state + incr state send_gdb "step\n" exp_continue } } +with_test_prefix $test { + check_before retry_state hit_state + check_before thunk_state hit_state + check_before retry_state thunk_state +} + if ![runto_main] then { return 0 } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 2021-01-28 17:50 ` Tom de Vries @ 2021-01-28 18:04 ` Simon Marchi via Gdb-patches 2021-01-28 18:15 ` Tom de Vries 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi via Gdb-patches @ 2021-01-28 18:04 UTC (permalink / raw) To: Tom de Vries, gdb-patches >>> @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { >>> exp_continue >>> } >>> -re -wrap "get_pc_thunk.*" { >>> - if { $state != 1 } { >>> + if { $state != 0 && $state != 1 } { >>> set state -1 >>> } else { >>> set state 2 >>> >> >> I don't really understand what happens here, what state value means what. >> >> A bit of commenting would help. > > I tried to add comments but didn't manage to come up with something > sensible. > > Instead, I simplified gdb_test_multiple to just track the order of > events, and then added a few asserts about order of events. > > I hope this clarifies what the test is trying to do. WDYT? Hmm, it's still not clear to me what the intention of the test is. It's not clear what kind of good or bad behavior from GDB we are looking for. That intention needs to be recorded in a comment, otherwise, I can't tell if the code matches what we want (since I don't know what we want). I kind of understand now that we do a step, we want to get until the "first-hit" line (or "second-hit" in the second case), but it's possible that we land on intermediary states, which are acceptable. But there also seems to be an ordering component? Why is that important? Why don't we simply "exp_continue" when seeing "retry" or "get_pc_thunk", why bother recording anything? Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 2021-01-28 18:04 ` Simon Marchi via Gdb-patches @ 2021-01-28 18:15 ` Tom de Vries 2021-01-28 18:20 ` Simon Marchi via Gdb-patches 0 siblings, 1 reply; 7+ messages in thread From: Tom de Vries @ 2021-01-28 18:15 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 1/28/21 7:04 PM, Simon Marchi wrote: >>>> @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { >>>> exp_continue >>>> } >>>> -re -wrap "get_pc_thunk.*" { >>>> - if { $state != 1 } { >>>> + if { $state != 0 && $state != 1 } { >>>> set state -1 >>>> } else { >>>> set state 2 >>>> >>> >>> I don't really understand what happens here, what state value means what. >>> >>> A bit of commenting would help. >> >> I tried to add comments but didn't manage to come up with something >> sensible. >> >> Instead, I simplified gdb_test_multiple to just track the order of >> events, and then added a few asserts about order of events. >> >> I hope this clarifies what the test is trying to do. WDYT? > > Hmm, it's still not clear to me what the intention of the test is. It's > not clear what kind of good or bad behavior from GDB we are looking for. > That intention needs to be recorded in a comment, otherwise, I can't > tell if the code matches what we want (since I don't know what we want). > I kind of understand now that we do a step, we want to get until the > "first-hit" line (or "second-hit" in the second case), but it's possible > that we land on intermediary states, which are acceptable. But there > also seems to be an ordering component? Why is that important? Why > don't we simply "exp_continue" when seeing "retry" or "get_pc_thunk", > why bother recording anything? Ah, I see. Well, it's an attempt to be precise about what we accept in the test. Much in the same way that two subsequent gdb_test do that. But yeah, I don't think it's really important, so I can drop that part. Thanks, - Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 2021-01-28 18:15 ` Tom de Vries @ 2021-01-28 18:20 ` Simon Marchi via Gdb-patches 2021-01-29 10:44 ` Tom de Vries 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi via Gdb-patches @ 2021-01-28 18:20 UTC (permalink / raw) To: Tom de Vries, gdb-patches On 2021-01-28 1:15 p.m., Tom de Vries wrote:> On 1/28/21 7:04 PM, Simon Marchi wrote: >>>>> @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { >>>>> exp_continue >>>>> } >>>>> -re -wrap "get_pc_thunk.*" { >>>>> - if { $state != 1 } { >>>>> + if { $state != 0 && $state != 1 } { >>>>> set state -1 >>>>> } else { >>>>> set state 2 >>>>> >>>> >>>> I don't really understand what happens here, what state value means what. >>>> >>>> A bit of commenting would help. >>> >>> I tried to add comments but didn't manage to come up with something >>> sensible. >>> >>> Instead, I simplified gdb_test_multiple to just track the order of >>> events, and then added a few asserts about order of events. >>> >>> I hope this clarifies what the test is trying to do. WDYT? >> >> Hmm, it's still not clear to me what the intention of the test is. It's >> not clear what kind of good or bad behavior from GDB we are looking for. >> That intention needs to be recorded in a comment, otherwise, I can't >> tell if the code matches what we want (since I don't know what we want). >> I kind of understand now that we do a step, we want to get until the >> "first-hit" line (or "second-hit" in the second case), but it's possible >> that we land on intermediary states, which are acceptable. But there >> also seems to be an ordering component? Why is that important? Why >> don't we simply "exp_continue" when seeing "retry" or "get_pc_thunk", >> why bother recording anything? > > Ah, I see. > > Well, it's an attempt to be precise about what we accept in the test. > Much in the same way that two subsequent gdb_test do that. > > But yeah, I don't think it's really important, so I can drop that part. Thanks, and to be clear I don't have anything against what you suggest, the test was lacking proper documentation before you touched it. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 2021-01-28 18:20 ` Simon Marchi via Gdb-patches @ 2021-01-29 10:44 ` Tom de Vries 0 siblings, 0 replies; 7+ messages in thread From: Tom de Vries @ 2021-01-29 10:44 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 1/28/21 7:20 PM, Simon Marchi wrote: > On 2021-01-28 1:15 p.m., Tom de Vries wrote:> On 1/28/21 7:04 PM, Simon Marchi wrote: >>>>>> @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test { >>>>>> exp_continue >>>>>> } >>>>>> -re -wrap "get_pc_thunk.*" { >>>>>> - if { $state != 1 } { >>>>>> + if { $state != 0 && $state != 1 } { >>>>>> set state -1 >>>>>> } else { >>>>>> set state 2 >>>>>> >>>>> >>>>> I don't really understand what happens here, what state value means what. >>>>> >>>>> A bit of commenting would help. >>>> >>>> I tried to add comments but didn't manage to come up with something >>>> sensible. >>>> >>>> Instead, I simplified gdb_test_multiple to just track the order of >>>> events, and then added a few asserts about order of events. >>>> >>>> I hope this clarifies what the test is trying to do. WDYT? >>> >>> Hmm, it's still not clear to me what the intention of the test is. It's >>> not clear what kind of good or bad behavior from GDB we are looking for. >>> That intention needs to be recorded in a comment, otherwise, I can't >>> tell if the code matches what we want (since I don't know what we want). >>> I kind of understand now that we do a step, we want to get until the >>> "first-hit" line (or "second-hit" in the second case), but it's possible >>> that we land on intermediary states, which are acceptable. But there >>> also seems to be an ordering component? Why is that important? Why >>> don't we simply "exp_continue" when seeing "retry" or "get_pc_thunk", >>> why bother recording anything? >> >> Ah, I see. >> >> Well, it's an attempt to be precise about what we accept in the test. >> Much in the same way that two subsequent gdb_test do that. >> >> But yeah, I don't think it's really important, so I can drop that part. > > Thanks, and to be clear I don't have anything against what you suggest, > the test was lacking proper documentation before you touched it. Right, nevertheless I think it was good to go over this and try to make thing more readable. So, thanks for the review :) Committed with the state machine part removed. Thanks, - Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-29 10:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-26 18:03 [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10 Tom de Vries 2021-01-28 15:03 ` Simon Marchi via Gdb-patches 2021-01-28 17:50 ` Tom de Vries 2021-01-28 18:04 ` Simon Marchi via Gdb-patches 2021-01-28 18:15 ` Tom de Vries 2021-01-28 18:20 ` Simon Marchi via Gdb-patches 2021-01-29 10:44 ` Tom de Vries
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox