On 03/06/2012 01:38 PM, Yao Qi wrote: > Hi, > Bug 13392 is caused by incorrect JMP instruction when offset exceeds > integer limit. This patch is to check the offset first to see if it is > still within the integer range. Return error if offset exceeds. > > The changes to test cases is a little mechanical, simply remove kfail, > and check the error messages of downloading tracepoints. The test > result changes like this, > > -KFAIL: gdb.trace/change-loc.exp: 1 ftrace: continue to marker 2 (PRMS: > gdb/13392) > -PASS: gdb.trace/change-loc.exp: 2 ftrace: tstart > -KFAIL: gdb.trace/change-loc.exp: 2 ftrace: continue to marker 1 (PRMS: > gdb/13392) > -PASS: gdb.trace/pending.exp: ftrace works: start trace experiment > -KFAIL: gdb.trace/pending.exp: ftrace works: continue to marker (PRMS: > gdb/13392) > -KFAIL: gdb.trace/pending.exp: ftrace resolved_in_trace: continue to > marker 2 (PRMS: gdb/13392) > -KFAIL: gdb.trace/pending.exp: ftrace action_resolved: continue to > marker 2 (PRMS: gdb/13392) > -KFAIL: gdb.trace/pending.exp: ftrace installed_in_trace: continue to > marker 2 (PRMS: gdb/13392) > > Tested on x86_64-linux, OK to apply? > > -- Yao (齐尧) > > > 0001-fix-pr13392-check-offset-of-jmp-insn.patch > > > 2012-03-06 Yao Qi > > Fix PR server/13392. > * linux-x86-low.c (amd64_install_fast_tracepoint_jump_pad): Check > offset of JMP insn. > * tracepoint.c (install_fast_tracepoint): Fill in ERRBUF when > gets error. > > 2012-03-06 Yao Qi > > Fix PR server/13392. > * gdb.trace/change-loc.exp (tracepoint_change_loc_1): Remove kfail. > (tracepoint_change_loc_2): Remove kfail. Return if failed to > download tracepoints. > * gdb.trace/pending.exp (pending_tracepoint_works): Likwise. > (pending_tracepoint_resolved_during_trace): Likewise. > (pending_tracepoint_installed_during_trace): Likewise. > (pending_tracepoint_with_action_resolved): Likewise. > --- > gdb/gdbserver/linux-x86-low.c | 25 +++++++++++++- > gdb/gdbserver/tracepoint.c | 5 ++- > gdb/testsuite/gdb.trace/change-loc.exp | 38 +++++++++++++++----- > gdb/testsuite/gdb.trace/pending.exp | 57 ++++++++++++++++++++++++-------- > 4 files changed, 98 insertions(+), 27 deletions(-) > > diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c > index 58aaf9a..4647f5a 100644 > --- a/gdb/gdbserver/linux-x86-low.c > +++ b/gdb/gdbserver/linux-x86-low.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > #include "server.h" > #include "linux-low.h" > #include "i387-fp.h" > @@ -1200,6 +1201,8 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr, > { > unsigned char buf[40]; > int i, offset; > + int64_t loffset; > + > CORE_ADDR buildaddr = *jump_entry; > > /* Build the jump pad. */ > @@ -1323,7 +1326,16 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr, > *adjusted_insn_addr_end = buildaddr; > > /* Finally, write a jump back to the program. */ > - offset = (tpaddr + orig_size) - (buildaddr + sizeof (jump_insn)); > + > + loffset = (tpaddr + orig_size) - (buildaddr + sizeof (jump_insn)); > + if (loffset > INT_MAX || loffset < INT_MIN) > + { > + warning ("Cannot handle jump of offset 0x%" PRIx64 " > 4-byte\n", > + loffset); We should send an error back to GDB with "E." instead of printing something to gdbserver's console, and leaving the user with a generic and unhelful error. "4-byte" isn't strictly correct, as this is a signed offset, and I think we can be a bit more clear. So we end up with: sprintf (err, "E.Jump back from jump pad too far from tracepoint " "(offset 0x%" PRIx64 " > int32).", loffset); > + return 1; > + } > + > + offset = (int) loffset; > memcpy (buf, jump_insn, sizeof (jump_insn)); > memcpy (buf + 1, &offset, 4); > append_insns (&buildaddr, sizeof (jump_insn), buf); > @@ -1332,7 +1344,16 @@ amd64_install_fast_tracepoint_jump_pad (CORE_ADDR tpoint, CORE_ADDR tpaddr, > is always done last (by our caller actually), so that we can > install fast tracepoints with threads running. This relies on > the agent's atomic write support. */ > - offset = *jump_entry - (tpaddr + sizeof (jump_insn)); > + loffset = *jump_entry - (tpaddr + sizeof (jump_insn)); > + if (loffset > INT_MAX || loffset < INT_MIN) > + { > + warning ("Cannot handle jump of offset 0x%" PRIx64 " > 4-byte\n", > + loffset); sprintf (err, "E.Jump pad too far from tracepoint " "(offset 0x%" PRIx64 " > int32).", loffset); > + return 1; > + } > + > + offset = (int) loffset; > + > memcpy (buf, jump_insn, sizeof (jump_insn)); > memcpy (buf + 1, &offset, 4); > memcpy (jjump_pad_insn, buf, sizeof (jump_insn)); > diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c > index 21e58ff..1f85f9a 100644 > --- a/gdb/gdbserver/tracepoint.c > +++ b/gdb/gdbserver/tracepoint.c > @@ -2853,7 +2853,10 @@ install_fast_tracepoint (struct tracepoint *tpoint, char *errbuf) > errbuf); > > if (err) > - return 1; > + { > + xsnprintf (errbuf, 50, "E.Failed to install fast tracepoint jumppad"); This would overwrite any error the install_fast_tracepoint target hook wrote to errbuf (can be seen quoted just above). > + return 1; > + } > > /* Wire it in. */ > tpoint->handle = set_fast_tracepoint_jump (tpoint->address, fjump, > diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp > index d6062fc..7f2fd91 100644 > --- a/gdb/testsuite/gdb.trace/change-loc.exp > +++ b/gdb/testsuite/gdb.trace/change-loc.exp > @@ -114,8 +114,16 @@ proc tracepoint_change_loc_1 { trace_type } { with_test_prefix "1 $trace_type" { > pass "continue to marker 2" > } > -re ".*$gdb_prompt $" { > - kfail "gdb/13392" "continue to marker 2" > - return > + > + # It is possible to unable to create a jumppad for a fast tracepoint > + # due to the limitation of instructions. We simply return to skip > + # the rest of tests. I understand what you mean, but this needs copy/editing. "possible to unable to create" doesn't parse. But, I think we can do better when the target sends back the real error to GDB. Since that essentially means every hunk in your patch needs redoing, I went ahead and did the changes. See patches attached. > + if [string equal $trace_type "ftrace"] { > + return > + } else { > + fail "continue to marker 2" > + } > + > } > } > # tracepoint has three locations after shlib change-loc-2 is loaded. > @@ -198,19 +206,29 @@ proc tracepoint_change_loc_2 { trace_type } { with_test_prefix "2 $trace_type" { > "breakpoint on marker" > > # tracepoint with two locations will be downloaded and installed. > - gdb_test_no_output "tstart" > - > - gdb_test_multiple "continue" "continue to marker 1" { > - -re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { > - pass "continue to marker 1" > - } > + gdb_test_multiple "tstart" "tstart" { > + -re "^tstart\r\n$gdb_prompt $" { > + if ![string match "" "tstart"] then { ??? When can [string match "" "tstart"] ever return something other than 0 ? > + pass "tstart" > + } > + } > -re ".*$gdb_prompt $" { > - kfail "gdb/13392" "continue to marker 1" > - return > + # It is possible to unable to create a jumppad for a fast tracepoint > + # due to the limitation of instructions. We simply return to skip > + # the rest of tests. > + > + if [string equal $trace_type "ftrace"] { > + return > + } else { > + fail "tstart" > + } Hmm, what does that error on "continue" mean? With just your patch, I see in gdb.log: continue Continuing. Target returns error code '01'. That's another generic error that could be made to return "E." instead. So I enabled remote debug, and we see: continue Continuing. Sending packet: $QPassSignals:#f3...Packet received: OK Sending packet: $vCont;s:p3e0d.3e0d#e8...Packet received: T0506:80d6ffffff7f0000;07:58d6ffffff7f0000;10:6a07400000000000;thread:p3e0d.3e0d;core:3; Sending packet: $qTStatus#49...Packet received: T1;tnotrun:0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:001331049107617717;stoptime:00000 0000;username::;notes:: Sending packet: $qTMinFTPILen#3b...Packet received: 5 Sending packet: $m7ffff7bf05c6,1#96...Packet received: e8 Sending packet: $m7ffff7bf05c7,4#9a...Packet received: f1ffffff Sending packet: $QTDP:4:00007ffff7bf05c6:E:0:0:F5#75...Packet received: E.duplicate Target returns error code '01'. So we're failing in QTDP handling, as expected. But where exactly? It's not when trying to install the fast tracepoint to target memory. It's here: --- c/gdb/gdbserver/tracepoint.c +++ w/gdb/gdbserver/tracepoint.c @@ -2307,7 +2307,8 @@ cmd_qtdp (char *own_buf) trace_debug ("Tracepoint error: tracepoint %d" " at 0x%s already exists", (int) num, paddress (addr)); - write_enn (own_buf); + sprintf (own_buf, "E.duplicate"); + //write_enn (own_buf); return; } continue Continuing. Sending packet: $QPassSignals:#f3...Packet received: OK Sending packet: $vCont;s:p3e0d.3e0d#e8...Packet received: T0506:80d6ffffff7f0000;07:58d6ffffff7f0000;10:6a07400000000000;thread:p3e0d.3e0d;core:3; Sending packet: $qTStatus#49...Packet received: T1;tnotrun:0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:001331049107617717;stoptime:00000 0000;username::;notes:: Sending packet: $qTMinFTPILen#3b...Packet received: 5 Sending packet: $m7ffff7bf05c6,1#96...Packet received: e8 Sending packet: $m7ffff7bf05c7,4#9a...Packet received: f1ffffff Sending packet: $QTDP:4:00007ffff7bf05c6:E:0:0:F5#75...Packet received: E.duplicate Target returns error code '.duplicate'. So something doesn't look exactly right. The target knows about this fast tracepoint already, but it isn't really installed yet (so we have 3 levels of "inserted", not just 2: never downloaded; downloaded; downloaded and installed). Maybe GDB should have disabled the tracepoint on "tstart", or deleted it from the target, or... Anyway, that'll need some further thinking. In the mean time, I've made gdbserver send a clearer error back to GDB, and we end up with: Target returns error code '.Tracepoint 2 at 0x7ffff67c2579 already exists'. > # tracepoint has three locations after shlib change-loc-2 is loaded. > diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp > index 017aea9..1391d42 100644 > --- a/gdb/testsuite/gdb.trace/pending.exp > +++ b/gdb/testsuite/gdb.trace/pending.exp ... more of the same. Please find attached the interdiff (my changes on top of yours), and then the final patch (both patches merged). WDYT? -- Pedro Alves