* [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support @ 2014-01-09 21:21 Sergio Durigan Junior 2014-01-10 1:23 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Sergio Durigan Junior @ 2014-01-09 21:21 UTC (permalink / raw) To: GDB Patches Hi, This patch is related to one of the "regressions" that I reported on the GDB 7.7 testing thread. While investigating the issue, I found that gdb.trace/mi-traceframe-changed.exp was running without actually checking if the target supported tracing or not. So I wrote this patch to fix the issue. It is now reporting the test as unsupported on s390x when run without gdbserver. When I run it with gdbserver, yet another error happens that prevents the testcase to test if the target supports tracing, but it is unrelated to the "regression" and happens with other testcases as well. OK to apply? -- Sergio gdb/testsuite 2014-01-09 Sergio Durigan Junior <sergiodj@redhat.com> * gdb.trace/mi-traceframe-changed.exp: Test if the target supports tracing before running the test. Use prepare_for_testing instead of gdb_compile. diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp index 4bcf379..b5380c5 100644 --- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp +++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp @@ -14,8 +14,6 @@ # along with this program. If not, see <http://www.gnu.org/licenses/>. load_lib trace-support.exp -load_lib mi-support.exp -set MIFLAGS "-i=mi" standard_testfile tfile.c set executable $testfile @@ -30,15 +28,30 @@ if {![is_remote host] && ![is_remote target]} { set purely_local 0 } -if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \ - executable \ - [list debug nowarnings \ - "additional_flags=-DTFILE_DIR=\"$tfile_dir\""]] \ - != "" } { - untested ${testfile}.exp - return -1 +if { [prepare_for_testing $testfile.exp $testfile $srcfile \ + [list debug nowarnings additional_flags=-DTFILE_DIR="${tfile_dir}"]] } { + untested $testfile.exp + return -1 } +# Testing if the target supports tracing. +if { ![runto_main] } { + fail "Cannot test if target supports tracing" + return -1 +} + +if { ![gdb_target_supports_trace] } { + unsupported "Current target does not support trace" + return -1 +} + +gdb_exit + +load_lib mi-support.exp +set MIFLAGS "-i=mi" + +mi_gdb_start + # Make sure we are starting fresh. remote_file host delete $tfile_basic remote_file target delete $tfile_basic ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-09 21:21 [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support Sergio Durigan Junior @ 2014-01-10 1:23 ` Yao Qi 2014-01-10 2:17 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-01-10 1:23 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: GDB Patches On 01/10/2014 05:21 AM, Sergio Durigan Junior wrote: > gdb.trace/mi-traceframe-changed.exp was running without actually > checking if the target supported tracing or not. So I wrote this patch > to fix the issue. The patch looks a right fix. Any tracepoint related tests should check whether target supports tracing or not at first. I did it through an oversight when I wrote this case. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 1:23 ` Yao Qi @ 2014-01-10 2:17 ` Yao Qi 2014-01-10 3:58 ` Sergio Durigan Junior 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-01-10 2:17 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: GDB Patches On 01/10/2014 09:22 AM, Yao Qi wrote: > On 01/10/2014 05:21 AM, Sergio Durigan Junior wrote: >> > gdb.trace/mi-traceframe-changed.exp was running without actually >> > checking if the target supported tracing or not. So I wrote this patch >> > to fix the issue. > The patch looks a right fix. Any tracepoint related tests should check > whether target supports tracing or not at first. > > I did it through an oversight when I wrote this case. Ah, I read the patch and mi-traceframe-change.exp again, and find my last comment is wrong. Sorry for the confusion. The first half of mi-traceframe-changed.exp (test_tfind_tfile) is to test "=traceframe-changed" on tfile target, which is produced by tfile.c. It is expected to run on native debugging. The second half of mi-traceframe-changed.exp (test_tfile_remote) is to test "=traceframe-changed" on remote target with a gdbserver connected. We can see mi-traceframe-changed.exp has already have the code to check target supports tracing or not. The root cause is that tfile.c isn't portable and unable to produce trace file properly for s390x. Search FIXME in it. We should skip test_find_tfile for targets other than x86-linux or x86_64-linux. Alternatively, we can modify tfile.c for s390x, but I think "generating tfile on a unsupported-tracing target" isn't useful. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 2:17 ` Yao Qi @ 2014-01-10 3:58 ` Sergio Durigan Junior 2014-01-10 10:57 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Sergio Durigan Junior @ 2014-01-10 3:58 UTC (permalink / raw) To: Yao Qi; +Cc: GDB Patches On Friday, January 10 2014, Yao Qi wrote: > Ah, I read the patch and mi-traceframe-change.exp again, and find my > last comment is wrong. Sorry for the confusion. Thank you! I was secretly wondering whether my patch was correct or not, because I thought the testcase was intended to be run partially on native debugging, as you explained. I was going to e-mail something about it tomorrow, but you were faster :-). > The first half of mi-traceframe-changed.exp (test_tfind_tfile) is to > test "=traceframe-changed" on tfile target, which is produced by > tfile.c. It is expected to run on native debugging. The second half > of mi-traceframe-changed.exp (test_tfile_remote) is to test > "=traceframe-changed" on remote target with a gdbserver connected. We > can see mi-traceframe-changed.exp has already have the code to check > target supports tracing or not. > > The root cause is that tfile.c isn't portable and unable to produce > trace file properly for s390x. Search FIXME in it. Indeed, thanks for pointing that. > We should skip test_find_tfile for targets other than x86-linux or > x86_64-linux. Alternatively, we can modify tfile.c for s390x, but I > think "generating tfile on a unsupported-tracing target" isn't useful. OK, WDYT of this version then? -- Sergio 2014-01-10 Sergio Durigan Junior <sergiodj@redhat.com> * gdb.trace/mi-traceframe-changed.exp: Only run test_find_tfile if the target is x86_64 or i*86 running the Linux kernel. diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp index 4bcf379..f2f5224 100644 --- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp +++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp @@ -88,7 +88,11 @@ proc test_tfind_tfile { } { } } -test_tfind_tfile +if { [istarget "x86_64-*-linux*"] || [istarget "i*86-*-linux*"] } { + # This test only works on x86_64 and i*86 targets running the Linux + # kernel. See the FIXME's on gdb.trace/tfile.c for more details. + test_tfind_tfile +} # Change to a different test case in order to run it on target, and get # several traceframes. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 3:58 ` Sergio Durigan Junior @ 2014-01-10 10:57 ` Pedro Alves 2014-01-10 17:31 ` Sergio Durigan Junior 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2014-01-10 10:57 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Yao Qi, GDB Patches On 01/10/2014 03:57 AM, Sergio Durigan Junior wrote: > On Friday, January 10 2014, Yao Qi wrote: > >> Ah, I read the patch and mi-traceframe-change.exp again, and find my >> last comment is wrong. Sorry for the confusion. > > Thank you! I was secretly wondering whether my patch was correct or > not, because I thought the testcase was intended to be run partially on > native debugging, as you explained. I was going to e-mail something > about it tomorrow, but you were faster :-). > >> The first half of mi-traceframe-changed.exp (test_tfind_tfile) is to >> test "=traceframe-changed" on tfile target, which is produced by >> tfile.c. It is expected to run on native debugging. The second half >> of mi-traceframe-changed.exp (test_tfile_remote) is to test >> "=traceframe-changed" on remote target with a gdbserver connected. We >> can see mi-traceframe-changed.exp has already have the code to check >> target supports tracing or not. >> >> The root cause is that tfile.c isn't portable and unable to produce >> trace file properly for s390x. Search FIXME in it. > > Indeed, thanks for pointing that. Is it the register block size? What's the actual error that causes / you're seeing? >> We should skip test_find_tfile for targets other than x86-linux or >> x86_64-linux. Alternatively, we can modify tfile.c for s390x, but I >> think "generating tfile on a unsupported-tracing target" isn't useful. > > OK, WDYT of this version then? -- Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 10:57 ` Pedro Alves @ 2014-01-10 17:31 ` Sergio Durigan Junior 2014-01-10 18:57 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Sergio Durigan Junior @ 2014-01-10 17:31 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On Friday, January 10 2014, Pedro Alves wrote: >>> The root cause is that tfile.c isn't portable and unable to produce >>> trace file properly for s390x. Search FIXME in it. >> >> Indeed, thanks for pointing that. > > Is it the register block size? What's the actual error that > causes / you're seeing? The error is: -trace-find frame-number 0^M &"PC not available\n"^M ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M (gdb) ^M FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: -trace-find frame-number 0 There is also another error before, but it was already failing for 7.6.2. -- Sergio ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 17:31 ` Sergio Durigan Junior @ 2014-01-10 18:57 ` Pedro Alves 2014-01-10 20:41 ` Sergio Durigan Junior 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2014-01-10 18:57 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Yao Qi, GDB Patches On 01/10/2014 05:30 PM, Sergio Durigan Junior wrote: > On Friday, January 10 2014, Pedro Alves wrote: > >>>> The root cause is that tfile.c isn't portable and unable to produce >>>> trace file properly for s390x. Search FIXME in it. >>> >>> Indeed, thanks for pointing that. >> >> Is it the register block size? What's the actual error that >> causes / you're seeing? > > The error is: > > -trace-find frame-number 0^M > &"PC not available\n"^M > ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M > (gdb) ^M > FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: -trace-find frame-number 0 Hmm, OK, I think I see. I don't think the register block size in the header needs to be correct for this test -- tfile.c portability sounds like a red herring to me. (I don't think 500 is correct for x86/x86_64 either?) There's no register block in the trace file anyway. Only memory blocks. tfile knows to infer the PC from the tracepoint's address if the PC wasn't collected (tfile_fetch_registers) but, (guessing) the issue is that PC register in s390 is a pseudo-register. tfile_fetch_registers guesses the PC from the tracepoint's address and stores it in the regcache, but when reading a (non-readonly) pseudo register from a regcache, we never use the stored value in the cache -- we always recompute it. In fact, writing the pseudo PC to the regcache in tfile_fetch_registers with regcache_raw_supply seems reachable when regnum==-1, and I'm surprised this isn't triggering the assertion that makes sure the regnum being written to is a raw register (as the regcache's buffer only holds the raw registers -- see regcache_xmalloc_1 and regcache_raw_write). When debugging a live traceframe, i.e., with gdbserver, it's gdbserver itself that does this same PC guessing. And of course, that also only works when the PC isn't a pseudo register, as the target backend only ever sees raw registers accesses. Seems like this PC guessing should move out of target_fetch_registers to somewhere higher level... I.e., this is not a bug specific to s390, but to the tracing/unavailable machinery itself, for all targets whose PC is a pseudo register. It's fine with me to skip the test on targets with pseudo register PCs for now, though probably we should record this info somewhere, probably in the code, like in the patch below. > There is also another error before, but it was already failing for > 7.6.2. Hard to say anything about it if you don't show it. :-) ------- Subject: [PATCH] tfile: Don't infer the PC from the tracepoint if the PC is a pseudo-register. This PC guessing can't work when the PC is a pseudo-register. Pseudo-register values don't end up stored in the regcache, they're always recomputed. And, it's actually wrong to try to write a pseudo-register with regcache_raw_supply. Skip it and add a comment. gdb/ 2014-01-10 Pedro Alves <palves@redhat.com> * tracepoint.c (tfile_fetch_registers): Don't infer the PC from the tracepoint if the PC is a pseudo-register. --- gdb/tracepoint.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c index 582c284..4b47282 100644 --- a/gdb/tracepoint.c +++ b/gdb/tracepoint.c @@ -5060,7 +5060,17 @@ tfile_fetch_registers (struct target_ops *ops, /* We can often usefully guess that the PC is going to be the same as the address of the tracepoint. */ pc_regno = gdbarch_pc_regnum (gdbarch); - if (pc_regno >= 0 && (regno == -1 || regno == pc_regno)) + + /* XXX This guessing code below only works if the PC register isn't + a pseudo-register. The value of a pseudo-register isn't stored + in the (non-readonly) regcache -- instead it's recomputed + (probably from some other cached raw register) whenever the + register is read. This guesswork should probably move to some + higher layer. */ + if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch)) + return; + + if (regno == -1 || regno == pc_regno) { struct tracepoint *tp = get_tracepoint (tracepoint_number); -- 1.7.11.7 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 18:57 ` Pedro Alves @ 2014-01-10 20:41 ` Sergio Durigan Junior 2014-01-13 17:12 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Sergio Durigan Junior @ 2014-01-10 20:41 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On Friday, January 10 2014, Pedro Alves wrote: > Hmm, OK, I think I see. I don't think the register block > size in the header needs to be correct for this test -- tfile.c > portability sounds like a red herring to me. (I don't think 500 > is correct for x86/x86_64 either?) There's no register block > in the trace file anyway. Only memory blocks. > > tfile knows to infer the PC from the tracepoint's address if > the PC wasn't collected (tfile_fetch_registers) but, > (guessing) the issue is that PC register in s390 is a > pseudo-register. tfile_fetch_registers guesses the PC from the > tracepoint's address and stores it in the regcache, but > when reading a (non-readonly) pseudo register from a regcache, > we never use the stored value in the cache -- we always > recompute it. In fact, writing the pseudo PC to the regcache > in tfile_fetch_registers with regcache_raw_supply seems reachable > when regnum==-1, and I'm surprised this isn't triggering the assertion > that makes sure the regnum being written to is a raw register > (as the regcache's buffer only holds the raw registers -- see > regcache_xmalloc_1 and regcache_raw_write). Thanks for the investigation. By debugging it a little more, I see that we don't write the pseudo PC to the regcache. You are probably talking about this loop: /* We get here if no register data has been found. Mark registers as unavailable. */ for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) regcache_raw_supply (regcache, regn, NULL); The pseudo PC register number on s390x is 90, and "gdbarch_num_regs (gdbarch)" returns exactly 90. What happens is that when one requests the pseudo PC on s390x, it reads S390_PSWM_REGNUM, which is 1: static enum register_status s390_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache, int regnum, gdb_byte *buf) { struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); int regsize = register_size (gdbarch, regnum); ULONGEST val; if (regnum == tdep->pc_regnum) { enum register_status status; status = regcache_raw_read_unsigned (regcache, S390_PSWA_REGNUM, &val); Then, this check on tfile_fetch_registers evaluates to false: /* We can often usefully guess that the PC is going to be the same as the address of the tracepoint. */ pc_regno = gdbarch_pc_regnum (gdbarch); if (pc_regno >= 0 && (regno == -1 || regno == pc_regno)) Because regno == 1 and pc_regno == 90. Maybe you knew all that, but I thought it would be better to explicitly mention. > When debugging a live traceframe, i.e., with gdbserver, > it's gdbserver itself that does this same PC guessing. And > of course, that also only works when the PC isn't a pseudo > register, as the target backend only ever sees raw registers > accesses. > > Seems like this PC guessing should move out of > target_fetch_registers to somewhere higher level... > > I.e., this is not a bug specific to s390, but to the > tracing/unavailable machinery itself, for all targets > whose PC is a pseudo register. I see. I haven't had the chance to test on other targets where PC is a pseud register. > It's fine with me to skip the test on targets with pseudo > register PCs for now, though probably we should record this > info somewhere, probably in the code, like in the patch below. Nice, I like the comments, though your patch doesn't really change anything for s390x because of what I explained above (regno == 1 and pc_regno == 90), but I like to make things more explicit and your patch does that. I can push both our patches if you wish, btw. >> There is also another error before, but it was already failing for >> 7.6.2. > > Hard to say anything about it if you don't show it. :-) The same error: &"tfind 0\n"^M &"PC not available\n"^M ~"Found trace frame 0, tracepoint 1\n"^M ~"#-1 <unavailable> in ?? ()\n"^M ^done^M (gdb) ^M FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: tfind 0 again So I think the same explanation is valid. > ------- > Subject: [PATCH] tfile: Don't infer the PC from the tracepoint if the PC is a > pseudo-register. > > This PC guessing can't work when the PC is a pseudo-register. > Pseudo-register values don't end up stored in the regcache, they're > always recomputed. And, it's actually wrong to try to write a > pseudo-register with regcache_raw_supply. Skip it and add a comment. > > gdb/ > 2014-01-10 Pedro Alves <palves@redhat.com> > > * tracepoint.c (tfile_fetch_registers): Don't infer the PC from > the tracepoint if the PC is a pseudo-register. > --- > gdb/tracepoint.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c > index 582c284..4b47282 100644 > --- a/gdb/tracepoint.c > +++ b/gdb/tracepoint.c > @@ -5060,7 +5060,17 @@ tfile_fetch_registers (struct target_ops *ops, > /* We can often usefully guess that the PC is going to be the same > as the address of the tracepoint. */ > pc_regno = gdbarch_pc_regnum (gdbarch); > - if (pc_regno >= 0 && (regno == -1 || regno == pc_regno)) > + > + /* XXX This guessing code below only works if the PC register isn't > + a pseudo-register. The value of a pseudo-register isn't stored > + in the (non-readonly) regcache -- instead it's recomputed > + (probably from some other cached raw register) whenever the > + register is read. This guesswork should probably move to some > + higher layer. */ > + if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch)) > + return; > + > + if (regno == -1 || regno == pc_regno) > { > struct tracepoint *tp = get_tracepoint (tracepoint_number); > > -- > 1.7.11.7 -- Sergio ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-10 20:41 ` Sergio Durigan Junior @ 2014-01-13 17:12 ` Pedro Alves 2014-01-15 20:37 ` Sergio Durigan Junior 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2014-01-13 17:12 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Yao Qi, GDB Patches On 01/10/2014 08:41 PM, Sergio Durigan Junior wrote: > On Friday, January 10 2014, Pedro Alves wrote: > >> Hmm, OK, I think I see. I don't think the register block >> size in the header needs to be correct for this test -- tfile.c >> portability sounds like a red herring to me. (I don't think 500 >> is correct for x86/x86_64 either?) There's no register block >> in the trace file anyway. Only memory blocks. >> >> tfile knows to infer the PC from the tracepoint's address if >> the PC wasn't collected (tfile_fetch_registers) but, >> (guessing) the issue is that PC register in s390 is a >> pseudo-register. tfile_fetch_registers guesses the PC from the >> tracepoint's address and stores it in the regcache, but >> when reading a (non-readonly) pseudo register from a regcache, >> we never use the stored value in the cache -- we always >> recompute it. In fact, writing the pseudo PC to the regcache >> in tfile_fetch_registers with regcache_raw_supply seems reachable >> when regnum==-1, and I'm surprised this isn't triggering the assertion >> that makes sure the regnum being written to is a raw register >> (as the regcache's buffer only holds the raw registers -- see >> regcache_xmalloc_1 and regcache_raw_write). > > Thanks for the investigation. > > By debugging it a little more, I see that we don't write the pseudo PC > to the regcache. You are probably talking about this loop: > > /* We get here if no register data has been found. Mark registers > as unavailable. */ > for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) > regcache_raw_supply (regcache, regn, NULL); No. Below that, when regno==-1. Note my patch adds an early return _after_ that loop. /* We get here if no register data has been found. Mark registers as unavailable. */ for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) regcache_raw_supply (regcache, regn, NULL); /* We can often usefully guess that the PC is going to be the same as the address of the tracepoint. */ pc_regno = gdbarch_pc_regnum (gdbarch); if (pc_regno >= 0 && (regno == -1 || regno == pc_regno)) ^^^^^^^^^^^ { struct tracepoint *tp = get_tracepoint (tracepoint_number); if (tp && tp->base.loc) { /* But don't try to guess if tracepoint is multi-location... */ if (tp->base.loc->next) { warning (_("Tracepoint %d has multiple " "locations, cannot infer $pc"), tp->base.number); return; } /* ... or does while-stepping. */ if (tp->step_count > 0) { warning (_("Tracepoint %d does while-stepping, " "cannot infer $pc"), tp->base.number); return; } store_unsigned_integer (regs, register_size (gdbarch, pc_regno), gdbarch_byte_order (gdbarch), tp->base.loc->address); regcache_raw_supply (regcache, pc_regno, regs); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } } } regno==-1 means "retrieve all raw registers". It just seems like no such call happens to trigger in this case. > The pseudo PC register number on s390x is 90, and "gdbarch_num_regs > (gdbarch)" returns exactly 90. > > What happens is that when one requests the pseudo PC on s390x, it reads > S390_PSWM_REGNUM, which is 1: ... > Because regno == 1 and pc_regno == 90. Maybe you knew all that, but I > thought it would be better to explicitly mention. Right. >> I.e., this is not a bug specific to s390, but to the >> tracing/unavailable machinery itself, for all targets >> whose PC is a pseudo register. > > I see. I haven't had the chance to test on other targets where PC is a > pseud register. > >> It's fine with me to skip the test on targets with pseudo >> register PCs for now, though probably we should record this >> info somewhere, probably in the code, like in the patch below. > > Nice, I like the comments, though your patch doesn't really change > anything for s390x because of what I explained above (regno == 1 and > pc_regno == 90), but I like to make things more explicit and your patch > does that. > > I can push both our patches if you wish, btw. I've pushed mine in now. I'd really prefer to avoid hardcoding knowledge of which targets support tracing in the testsuite. Exposing the host-side bits to all targets helps identify design bugs, just like this very case of pseudo-register PCs. But even if GDB doesn't know how to infer the value of PC, saying the current frame is level -1 is a bug: ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M ^^^^^^^^^ that's of course, the sentinel frame peeking through. This is caused by the s390's heuristic unwinder accepting the frame (the fallback heuristic unwinders _always_ accept the frame), but then unwind->this_id method throws that "PC not available\n" error. IOW, the issue is in the target's unwinding mechanism not having been adjusted to handle unavailable register values gracefully (which can happen with e.g., a trimmed core file too). Could you please try the patch below? You should see something like: (gdb) tfind Found trace frame 0, tracepoint 1 #0 <unavailable> in ?? () That is, frame #0 instead of -1. This is just the minimal necessary for <unavailable> frames. We could get better info out of "info frame" (this will show "outermost"), but this would still be necessary. I believe mi-traceframe-changed.exp should pass with this patch. --- gdb/s390-linux-tdep.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c index b75c86a..8b71e78 100644 --- a/gdb/s390-linux-tdep.c +++ b/gdb/s390-linux-tdep.c @@ -2039,7 +2039,9 @@ static struct s390_unwind_cache * s390_frame_unwind_cache (struct frame_info *this_frame, void **this_prologue_cache) { + volatile struct gdb_exception ex; struct s390_unwind_cache *info; + if (*this_prologue_cache) return *this_prologue_cache; @@ -2050,10 +2052,15 @@ s390_frame_unwind_cache (struct frame_info *this_frame, info->frame_base = -1; info->local_base = -1; - /* Try to use prologue analysis to fill the unwind cache. - If this fails, fall back to reading the stack backchain. */ - if (!s390_prologue_frame_unwind_cache (this_frame, info)) - s390_backchain_frame_unwind_cache (this_frame, info); + TRY_CATCH (ex, RETURN_MASK_ERROR) + { + /* Try to use prologue analysis to fill the unwind cache. + If this fails, fall back to reading the stack backchain. */ + if (!s390_prologue_frame_unwind_cache (this_frame, info)) + s390_backchain_frame_unwind_cache (this_frame, info); + } + if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR) + throw_exception (ex); return info; } -- 1.7.11.7 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-13 17:12 ` Pedro Alves @ 2014-01-15 20:37 ` Sergio Durigan Junior 2014-01-16 18:20 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Sergio Durigan Junior @ 2014-01-15 20:37 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, GDB Patches On Monday, January 13 2014, Pedro Alves wrote: > On 01/10/2014 08:41 PM, Sergio Durigan Junior wrote: >> By debugging it a little more, I see that we don't write the pseudo PC >> to the regcache. You are probably talking about this loop: >> >> /* We get here if no register data has been found. Mark registers >> as unavailable. */ >> for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) >> regcache_raw_supply (regcache, regn, NULL); > > No. Below that, when regno==-1. Note my patch adds an early > return _after_ that loop. > > /* We get here if no register data has been found. Mark registers > as unavailable. */ > for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) > regcache_raw_supply (regcache, regn, NULL); > > /* We can often usefully guess that the PC is going to be the same > as the address of the tracepoint. */ > pc_regno = gdbarch_pc_regnum (gdbarch); > if (pc_regno >= 0 && (regno == -1 || regno == pc_regno)) > ^^^^^^^^^^^ > { > struct tracepoint *tp = get_tracepoint (tracepoint_number); > > if (tp && tp->base.loc) > { > /* But don't try to guess if tracepoint is multi-location... */ > if (tp->base.loc->next) > { > warning (_("Tracepoint %d has multiple " > "locations, cannot infer $pc"), > tp->base.number); > return; > } > /* ... or does while-stepping. */ > if (tp->step_count > 0) > { > warning (_("Tracepoint %d does while-stepping, " > "cannot infer $pc"), > tp->base.number); > return; > } > > store_unsigned_integer (regs, register_size (gdbarch, pc_regno), > gdbarch_byte_order (gdbarch), > tp->base.loc->address); > regcache_raw_supply (regcache, pc_regno, regs); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > } > } > } > > > regno==-1 means "retrieve all raw registers". It just > seems like no such call happens to trigger in this case. OK. >>> It's fine with me to skip the test on targets with pseudo >>> register PCs for now, though probably we should record this >>> info somewhere, probably in the code, like in the patch below. >> >> Nice, I like the comments, though your patch doesn't really change >> anything for s390x because of what I explained above (regno == 1 and >> pc_regno == 90), but I like to make things more explicit and your patch >> does that. >> >> I can push both our patches if you wish, btw. > > I've pushed mine in now. I'd really prefer to avoid > hardcoding knowledge of which targets support tracing > in the testsuite. Exposing the host-side bits to > all targets helps identify design bugs, just like > this very case of pseudo-register PCs. OK, makes sense. > But even if GDB doesn't know how to infer the value > of PC, saying the current frame is level -1 is a bug: > > ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M > ^^^^^^^^^ > > that's of course, the sentinel frame peeking through. > This is caused by the s390's heuristic unwinder accepting the > frame (the fallback heuristic unwinders _always_ accept the > frame), but then unwind->this_id method throws that > "PC not available\n" error. > IOW, the issue is in the target's unwinding mechanism not > having been adjusted to handle unavailable register > values gracefully (which can happen with e.g., a trimmed > core file too). Could you please try the patch below? > You should see something like: > > (gdb) tfind > Found trace frame 0, tracepoint 1 > #0 <unavailable> in ?? () > > That is, frame #0 instead of -1. > > This is just the minimal necessary for > <unavailable> frames. We could get better info out > of "info frame" (this will show "outermost"), but > this would still be necessary. I believe mi-traceframe-changed.exp > should pass with this patch. Yes, confirming that it passes with the patch. Sorry for taking long to test, I had issues trying to get another s390x machine to test it. Thanks for the patch and the further investigation. > --- > gdb/s390-linux-tdep.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c > index b75c86a..8b71e78 100644 > --- a/gdb/s390-linux-tdep.c > +++ b/gdb/s390-linux-tdep.c > @@ -2039,7 +2039,9 @@ static struct s390_unwind_cache * > s390_frame_unwind_cache (struct frame_info *this_frame, > void **this_prologue_cache) > { > + volatile struct gdb_exception ex; > struct s390_unwind_cache *info; > + > if (*this_prologue_cache) > return *this_prologue_cache; > > @@ -2050,10 +2052,15 @@ s390_frame_unwind_cache (struct frame_info *this_frame, > info->frame_base = -1; > info->local_base = -1; > > - /* Try to use prologue analysis to fill the unwind cache. > - If this fails, fall back to reading the stack backchain. */ > - if (!s390_prologue_frame_unwind_cache (this_frame, info)) > - s390_backchain_frame_unwind_cache (this_frame, info); > + TRY_CATCH (ex, RETURN_MASK_ERROR) > + { > + /* Try to use prologue analysis to fill the unwind cache. > + If this fails, fall back to reading the stack backchain. */ > + if (!s390_prologue_frame_unwind_cache (this_frame, info)) > + s390_backchain_frame_unwind_cache (this_frame, info); > + } > + if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR) > + throw_exception (ex); > > return info; > } > -- > 1.7.11.7 -- Sergio ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support 2014-01-15 20:37 ` Sergio Durigan Junior @ 2014-01-16 18:20 ` Pedro Alves 0 siblings, 0 replies; 11+ messages in thread From: Pedro Alves @ 2014-01-16 18:20 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Yao Qi, GDB Patches On 01/15/2014 08:37 PM, Sergio Durigan Junior wrote: > Yes, confirming that it passes with the patch. Sorry for taking long to > test, I had issues trying to get another s390x machine to test it. > > Thanks for the patch and the further investigation. Thank you. Patch is now pushed, as below. ------ Fix gdb.trace/mi-traceframe-changed.exp on s390. The test fails on s390 with: -trace-find frame-number 0^M &"PC not available\n"^M ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M (gdb) ^M FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: -trace-find frame-number 0 tfile knows to infer the PC from the tracepoint's address if the PC wasn't collected (tfile_fetch_registers) but, that only works on targets whose PC register is a raw register, and on s390, the PC register is a pseudo register. But even if GDB doesn't know how to infer the value of PC, saying the current frame is level -1 is a bug: ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M ^^^^^^^^^ '-1' is the level of the sentinel frame, which should never be visible. This is caused by the s390's heuristic unwinder accepting the frame (the fallback heuristic unwinders _always_ accept the frame), but then the unwind->this_id method throws that "PC not available\n" error. IOW, the s390's heuristic unwinder was never adjusted to handle unavailable register values gracefully, which can happen with e.g., a trimmed core file too. This is just the minimal necessary for <unavailable> frames, which at least gets us: (gdb) tfind Found trace frame 0, tracepoint 1 #0 <unavailable> in ?? () That is, frame #0 instead of -1. We could get better info out of "info frame" (this patch makes us show "outermost"), but this change would still be necessary. gdb/ 2014-01-16 Pedro Alves <palves@redhat.com> * s390-linux-tdep.c (s390_frame_unwind_cache): Swallow NOT_AVAILABLE_ERROR errors while parsing the prologue or reading the backchain. --- gdb/ChangeLog | 6 ++++++ gdb/s390-linux-tdep.c | 15 +++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index f614f1b..6eef81a 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +2014-01-16 Pedro Alves <palves@redhat.com> + + * s390-linux-tdep.c (s390_frame_unwind_cache): Swallow + NOT_AVAILABLE_ERROR errors while parsing the prologue or reading + the backchain. + 2014-01-16 Doug Evans <dje@google.com> * dwarf2read.c (open_and_init_dwp_file): Fix typo in comment. diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c index b75c86a..8b71e78 100644 --- a/gdb/s390-linux-tdep.c +++ b/gdb/s390-linux-tdep.c @@ -2039,7 +2039,9 @@ static struct s390_unwind_cache * s390_frame_unwind_cache (struct frame_info *this_frame, void **this_prologue_cache) { + volatile struct gdb_exception ex; struct s390_unwind_cache *info; + if (*this_prologue_cache) return *this_prologue_cache; @@ -2050,10 +2052,15 @@ s390_frame_unwind_cache (struct frame_info *this_frame, info->frame_base = -1; info->local_base = -1; - /* Try to use prologue analysis to fill the unwind cache. - If this fails, fall back to reading the stack backchain. */ - if (!s390_prologue_frame_unwind_cache (this_frame, info)) - s390_backchain_frame_unwind_cache (this_frame, info); + TRY_CATCH (ex, RETURN_MASK_ERROR) + { + /* Try to use prologue analysis to fill the unwind cache. + If this fails, fall back to reading the stack backchain. */ + if (!s390_prologue_frame_unwind_cache (this_frame, info)) + s390_backchain_frame_unwind_cache (this_frame, info); + } + if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR) + throw_exception (ex); return info; } -- 1.7.11.7 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-01-16 18:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-09 21:21 [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support Sergio Durigan Junior 2014-01-10 1:23 ` Yao Qi 2014-01-10 2:17 ` Yao Qi 2014-01-10 3:58 ` Sergio Durigan Junior 2014-01-10 10:57 ` Pedro Alves 2014-01-10 17:31 ` Sergio Durigan Junior 2014-01-10 18:57 ` Pedro Alves 2014-01-10 20:41 ` Sergio Durigan Junior 2014-01-13 17:12 ` Pedro Alves 2014-01-15 20:37 ` Sergio Durigan Junior 2014-01-16 18:20 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).