[PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Wei-cheng Wang]

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

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Yao Qi]

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 (齐尧)

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Wei-cheng Wang]

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 (齐尧)
>
>

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Ulrich Weigand]

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

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Yao Qi]

"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
+       );
 }

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Ulrich Weigand]

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

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Yao Qi]

"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 (齐尧)

Re: [PATCH 1/2 v2] Re-check fastpoint after reloading symbols.

[Ulrich Weigand]

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