* [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. @ 2015-09-01 18:25 Wei-cheng Wang 2015-09-02 15:51 ` Yao Qi 0 siblings, 1 reply; 8+ messages in thread From: Wei-cheng Wang @ 2015-09-01 18:25 UTC (permalink / raw) To: uweigand, gdb-patches, cole945 Hi, I just realized that disabled tracepoints will still be downloaded on the target by 'tstart' command, and they can be re-enabled (by QTEnable packet) when trace is running, so I cannot simply disable an invalid tracepoint to prevent invalid tracepoint being downloaded. Scenario 1: the trace is not running. Because all tracepoints will be downloaded, when 'tstart', it doesn't matter whether they are disabled or not. I think if any invalid tracepoint is found, simply throw an error to force user to delete invalid tracepoint. Scenario 2: the trace is already running. I think the check should be placed before update_global_location_list, because it will try to download new tracepoints on target. (download_tracepoint_locations). update_breakpoint_locations will not download disabled tracepoints, so we should disable invalid tracepoints before calling update_global_location_list. (see: should_be_inserted in download_tracepoint_locations) Otherwise, if we don't disable invalid tracepoints, an error is thrown, and the program will stop at _dl_debug_state. Then the user has to delete the tracepoint manually to continue the execution. If a pending tracepoint is resovled after trace is running, and the user tries to enable an invalid tracepoint after , server will report an error that it is not installed. It won't run into internal error. Any suggestion? Thanks, Wei-cheng -- gdb/ChangeLog 2015-08-23 Wei-cheng Wang <cole945@gmail.com> * breakpoint.c (update_breakpoint_locations): Check fast tracepoints after reloading symbols. * remote.c (remote_download_tracepoint): Change internal_error to error. * pending.exp (pending_tracepoint_resolved, pending_tracepoint_resolved, pending_tracepoint_works, pending_tracepoint_resolved_during_trace, pending_tracepoint_installed_during_trace, pending_tracepoint_with_action_resolved): Fix test cases accordingly. --- gdb/breakpoint.c | 22 +++++++++++++ gdb/remote.c | 4 +-- gdb/testsuite/gdb.trace/pending.exp | 62 +++++++++++++++++++++++++++++++++++-- 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 052aeb9..7645d18 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -14148,6 +14148,28 @@ update_breakpoint_locations (struct breakpoint *b, if (!locations_are_equal (existing_locations, b->loc)) observer_notify_breakpoint_modified (b); + /* Check fast tracepoints after symbols have been re-loaded. + For example, a pending tracepoint just becomes available after + a new shared object being loaded. We didn't check it before, + because we have no idea where it is. */ + if (b->type == bp_fast_tracepoint) + { + TRY + { + check_fast_tracepoint_sals (b->gdbarch, &sals); + } + CATCH (e, RETURN_MASK_ERROR) + { + /* If that tracepoint cannot be inserted, disable it, + so update_global_location_list will not install it + to the target. */ + b->enable_state = bp_disabled; + exception_fprintf (gdb_stderr, e, _("Invalid tracepoint %d: "), + b->number); + } + END_CATCH + } + update_global_location_list (UGLL_MAY_INSERT); } diff --git a/gdb/remote.c b/gdb/remote.c index ca1f0df..2e514fb 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -11145,9 +11145,7 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc) else /* If it passed validation at definition but fails now, something is very wrong. */ - internal_error (__FILE__, __LINE__, - _("Fast tracepoint not " - "valid during download")); + error (_("Fast tracepoint not valid during download")); } else /* Fast tracepoints are functionally identical to regular diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp index ed36cac..94f39ac 100644 --- a/gdb/testsuite/gdb.trace/pending.exp +++ b/gdb/testsuite/gdb.trace/pending.exp @@ -65,6 +65,7 @@ proc pending_tracepoint_resolved { trace_type } { global binfile global srcfile global lib_sl1 + global gdb_prompt # Start with a fresh gdb. gdb_exit @@ -89,12 +90,22 @@ proc pending_tracepoint_resolved { trace_type } { "breakpoint function" gdb_run_cmd - gdb_test "" "Breakpoint 2, main.*" + set test "Breakpoint 2, main" + set yesno "y" + gdb_test_multiple "" $test { + -re ".*May not have a fast tracepoint at.*$test.*" { + set yesno "n" + pass "$test (invalid)" + } + -re "Breakpoint.*main.*at.*$gdb_prompt $" { + pass $test + } + } # Run to main which should resolve a pending tracepoint gdb_test "info trace" \ "Num Type\[ \]+Disp Enb Address\[ \]+What.* -\[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \]+keep y.*pendfunc.*" \ +\[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \]+keep $yesno.*pendfunc.*" \ "single tracepoint info" } } @@ -151,7 +162,16 @@ proc pending_tracepoint_works { trace_type } { fail $test } } - + -re "No tracepoints enabled.*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # There is no valid tracepoint enabled. + pass "$test (no tracepoints)" + # Skip the rest of the tests. + return + } else { + fail $test + } + } } gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*" \ @@ -221,6 +241,18 @@ proc pending_tracepoint_resolved_during_trace { trace_type } \ fail $test } } + -re "May not have a fast tracepoint at.*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., the location is invalid for the + # tracepoint). + pass "$test (invalid)" + # Skip the rest of the tests. + return + } else { + fail $test + } + } -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { pass $test } @@ -294,6 +326,18 @@ proc pending_tracepoint_installed_during_trace { trace_type } \ fail $test } } + -re "May not have a fast tracepoint at.*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., the location is invalid for the + # tracepoint). + pass "$test (invalid)" + # Skip the rest of the tests. + return + } else { + fail $test + } + } -re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { pass $test } @@ -475,6 +519,18 @@ proc pending_tracepoint_with_action_resolved { trace_type } \ fail $test } } + -re "May not have a fast tracepoint at.*$gdb_prompt $" { + if [string equal $trace_type "ftrace"] { + # Expected if the target was unable to install the + # fast tracepoint (e.g., the location is invalid for the + # tracepoint). + pass "$test (invalid)" + # 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" } -- 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. 2015-09-01 18:25 [PATCH 1/2 v2] Re-check fastpoint after reloading symbols Wei-cheng Wang @ 2015-09-02 15:51 ` Yao Qi [not found] ` <CAPmZyH61j2ECFu7vcg6ZAyhJWNGHpUHk60fOfkXG5qb9M-5pFA@mail.gmail.com> 0 siblings, 1 reply; 8+ messages in thread From: Yao Qi @ 2015-09-02 15:51 UTC (permalink / raw) To: Wei-cheng Wang; +Cc: uweigand, gdb-patches Wei-cheng Wang <cole945@gmail.com> writes: Hi Wei-cheng, > I just realized that disabled tracepoints will still be downloaded > on the target by 'tstart' command, and they can be re-enabled (by > QTEnable packet) when trace is running, so I cannot simply disable > an invalid tracepoint to prevent invalid tracepoint being downloaded. > > Scenario 1: the trace is not running. > > Because all tracepoints will be downloaded, when 'tstart', it doesn't > matter whether they are disabled or not. I think if any invalid > tracepoint is found, simply throw an error to force user to delete > invalid tracepoint. > > Scenario 2: the trace is already running. > > I think the check should be placed before update_global_location_list, > because it will try to download new tracepoints on target. > (download_tracepoint_locations). update_breakpoint_locations will not > download disabled tracepoints, so we should disable invalid tracepoints > before calling update_global_location_list. > (see: should_be_inserted in download_tracepoint_locations) > > Otherwise, if we don't disable invalid tracepoints, an error is thrown, > and the program will stop at _dl_debug_state. Then the user has to delete > the tracepoint manually to continue the execution. > > If a pending tracepoint is resovled after trace is running, and the user > tries to enable an invalid tracepoint after , server will report an > error that it is not installed. It won't run into internal error. > > Any suggestion? I didn't follow your previous tracepoint patches, so I don't understand what problem are you trying to fix in this patch. Nowadays, GDB sends fast tracepoint to GDBserver, if GDBserver can't install it (jump pad is too far from tracepoint), GDBserver will return error, and GDB knows about it. Do you want to do the check in GDB side rather than GDBserver side? -- Yao (齐尧) ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CAPmZyH61j2ECFu7vcg6ZAyhJWNGHpUHk60fOfkXG5qb9M-5pFA@mail.gmail.com>]
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. [not found] ` <CAPmZyH61j2ECFu7vcg6ZAyhJWNGHpUHk60fOfkXG5qb9M-5pFA@mail.gmail.com> @ 2015-09-12 18:01 ` Wei-cheng Wang 2015-09-14 12:20 ` Ulrich Weigand 0 siblings, 1 reply; 8+ messages in thread From: Wei-cheng Wang @ 2015-09-12 18:01 UTC (permalink / raw) To: Yao Qi; +Cc: Ulrich Weigand, gdb-patches Hi Yao, Thank you for looking into this. https://sourceware.org/ml/gdb-patches/2015-06/msg00589.html Ulrich suggested me to check whether the fast tracepoint is valid on GDB side. However, if it returns false, it will cause an internal error in remote_download_tracepoint. And this patch is for that issue. As you said in previous mail. If it's not right to check the validity based on the assumption of where the jumppad is. Maybe it should always return 'yes' for powerpc in ppc_fast_tracepoint_valid_at (or simply use default_fast_tracepoint_valid_at) Any suggestion, Ulrich? Thanks, Wei-cheng On Sun, Sep 13, 2015 at 1:31 AM, Wei-cheng Wang <cole945@gmail.com> wrote: > Hi Yao, > > Thank you for looking into this. > > https://sourceware.org/ml/gdb-patches/2015-06/msg00589.html > Ulrich suggested me to check whether the fast tracepoint is valid on GDB > side. > However, if it returns false, it will cause an internal error in > remote_download_tracepoint. > > As you said in previous mail. If it's not right to check the validity based > on the assumption of > where the jumppad is. Maybe it should always return 'yes' for powerpc in > ppc_fast_tracepoint_valid_at > (or simply use default_fast_tracepoint_valid_at) > > Any suggestion, Ulrich? > > Thanks, > Wei-cheng > > > On Wed, Sep 2, 2015 at 11:51 PM, Yao Qi <qiyaoltc@gmail.com> wrote: >> >> Wei-cheng Wang <cole945@gmail.com> writes: >> >> Hi Wei-cheng, >> >> > I just realized that disabled tracepoints will still be downloaded >> > on the target by 'tstart' command, and they can be re-enabled (by >> > QTEnable packet) when trace is running, so I cannot simply disable >> > an invalid tracepoint to prevent invalid tracepoint being downloaded. >> > >> > Scenario 1: the trace is not running. >> > >> > Because all tracepoints will be downloaded, when 'tstart', it doesn't >> > matter whether they are disabled or not. I think if any invalid >> > tracepoint is found, simply throw an error to force user to delete >> > invalid tracepoint. >> > >> > Scenario 2: the trace is already running. >> > >> > I think the check should be placed before update_global_location_list, >> > because it will try to download new tracepoints on target. >> > (download_tracepoint_locations). update_breakpoint_locations will not >> > download disabled tracepoints, so we should disable invalid tracepoints >> > before calling update_global_location_list. >> > (see: should_be_inserted in download_tracepoint_locations) >> > >> > Otherwise, if we don't disable invalid tracepoints, an error is thrown, >> > and the program will stop at _dl_debug_state. Then the user has to >> > delete >> > the tracepoint manually to continue the execution. >> > >> > If a pending tracepoint is resovled after trace is running, and the user >> > tries to enable an invalid tracepoint after , server will report an >> > error that it is not installed. It won't run into internal error. >> > >> > Any suggestion? >> >> I didn't follow your previous tracepoint patches, so I don't understand >> what problem are you trying to fix in this patch. Nowadays, GDB sends >> fast tracepoint to GDBserver, if GDBserver can't install it (jump pad is >> too far from tracepoint), GDBserver will return error, and GDB knows >> about it. Do you want to do the check in GDB side rather than GDBserver >> side? >> >> -- >> Yao (齐尧) > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. 2015-09-12 18:01 ` Wei-cheng Wang @ 2015-09-14 12:20 ` Ulrich Weigand 2015-09-14 15:34 ` Yao Qi 0 siblings, 1 reply; 8+ messages in thread From: Ulrich Weigand @ 2015-09-14 12:20 UTC (permalink / raw) To: Wei-cheng Wang, Yao Qi; +Cc: gdb-patches On Wed, Sep 2, 2015 at 11:51 PM, Yao Qi <qiyaoltc@gmail.com> wrote: > I didn't follow your previous tracepoint patches, so I don't understand > what problem are you trying to fix in this patch. Nowadays, GDB sends > fast tracepoint to GDBserver, if GDBserver can't install it (jump pad is > too far from tracepoint), GDBserver will return error, and GDB knows > about it. Do you want to do the check in GDB side rather than GDBserver > side? Hi Yao, from what I see, if GDBserver cannot install a fast tracepoint, it will indeed return an error. However, GDB does not react particularly well to such errors: if you look at remote_download_tracepoint, any type of error returned from the QTDP packet will result in: putpkt (buf); remote_get_noisy_reply (&target_buf, &target_buf_size); if (strcmp (target_buf, "OK")) error (_("Target does not support tracepoints.")); All checking whether a specific tracepoint is valid currently takes place earlier, on the GDB side, using a gdbarch callback: if (b->type == bp_fast_tracepoint) { /* Only test for support at download time; we may not know target capabilities at definition time. */ if (remote_supports_fast_tracepoints ()) { if (gdbarch_fast_tracepoint_valid_at (loc->gdbarch, tpaddr, NULL)) xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x", gdb_insn_length (loc->gdbarch, tpaddr)); else /* If it passed validation at definition but fails now, something is very wrong. */ internal_error (__FILE__, __LINE__, _("Fast tracepoint not " "valid during download")); } else /* Fast tracepoints are functionally identical to regular tracepoints, so don't take lack of support as a reason to give up on the trace run. */ warning (_("Target does not support fast tracepoints, " "downloading %d as regular tracepoint"), b->number); } If the target does not support fast tracepoints at all, they are converted into regular tracepoints, which seems reasonable. However, if just a single tracepoint is invalid, we simply get an internal error here, which is the problem Wei-cheng is trying to fix. [ The same gdbarch_fast_tracepoint_valid_at call is done at the time the tracepoint was installed originally. However, if at that time the tracepoint was pending, and its location has now been resolved, that check was not re-done. ] I see two issues here: - I agree with you that there's duplicated checks here. The gdbarch callback gdbarch_fast_tracepoint_valid_at is apparently supposed to duplicate the logic done in gdbserver. In any case, any tracepoint considered "valid" by callback is expected to succeed on the target. - The ICE can be triggered by normal user action (if the result of gdbarch_fast_tracepoint_valid_at changes if locations are recomputed, see above). What I had asked Wei-cheng to implement is to fix these two issues: properly duplicate the validity check for PowerPC, and re-validate every time locations are resolved. Maybe it would indeed be better to move the validity-checking logic completely to the target side, i.e. always attempt to download the tracepoint, and react more intelligently if that fails (e.g. downgrade to a regular tracepoint?). I'm not sure if that is doable with the current remote protocol definition. Thoughts? Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. 2015-09-14 12:20 ` Ulrich Weigand @ 2015-09-14 15:34 ` Yao Qi 2015-09-15 16:22 ` Ulrich Weigand 0 siblings, 1 reply; 8+ messages in thread From: Yao Qi @ 2015-09-14 15:34 UTC (permalink / raw) To: Ulrich Weigand; +Cc: Wei-cheng Wang, Yao Qi, gdb-patches "Ulrich Weigand" <uweigand@de.ibm.com> writes: > However, if just a single tracepoint is invalid, we simply get an > internal error here, which is the problem Wei-cheng is trying to fix. > [ The same gdbarch_fast_tracepoint_valid_at call is done at the time > the tracepoint was installed originally. However, if at that time > the tracepoint was pending, and its location has now been resolved, > that check was not re-done. ] Thanks for the clarification, Ulrich. > > I see two issues here: > > - I agree with you that there's duplicated checks here. The gdbarch > callback gdbarch_fast_tracepoint_valid_at is apparently supposed to > duplicate the logic done in gdbserver. In any case, any tracepoint > considered "valid" by callback is expected to succeed on the target. > > - The ICE can be triggered by normal user action (if the result of > gdbarch_fast_tracepoint_valid_at changes if locations are recomputed, > see above). > > What I had asked Wei-cheng to implement is to fix these two issues: > properly duplicate the validity check for PowerPC, and re-validate > every time locations are resolved. That is fine by me in general, and I attach a reproducer to trigger GDB internal error on x86. Wei-cheng's patch fixes this internal error, and instead, a normal error is emitted. > > Maybe it would indeed be better to move the validity-checking logic > completely to the target side, i.e. always attempt to download the > tracepoint, and react more intelligently if that fails (e.g. downgrade > to a regular tracepoint?). I'm not sure if that is doable with the > current remote protocol definition. Thoughts? The duplicated checks in both GDB and GDBserver sides aren't that bad to me, however, I am worried about using internal knowledge of GDBserver and IPA to validate fast tracepoint in GDB side. I raised this here https://sourceware.org/ml/gdb-patches/2015-09/msg00041.html I suspect that we won't see GDB internal error on PPC if we don't do checks using GDBserver internal knowledge. Without GDBserver internal knowledge, ppc_fast_tracepoint_valid_at should always return true. -- Yao (齐尧) From 218d77634bb67045e235b3c619cfea53e561ce91 Mon Sep 17 00:00:00 2001 From: Yao Qi <yao.qi@linaro.org> Date: Mon, 14 Sep 2015 16:18:10 +0100 Subject: [PATCH] Trigger GDB internal error when pending fast tracepoint is resolved but invalide to install Nowadays, when pending fast tracepoint is resolved, but can't be installed, an internal error will be triggered. This patch is to reproduce this. With this patch applied, we'll see, (gdb) PASS: gdb.trace/pending.exp: ftrace on short insn: continue to marker 1 continue^M Continuing.^M Reading x86_64/gdb/testsuite/gdb.trace/pendshr2.sl from remote target...^M gdb/git/gdb/remote.c:11402: internal-error: Fast tracepoint not valid during download^M A problem internal to GDB has been detected,^M further debugging may prove unreliable.^M Quit this debugging session? (y or n) FAIL: gdb.trace/pending.exp: ftrace on short insn: continue to marker 2 (GDB internal error) gdb/testsuite: 2015-09-14 Yao Qi <yao.qi@linaro.org> * gdb.trace/pending.exp (pending_tracepoint_on_short_insn): New proc. Invoke it with "trace" and "ftrace". * gdb.trace/pendshr2.c (pendfunc2): Define symbol set_point3 and use short instructions. diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp index 9938c5a..b27887a 100644 --- a/gdb/testsuite/gdb.trace/pending.exp +++ b/gdb/testsuite/gdb.trace/pending.exp @@ -492,6 +492,64 @@ proc pending_tracepoint_with_action_resolved { trace_type } \ gdb_test "tfind" "Target failed to find requested trace frame..*" "tfind test frame" }} +# Verify setting tracepoint on a very short instruction. + +proc pending_tracepoint_on_short_insn { trace_type } \ +{ with_test_prefix "$trace_type on short insn" \ +{ + if { ![istarget "x86_64-*"] && ![istarget "i?86-*"] } { + return + } + + global executable + global srcfile + global lib_sl1 + global gdb_prompt + + # Start with a fresh gdb. + clean_restart $executable + if ![runto_main] { + fail "Can't run to main" + return -1 + } + + gdb_test_multiple "$trace_type set_point3" "set pending tracepoint on set_point2" { + -re ".*Make \(fast |\)tracepoint pending.*y or \\\[n\\\]. $" { + gdb_test "y" "\(Fast t|T\)racepoint.*set_point3.*pending." \ + "set pending tracepoint (without symbols)" + } + } + + gdb_test "info trace" \ + "Num Type\[ \]+Disp Enb Address\[ \]+What.* +\[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \]+keep y.*PENDING.*set_point3.*" \ + "single pending tracepoint on set_point3" + + gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \ + "breakpoint on marker" + + gdb_test_no_output "tstart" "start trace experiment" + + gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \ + "continue to marker 1" + + set test "continue to marker 2" + gdb_test_multiple "continue" $test { + -re "Continuing.\r\n(Reading .* from remote target...\r\n)?\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" { + pass "continue to marker 2" + } + + } + + gdb_test "tstop" "\[\r\n\]+" "stop trace experiment" + + # tracepoint should be resolved. + gdb_test "info trace" \ + "Num Type\[ \]+Disp Enb Address\[ \]+What.* +\[0-9\]+\[\t \]+\(fast |\)tracepoint\[ \]+keep y.*set_point3.*" \ + "tracepoint is resolved" +}} + pending_tracepoint_resolved "trace" pending_tracepoint_works "trace" @@ -506,6 +564,8 @@ pending_tracepoint_with_action_resolved "trace" pending_tracepoint_installed_during_trace "trace" +pending_tracepoint_on_short_insn "trace" + # Re-compile test case with IPA. set libipa [get_in_proc_agent] gdb_load_shlibs $libipa @@ -524,3 +584,4 @@ pending_tracepoint_disconnect_during_trace "ftrace" pending_tracepoint_disconnect_after_resolved "ftrace" pending_tracepoint_with_action_resolved "ftrace" pending_tracepoint_installed_during_trace "ftrace" +pending_tracepoint_on_short_insn "ftrace" diff --git a/gdb/testsuite/gdb.trace/pendshr2.c b/gdb/testsuite/gdb.trace/pendshr2.c index b8a51a5..7d7a2db 100644 --- a/gdb/testsuite/gdb.trace/pendshr2.c +++ b/gdb/testsuite/gdb.trace/pendshr2.c @@ -37,4 +37,19 @@ pendfunc2 (int x) " call " SYMBOL(foo) "\n" #endif ); + + /* `set_point3' is the label where we'll set multiple tracepoints at, + but the instruction at the label isn't large enough to fit a + fast tracepoint jump. */ + asm (" .global " SYMBOL(set_point3) "\n" + SYMBOL(set_point3) ":\n" +#if defined __i386__ + "push %ecx\n" + "pop %ecx\n" +#elif defined __x86_64__ + "pushq %rax\n" + "popq %rax\n" +#else +#endif + ); } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. 2015-09-14 15:34 ` Yao Qi @ 2015-09-15 16:22 ` Ulrich Weigand 2015-09-16 8:35 ` Yao Qi 0 siblings, 1 reply; 8+ messages in thread From: Ulrich Weigand @ 2015-09-15 16:22 UTC (permalink / raw) To: Yao Qi; +Cc: Wei-cheng Wang, Yao Qi, gdb-patches Yao Qi wrote: > "Ulrich Weigand" <uweigand@de.ibm.com> writes: > > What I had asked Wei-cheng to implement is to fix these two issues: > > properly duplicate the validity check for PowerPC, and re-validate > > every time locations are resolved. > > That is fine by me in general, and I attach a reproducer to trigger GDB > internal error on x86. Wei-cheng's patch fixes this internal error, and > instead, a normal error is emitted. Thanks for the test case! > > Maybe it would indeed be better to move the validity-checking logic > > completely to the target side, i.e. always attempt to download the > > tracepoint, and react more intelligently if that fails (e.g. downgrade > > to a regular tracepoint?). I'm not sure if that is doable with the > > current remote protocol definition. Thoughts? > > The duplicated checks in both GDB and GDBserver sides aren't that bad to > me, however, I am worried about using internal knowledge of > GDBserver and IPA to validate fast tracepoint in GDB side. I raised > this here https://sourceware.org/ml/gdb-patches/2015-09/msg00041.html > > I suspect that we won't see GDB internal error on PPC if we don't do > checks using GDBserver internal knowledge. Without GDBserver internal > knowledge, ppc_fast_tracepoint_valid_at should always return true. I guess I'm not really sure what the difference is between checks "using GDBserver internal knowledge" and those that don't. Doesn't the x86 check, strictly speaking, also use GDBserver internal knowlegde, i.e. it knows which instruction GDBserver attempts to place at the tracepoint location, and therefore knows how much space must be available there? That's why I am now wondering whether the best fix shouldn't be to simply remove the GDB-side check completely, even on x86, and solely rely on GDBserver reporting errors via the remote protocol ... Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. 2015-09-15 16:22 ` Ulrich Weigand @ 2015-09-16 8:35 ` Yao Qi 2015-09-16 12:41 ` Ulrich Weigand 0 siblings, 1 reply; 8+ messages in thread From: Yao Qi @ 2015-09-16 8:35 UTC (permalink / raw) To: Ulrich Weigand; +Cc: Yao Qi, Wei-cheng Wang, gdb-patches "Ulrich Weigand" <uweigand@de.ibm.com> writes: > I guess I'm not really sure what the difference is between checks > "using GDBserver internal knowledge" and those that don't. Doesn't > the x86 check, strictly speaking, also use GDBserver internal knowlegde, > i.e. it knows which instruction GDBserver attempts to place at the > tracepoint location, and therefore knows how much space must be > available there? It is different when GDB talks with some debugging stubs other than GDBserver, which support tracepoint too. It is OK that GDB checks fast tracepoint's validity according to GDB's own knowledge, such as instruction size. That is what x86 does nowadays. However, GDB can't check validity by checking whether it is too far to jump to the jumppad from the tracepoint address. Jumppad address is unknown to GDB, and other debugging stub may allocate jumppad differently. PPC GDB tracepoint do checks in this way, which isn't right to me. > > That's why I am now wondering whether the best fix shouldn't be to > simply remove the GDB-side check completely, even on x86, and solely > rely on GDBserver reporting errors via the remote protocol ... It is fine with me to move validity checks to GDBserver side, but we need to add some thing to decode x86 instructions and get to know instruction length. Nowadays, we are using gdb_print_insn which calls some function in opcodes in GDB side, but it doesn't exist in GDBserver side. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols. 2015-09-16 8:35 ` Yao Qi @ 2015-09-16 12:41 ` Ulrich Weigand 0 siblings, 0 replies; 8+ messages in thread From: Ulrich Weigand @ 2015-09-16 12:41 UTC (permalink / raw) To: Yao Qi; +Cc: Yao Qi, Wei-cheng Wang, gdb-patches Yao Qi wrote: > "Ulrich Weigand" <uweigand@de.ibm.com> writes: > > > I guess I'm not really sure what the difference is between checks > > "using GDBserver internal knowledge" and those that don't. Doesn't > > the x86 check, strictly speaking, also use GDBserver internal knowlegde, > > i.e. it knows which instruction GDBserver attempts to place at the > > tracepoint location, and therefore knows how much space must be > > available there? > > It is different when GDB talks with some debugging stubs other than > GDBserver, which support tracepoint too. It is OK that GDB checks fast > tracepoint's validity according to GDB's own knowledge, such as > instruction size. That is what x86 does nowadays. However, GDB can't > check validity by checking whether it is too far to jump to the jumppad > from the tracepoint address. Jumppad address is unknown to GDB, and > other debugging stub may allocate jumppad differently. PPC GDB > tracepoint do checks in this way, which isn't right to me. I understand simply measuring how much space is *available* does not require any GDBserver knowledge. But how much space is *required* does depend on the agent implementation. However, what I missed is that this fact is actually retrieved from the target using a special target_get_min_fast_tracepoint_insn_len callback. So you're right that the current x86 implementation does not rely on agent implentation details in GDB. I guess we could do something similar for ppc64 by adding a new remote protocol command to verify whether a tracepoint address is valid. (This could in fact supersede the target_get_min_fast_tracepoint_insn_len callback then.) But I guess I'd still prefer to do it this way, to avoid needing two packet round trips for each tracepoint: > > That's why I am now wondering whether the best fix shouldn't be to > > simply remove the GDB-side check completely, even on x86, and solely > > rely on GDBserver reporting errors via the remote protocol ... > > It is fine with me to move validity checks to GDBserver side, but we > need to add some thing to decode x86 instructions and get to know > instruction length. Nowadays, we are using gdb_print_insn which > calls some function in opcodes in GDB side, but it doesn't exist > in GDBserver side. That's true, but this information is computed by GDB and passed to the remote agent via the orig_size field of the tracepoint packet anyway, see this piece of code in remote_download_tracepoint: xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":F%x", gdb_insn_length (loc->gdbarch, tpaddr)); So the remote agent doesn't actually need to compute it again. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-16 12:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-01 18:25 [PATCH 1/2 v2] Re-check fastpoint after reloading symbols Wei-cheng Wang 2015-09-02 15:51 ` Yao Qi [not found] ` <CAPmZyH61j2ECFu7vcg6ZAyhJWNGHpUHk60fOfkXG5qb9M-5pFA@mail.gmail.com> 2015-09-12 18:01 ` Wei-cheng Wang 2015-09-14 12:20 ` Ulrich Weigand 2015-09-14 15:34 ` Yao Qi 2015-09-15 16:22 ` Ulrich Weigand 2015-09-16 8:35 ` Yao Qi 2015-09-16 12:41 ` Ulrich Weigand
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).