diff mbox

Fix several "set remote foo-packet on/off" commands.

Message ID 533AABE1.8040101@redhat.com
State Superseded
Headers show

Commit Message

Pedro Alves April 1, 2014, 12:06 p.m. UTC
Hi Yao,

Thanks for the review.

On 04/01/2014 09:53 AM, Yao Qi wrote:
> On 04/01/2014 07:10 AM, Pedro Alves wrote:
>> +/* Returns true if the multi-process extensions are in effect.  */
>> +static int
>> +remote_multi_process_p (struct remote_state *rs)
>> +{
>> +  return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE;
>> +}
> 
> Argument 'rs' is not needed.  We can remove it.

Yeah.  (Though note the parameter was already there, I just moved the
function further below at the same time I reimplemented it.)

> However, your patch moves more gdb remote target states from
> 'struct remote_state' to a global state.  I feel that it has
> conflict with the multi-target project.  

As part of that project, we'll definitely need to move the
whole remote_protocol_packets global array to remote_state.
This global array already exists.  I've just moved a few more manually
managed flags into this array.  Note this is listed as a TODO in
the MultiTarget project's wiki page.
Therefore, I'm not really adding any conflict, as that work will
already have to be done.  When that is done, we'll likely end up
with passing a 'struct remote_state *' pointer to the new packet_support
function.  And with that in mind, it just looks like unnecessary
churn to remove the parameter from remote_multi_process_p now (and
update all callers), only to add it back again.  Do you agree?

> Not sure it is part
> of your plan to convert the global state to per-target state.

I don't plan to work on it myself in the near future, but it's
definitely the way to go.

>> +
>> +	# Force-disable the InstallInTrace RSP feature.
>> +	gdb_test_no_output "set remote install-in-trace-packet off"
>> +
>> +	# Set a tracepoint while a trace experiment is ongoing.
>> +	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)"
> 
> A nit here, a fast tracepoint is not installed due to "jump pad too far"
> instead of "set remote install-in-trace-packet off".  Do we want to emit
> a PASS here?

Oh, hmm.  Copy-paste from the other function in the file, and I
just didn't think about that part much.  It actually makes no sense
to even expect that the target sends back any error code.  We've just
disabled installing tracepoints while a trace experiment is running,
so we shouldn't see any response from the target at all.

> IMO, UNSUPPORTED is better.

Agreed, though note you made this a PASS in tracepoint_change_loc_1.  ;-)

>  Secondly, if "jump pad too far"
> causes fast tracepoint not installed, we should return here to skip the
> rest of the test, because the test below can't tell why a fast
> tracepoint is not installed, is it caused by
> "set remote install-in-trace-packet off" or "jump pad too far".

Even better is to not expect any error at all, and then make sure
that no traceframe was collected.

Here's what I'm folding into v2 of the patch.

---
 gdb/remote.c                           |  1 +
 gdb/testsuite/gdb.trace/change-loc.exp | 35 +++++++++++++++++-----------------
 2 files changed, 18 insertions(+), 18 deletions(-)

Comments

Yao Qi April 1, 2014, 12:46 p.m. UTC | #1
On 04/01/2014 08:06 PM, Pedro Alves wrote:
> As part of that project, we'll definitely need to move the
> whole remote_protocol_packets global array to remote_state.
> This global array already exists.  I've just moved a few more manually
> managed flags into this array.  Note this is listed as a TODO in
> the MultiTarget project's wiki page.
> Therefore, I'm not really adding any conflict, as that work will
> already have to be done.  When that is done, we'll likely end up
> with passing a 'struct remote_state *' pointer to the new packet_support
> function.  And with that in mind, it just looks like unnecessary
> churn to remove the parameter from remote_multi_process_p now (and
> update all callers), only to add it back again.  Do you agree?
> 

Yes, that is fine to keep unused parameter 'rs' remote_multi_process_p.

