Re: [PATCH] Fix several "set remote foo-packet on/off" commands.

Pedro Alves
2014-04-01T12:07:04Z

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

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\\..*"
}
}

--
1.7.11.7