[1/2,v2] Re-check fastpoint after reloading symbols.
Commit Message
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(-)
Comments
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?
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 (齐尧)
>
>
@@ -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);
}
@@ -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
@@ -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"
}