From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21923 invoked by alias); 9 Mar 2012 03:51:38 -0000 Received: (qmail 21913 invoked by uid 22791); 9 Mar 2012 03:51:36 -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; Fri, 09 Mar 2012 03:51:16 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1S5qrL-0000Mq-2S from Yao_Qi@mentor.com ; Thu, 08 Mar 2012 19:51:15 -0800 Received: from SVR-ORW-FEM-05.mgc.mentorg.com ([147.34.97.43]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 8 Mar 2012 19:50:58 -0800 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.1.289.1; Thu, 8 Mar 2012 19:51:09 -0800 Message-ID: <4F597DED.7090601@codesourcery.com> Date: Fri, 09 Mar 2012 03:51: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> <4F571B2E.3080707@codesourcery.com> <4F58DDAB.7000304@redhat.com> In-Reply-To: <4F58DDAB.7000304@redhat.com> Content-Type: multipart/mixed; boundary="------------050205090302050804000400" 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/msg00318.txt.bz2 --------------050205090302050804000400 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Content-length: 1517 On 03/09/2012 12:26 AM, Pedro Alves wrote: >> > - remove downloaded tracepoint in target when failed to install, > We should also check what happens when we do "enable TRACEPOINT_FOO", and > that tracepoints fails to be likewise inserted. Following this direction, > we should make sure it stays disabled in the target, but _not_ deleted. > AFAIK, tracepoint is not inserted when "enable tracepoint", so we don't have to worry about the fails of inserting tracepoints in "enable tracepoint". Tracepoints are downloaded and installed in QTDP or QTStart. For QTenable/QTDisable, it simply changes the flag `enable' in tracepoint. Of course, we may get fails during QTenable/QTDisable, but fails are not caused by inserting/installing tracepoint. >> > +/* 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; > > This doesn't look correct. If there's no tp_prev, then TP must be > TRACEPOINTS, and so you need to do 'tracepoints = tp->next;'. > Oops, sorry. I should stop coding at mid-night. Fixed. Regression tested on x86_64-linux. Committed. http://sourceware.org/ml/gdb-cvs/2012-03/msg00146.html -- Yao (齐尧) --------------050205090302050804000400 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: 12261 2012-03-08 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-08 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 | 90 ++++++++++++++++++++++--------- 4 files changed, 166 insertions(+), 46 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..7bf576b 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 = 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 (strcmp (own_buf, "OK") != 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..a5a982f 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..4e7dc31 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" - - gdb_test_multiple "continue" "continue to marker" { - -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { - pass "continue to marker" + set test "start trace experiment" + gdb_test_multiple "tstart" $test { + -re "^tstart\r\n$gdb_prompt $" { + pass $test } - -re ".*$gdb_prompt $" { - kfail "gdb/13392" "continue to marker" - return + -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" "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 --------------050205090302050804000400--