From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19811 invoked by alias); 7 Mar 2012 08:25:05 -0000 Received: (qmail 19738 invoked by uid 22791); 7 Mar 2012 08:24:56 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,TW_CP,TW_TD X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Mar 2012 08:24:40 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1S5CAp-0001hp-Ic from Yao_Qi@mentor.com ; Wed, 07 Mar 2012 00:24:39 -0800 Received: from SVR-ORW-FEM-02.mgc.mentorg.com ([147.34.96.206]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Wed, 7 Mar 2012 00:24:38 -0800 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server id 14.1.289.1; Wed, 7 Mar 2012 00:24:37 -0800 Message-ID: <4F571B2E.3080707@codesourcery.com> Date: Wed, 07 Mar 2012 08:25:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 MIME-Version: 1.0 To: Pedro Alves CC: Subject: Re: [patch] Fix PR 13392 : check offset of JMP insn References: <4F561363.4070702@codesourcery.com> <4F56434F.4030107@redhat.com> In-Reply-To: <4F56434F.4030107@redhat.com> Content-Type: multipart/mixed; boundary="------------020401090203060508060207" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-03/txt/msg00213.txt.bz2 --------------020401090203060508060207 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Content-length: 3888 On 03/07/2012 01:03 AM, Pedro Alves wrote: > 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. This problem is what I planed to fix in next step. > > 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 Yes, there are 3 levels so far. Do you think target should keep fast tracepoints that "downloaded but not installed yet"? I am inclined to teach target to remove downloaded fast tracepoints if they are not installed successfully. Does it sounds good? The QTDP is like a transaction, returns OK when both download and installation is successful. If installation failed, roll back to cleanup downloaded fast tracepoint. > 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 Agreed. GDB tracepoint/breakpoint module should exposes some interfaces for other modules, such as target, to delete or disable breakpoint locations when needed. > send a clearer error back to GDB, and we end up with: > > Target returns error code '.Tracepoint 2 at 0x7ffff67c2579 already exists'. It is OK for me to let remote target to reports a duplicated tracepoint. In this version, some changes compared with last one, - move `tracepoint already exist' patch out, which can be a separate one, - remove downloaded tracepoint in target when failed to install, -- Yao (齐尧) --------------020401090203060508060207 Content-Type: text/x-patch; name="0001-fix-pr13392-check-offset-of-jmp-insn.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-fix-pr13392-check-offset-of-jmp-insn.patch" Content-length: 12421 2012-03-07 Yao Qi Pedro Alves Fix PR server/13392. * linux-x86-low.c (amd64_install_fast_tracepoint_jump_pad): Check offset of JMP insn. * tracepoint.c (remove_tracepoint): New. (cmd_qtdp): Call remove_tracepoint when failed to install. 2012-03-07 Yao Qi Pedro Alves 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): Likewise. (pending_tracepoint_resolved_during_trace): Likewise. (pending_tracepoint_installed_during_trace): Likewise. (pending_tracepoint_with_action_resolved): Likewise. --- gdb/gdbserver/linux-x86-low.c | 27 +++++++++- gdb/gdbserver/tracepoint.c | 24 ++++++++ gdb/testsuite/gdb.trace/change-loc.exp | 71 ++++++++++++++++++------ gdb/testsuite/gdb.trace/pending.exp | 94 ++++++++++++++++++++++---------- 4 files changed, 168 insertions(+), 48 deletions(-) diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c index 58aaf9a..ed1f8a8 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,17 @@ 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) + { + 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 +1345,17 @@ 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) + { + 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..6ec322a 100644 --- a/gdb/gdbserver/tracepoint.c +++ b/gdb/gdbserver/tracepoint.c @@ -1684,6 +1684,28 @@ find_tracepoint (int id, CORE_ADDR addr) return NULL; } +/* Remove TPOINT from global list. */ + +static void +remove_tracepoint (struct tracepoint *tpoint) +{ + struct tracepoint *tp, *tp_prev; + + for (tp = tracepoints, tp_prev = NULL; tp && tp != tpoint; + tp_prev = tp, tp = tp->next) + ; + + if (tp) + { + if (tp_prev) + tp_prev->next = tp->next; + else + tracepoints->next = tp->next; + + xfree (tp); + } +} + /* There may be several tracepoints with the same number (because they are "locations", in GDB parlance); return the next one after the given tracepoint, or search from the beginning of the list if the @@ -2391,6 +2413,8 @@ cmd_qtdp (char *own_buf) download_tracepoint (tpoint); install_tracepoint (tpoint, own_buf); + if (strncmp (own_buf, "OK", 2) != 0) + remove_tracepoint (tpoint); unpause_all (1); return; diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp index d6062fc..71890d7 100644 --- a/gdb/testsuite/gdb.trace/change-loc.exp +++ b/gdb/testsuite/gdb.trace/change-loc.exp @@ -98,7 +98,21 @@ proc tracepoint_change_loc_1 { trace_type } { with_test_prefix "1 $trace_type" { gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \ "continue to marker 1" # Set a tracepoint during tracing. - gdb_test "${trace_type} set_tracepoint" ".*" "set tracepoint on set_tracepoint" + set test "set tracepoint on set_tracepoint" + gdb_test_multiple "${trace_type} set_tracepoint" $test { + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # The target was unable to install the fast tracepoint + # (e.g., jump pad too far from tracepoint). + pass "$test (too far)" + } else { + fail $test + } + } + -re "\r\n$gdb_prompt $" { + pass $test + } + } gdb_trace_setactions "set action for tracepoint" "" \ "collect \$$pcreg" "^$" @@ -109,15 +123,27 @@ proc tracepoint_change_loc_1 { trace_type } { with_test_prefix "1 $trace_type" { \[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\.*4\.1.* in func4.*4\.2.* in func4.*" \ "tracepoint with two locations" - gdb_test_multiple "continue" "continue to marker 2" { - -re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { - pass "continue to marker 2" - } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker 2" - return - } + set test "continue to marker 2" + gdb_test_multiple "continue" $test { + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., jump pad too far from + # tracepoint). + pass "$test (too far)" + # Skip the rest of the tests. + return + } else { + fail "continue to marker 2" + fail $test + } + + } + -re ".*Breakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { + pass "continue to marker 2" + } } + # tracepoint has three locations after shlib change-loc-2 is loaded. gdb_test "info trace" \ "Num Type\[ \]+Disp Enb Address\[ \]+What.* @@ -198,19 +224,28 @@ 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" - } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker 1" - return + set test "tstart" + gdb_test_multiple "tstart" $test { + -re "^tstart\r\n$gdb_prompt $" { + pass "tstart" + } + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # The target was unable to install the fast tracepoint + # (e.g., jump pad too far from tracepoint). + pass "$test (too far)" + # Skip the rest of the tests. + return + } else { + fail $test + } } } gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \ + "continue to marker 1" + + gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \ "continue to marker 2" # 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..7a41b2f 100644 --- a/gdb/testsuite/gdb.trace/pending.exp +++ b/gdb/testsuite/gdb.trace/pending.exp @@ -132,18 +132,28 @@ proc pending_tracepoint_works { trace_type } { with_test_prefix "$trace_type wor gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \ "breakpoint on marker" - gdb_test_no_output "tstart" "start trace experiment" + set test "start trace experiment" + gdb_test_multiple "tstart" $test { + -re "^tstart\r\n$gdb_prompt $" { + pass $test + } + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # The target was unable to install the fast tracepoint + # (e.g., jump pad too far from tracepoint). + pass "$test (too far)" + # Skip the rest of the tests. + return + } else { + fail $test + } + } - gdb_test_multiple "continue" "continue to marker" { - -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { - pass "continue to marker" - } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker" - return - } } + gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*" \ + "continue to marker" + gdb_test "tstop" "\[\r\n\]+" "stop trace experiment" gdb_test "tfind start" "#0 .*" "tfind test frame 0" @@ -189,13 +199,22 @@ proc pending_tracepoint_resolved_during_trace { trace_type } \ gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \ "continue to marker 1" - gdb_test_multiple "continue" "continue to marker 2" { - -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { - pass "continue to marker 2" + set test "continue to marker 2" + gdb_test_multiple "continue" $test { + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., jump pad too far from + # tracepoint). + pass "$test (too far)" + # Skip the rest of the tests. + return + } else { + fail $test + } } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker 2" - return + -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { + pass $test } } @@ -253,14 +272,23 @@ proc pending_tracepoint_installed_during_trace { trace_type } \ \[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \t\]+keep y.*PENDING.*set_point2.*" \ "single pending tracepoint on set_point2" - gdb_test_multiple "continue" "continue to marker 2" { - -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { - pass "continue to marker 2" - } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker 2" - return - } + set test "continue to marker 2" + gdb_test_multiple "continue" $test { + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., jump pad too far from + # tracepoint). + pass "$test (too far)" + # Skip the rest of the tests. + return + } else { + fail $test + } + } + -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { + pass $test + } } gdb_test "tstop" "\[\r\n\]+" "stop trace experiment" @@ -423,14 +451,24 @@ proc pending_tracepoint_with_action_resolved { trace_type } \ gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \ "continue to marker 1" - gdb_test_multiple "continue" "continue to marker 2" { + set test "continue to marker 2" + gdb_test_multiple "continue" $test { + -re "Target returns error code .* too far .*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., jump pad too far from + # tracepoint). + pass "$test (too far)" + # Skip the rest of the tests. + return + } else { + fail $test + } + } -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { pass "continue to marker 2" } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker 2" - return - } + } gdb_test "tstop" "\[\r\n\]+" "stop trace experiment" -- 1.7.0.4 --------------020401090203060508060207--