>> > Not sure it is part
>> > of your plan to convert the global state to per-target state.
> I don't plan to work on it myself in the near future, but it's
> definitely the way to go.
> 

OK, got it.

>>> >> +
>>> >> +	# Force-disable the InstallInTrace RSP feature.
>>> >> +	gdb_test_no_output "set remote install-in-trace-packet off"
>>> >> +
>>> >> +	# Set a tracepoint while a trace experiment is ongoing.
>>> >> +	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)"
>> > 
>> > A nit here, a fast tracepoint is not installed due to "jump pad too far"
>> > instead of "set remote install-in-trace-packet off".  Do we want to emit
>> > a PASS here?
> Oh, hmm.  Copy-paste from the other function in the file, and I
> just didn't think about that part much.  It actually makes no sense
> to even expect that the target sends back any error code.  We've just
> disabled installing tracepoints while a trace experiment is running,
> so we shouldn't see any response from the target at all.
> 
>> > IMO, UNSUPPORTED is better.
> Agreed, though note you made this a PASS in tracepoint_change_loc_1.  ;-)
> 

Sorry about that :)

patch v2 looks good to me.
Pedro Alves April 25, 2014, 6:04 p.m. UTC | #2
On 04/01/2014 01:46 PM, Yao Qi wrote:

> patch v2 looks good to me.

Thanks Yao.  I've pushed this in.
diff mbox

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 1aac5e3..dcd3cdd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1432,6 +1432,7 @@  show_remote_protocol_Z_packet_cmd (struct ui_file *file, int from_tty,
 }
 
 /* Returns true if the multi-process extensions are in effect.  */
+
 static int
 remote_multi_process_p (struct remote_state *rs)
 {
diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp
index ff6918a..d1a7aee 100644
--- a/gdb/testsuite/gdb.trace/change-loc.exp
+++ b/gdb/testsuite/gdb.trace/change-loc.exp
@@ -286,7 +286,7 @@  proc tracepoint_change_loc_2 { trace_type } {
 # doesn't work when we force-disable the InstallInTrace RSP feature.
 
 proc tracepoint_install_in_trace_disabled { trace_type } {
-    with_test_prefix "3 $trace_type" {
+    with_test_prefix "InstallInTrace disabled: $trace_type" {
 	global testfile
 	global srcfile
 	global pcreg
@@ -312,27 +312,18 @@  proc tracepoint_install_in_trace_disabled { trace_type } {
 	    "Tracepoint \[0-9\] at.* file .*$srcfile, line.*" \
 	    "set tracepoint on main"
 
+	gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \
+	    "breakpoint on marker"
+
 	gdb_test_no_output "tstart"
 
 	# Force-disable the InstallInTrace RSP feature.
 	gdb_test_no_output "set remote install-in-trace-packet off"
 
 	# Set a tracepoint while a trace experiment is ongoing.
-	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_test "${trace_type} set_tracepoint" \
+	    "racepoint .* at .* set_tracepoint.*" \
+	    "set tracepoint on set_tracepoint"
 
 	gdb_trace_setactions "set action for tracepoint" "" \
 	    "collect \$$pcreg" "^$"
@@ -340,8 +331,16 @@  proc tracepoint_install_in_trace_disabled { trace_type } {
 	# Make sure the tracepoint is _not_ installed on the target.
 	gdb_test "info trace" \
 	    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
-\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\<MULTIPLE\>.*3\.1.* in func4.*not installed on target.*3\.2.* in func4.*not installed on target.*" \
-	    "tracepoint with two locations"
+\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*installed on target.*\<MULTIPLE\>.*4\.1.* in func4.*not installed on target.*4\.2.* in func4.*not installed on target.*" \
+	    "tracepoint is not installed"
+
+	gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \
+	    "continue to marker"
+
+	gdb_test_no_output "tstop"
+
+	# Nothing should have been collected.
+	gdb_test "tfind" "Target failed to find requested trace frame\\..*"
     }
 }