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

[Yao Qi <qiyaoltc at gmail dot com>]

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