[PATCH] make "set debug target" take effect immediately

[PATCH] make "set debug target" take effect immediately

[Tom Tromey]

Right now, "set debug target" acts a bit strangely.

Most target APIs only notice that it has changed when the target stack
is changed in some way.  This is because many methods implement the
setting using the special debug target.  However, a few spots do
change their behavior immediately -- any place explicitly checking
"targetdebug".

Some of this peculiar behavior is documented.  However, I think that
it just isn't very useful for it to work this way.  So, This patch
changes "set debug target" to take effect immediately in all cases.
This is done by simply calling update_current_target when the setting
is changed.

This required one small change in the test suite.  Here a test was
expecting the current behavior.

Built and regtested on x86-64 Fedora 20.

2014-07-28  Tom Tromey  <tromey@redhat.com>

	* target.c (set_targetdebug): New function.
	(initialize_targets): Pass set_targetdebug when creating "set
	debug target".

2014-07-28  Tom Tromey  <tromey@redhat.com>

	* gdb.texinfo (Debugging Output): Update for change to "set debug
	target".

2014-07-28  Tom Tromey  <tromey@redhat.com>

	* gdb.base/sss-bp-on-user-bp-2.exp: Expect output from "set debug
	target 0".
---
 gdb/ChangeLog                                  |  6 ++++++
 gdb/doc/ChangeLog                              |  5 +++++
 gdb/doc/gdb.texinfo                            |  3 +--
 gdb/target.c                                   | 11 ++++++++++-
 gdb/testsuite/ChangeLog                        |  5 +++++
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp |  2 +-
 6 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 32f709a..8d9148c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22996,8 +22996,7 @@ Show the current state of symbol table creation debugging.
 Turns on or off display of @value{GDBN} target debugging info. This info
 includes what is going on at the target level of GDB, as it happens. The
 default is 0.  Set it to 1 to track events, and to 2 to also track the
-value of large memory transfers.  Changes to this flag do not take effect
-until the next time you connect to a target or use the @code{run} command.
+value of large memory transfers.
 @item show debug target
 Displays the current state of displaying @value{GDBN} target debugging
 info.
diff --git a/gdb/target.c b/gdb/target.c
index d9b471b..a54494f 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -120,6 +120,8 @@ static void init_dummy_target (void);
 
 static void debug_to_open (char *, int);
 
+static void update_current_target (void);
+
 /* Pointer to array of target architecture structures; the size of the
    array; the current index into the array; the allocated size of the
    array.  */
@@ -175,6 +177,13 @@ int may_stop = 1;
 /* Non-zero if we want to see trace of target level stuff.  */
 
 static unsigned int targetdebug = 0;
+
+static void
+set_targetdebug  (char *args, int from_tty, struct cmd_list_element *c)
+{
+  update_current_target ();
+}
+
 static void
 show_targetdebug (struct ui_file *file, int from_tty,
 		  struct cmd_list_element *c, const char *value)
@@ -3556,7 +3565,7 @@ Show target debugging."), _("\
 When non-zero, target debugging is enabled.  Higher numbers are more\n\
 verbose.  Changes do not take effect until the next \"run\" or \"target\"\n\
 command."),
-			     NULL,
+			     set_targetdebug,
 			     show_targetdebug,
 			     &setdebuglist, &showdebuglist);
 
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index a196f68..dd793bd 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -76,7 +76,7 @@ if { $hardware_step } {
     return
 }
 
-gdb_test_no_output "set debug target 0"
+gdb_test "set debug target 0" "->to_log_command.*\\)"
 
 set line_re "\[^\r\n\]*"
 
-- 
1.9.3

Re: [PATCH] make "set debug target" take effect immediately

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Tom Tromey <tromey@redhat.com>
> Date: Mon, 28 Jul 2014 13:04:27 -0600
> 
> 2014-07-28  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.texinfo (Debugging Output): Update for change to "set debug
> 	target".

This part is approved.

>  static void
>  show_targetdebug (struct ui_file *file, int from_tty,
>  		  struct cmd_list_element *c, const char *value)
> @@ -3556,7 +3565,7 @@ Show target debugging."), _("\
>  When non-zero, target debugging is enabled.  Higher numbers are more\n\
>  verbose.  Changes do not take effect until the next \"run\" or \"target\"\n\
>  command."),

The 2nd sentence of the doc string says something that you deleted
from the manual.  Shouldn't it be deleted from here as well?

Thanks.

Re: [PATCH] make "set debug target" take effect immediately

[Eli Zaretskii]

> Date: Mon, 28 Jul 2014 22:18:47 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org, tromey@redhat.com
> 
> >  When non-zero, target debugging is enabled.  Higher numbers are more\n\
> >  verbose.  Changes do not take effect until the next \"run\" or \"target\"\n\
> >  command."),
> 
> The 2nd sentence of the doc string says something that you deleted
> from the manual.

Sorry, I meant the 3rd sentence, of course.

Re: [PATCH] make "set debug target" take effect immediately

[Tom Tromey]

Eli> The 2nd sentence of the doc string says something that you deleted
Eli> from the manual.  Shouldn't it be deleted from here as well?

Yeah, overlooked that one.

Tom

b/gdb/ChangeLog:
2014-07-28  Tom Tromey  <tromey@redhat.com>

	* target.c (set_targetdebug): New function.
	(initialize_targets): Pass set_targetdebug when creating "set
	debug target".

b/gdb/doc/ChangeLog:
2014-07-28  Tom Tromey  <tromey@redhat.com>

	* gdb.texinfo (Debugging Output): Update for change to "set debug
	target".

b/gdb/testsuite/ChangeLog:
2014-07-28  Tom Tromey  <tromey@redhat.com>

	* gdb.base/sss-bp-on-user-bp-2.exp: Expect output from "set debug
	target 0".

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 32f709a..8d9148c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22996,8 +22996,7 @@ Show the current state of symbol table creation debugging.
 Turns on or off display of @value{GDBN} target debugging info. This info
 includes what is going on at the target level of GDB, as it happens. The
 default is 0.  Set it to 1 to track events, and to 2 to also track the
-value of large memory transfers.  Changes to this flag do not take effect
-until the next time you connect to a target or use the @code{run} command.
+value of large memory transfers.
 @item show debug target
 Displays the current state of displaying @value{GDBN} target debugging
 info.
diff --git a/gdb/target.c b/gdb/target.c
index d9b471b..2a3baa4 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -120,6 +120,8 @@ static void init_dummy_target (void);
 
 static void debug_to_open (char *, int);
 
+static void update_current_target (void);
+
 /* Pointer to array of target architecture structures; the size of the
    array; the current index into the array; the allocated size of the
    array.  */
@@ -175,6 +177,13 @@ int may_stop = 1;
 /* Non-zero if we want to see trace of target level stuff.  */
 
 static unsigned int targetdebug = 0;
+
+static void
+set_targetdebug  (char *args, int from_tty, struct cmd_list_element *c)
+{
+  update_current_target ();
+}
+
 static void
 show_targetdebug (struct ui_file *file, int from_tty,
 		  struct cmd_list_element *c, const char *value)
@@ -3554,9 +3563,8 @@ initialize_targets (void)
 Set target debugging."), _("\
 Show target debugging."), _("\
 When non-zero, target debugging is enabled.  Higher numbers are more\n\
-verbose.  Changes do not take effect until the next \"run\" or \"target\"\n\
-command."),
-			     NULL,
+verbose."),
+			     set_targetdebug,
 			     show_targetdebug,
 			     &setdebuglist, &showdebuglist);
 
diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index a196f68..dd793bd 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -76,7 +76,7 @@ if { $hardware_step } {
     return
 }
 
-gdb_test_no_output "set debug target 0"
+gdb_test "set debug target 0" "->to_log_command.*\\)"
 
 set line_re "\[^\r\n\]*"
 

Re: [PATCH] make "set debug target" take effect immediately

[Yao Qi]

On 07/29/2014 03:04 AM, Tom Tromey wrote:
> diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
> index a196f68..dd793bd 100644
> --- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
> +++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
> @@ -76,7 +76,7 @@ if { $hardware_step } {
>      return
>  }
>  
> -gdb_test_no_output "set debug target 0"
> +gdb_test "set debug target 0" "->to_log_command.*\\)"
>  
>  set line_re "\[^\r\n\]*"

Tom,
Beside this change, we still need to update the pattern to match the
output of "set debug target 1".  We match "target_resume " nowadays,
but it doesn't exist in the output at all, because of the recent target
delegation changes.  In sss-bp-on-user-bp-2.exp, we have

gdb_test_no_output "set debug target 1"
set hardware_step 0
set test "probe target hardware step"
gdb_test_multiple "si" $test {
    -re "target_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
         ^^^^^^^^^^^^^
	set hardware_step 1
	pass $test
    }
    -re "$gdb_prompt $" {
	pass $test
    }
}

We need to replace "target_resume" with "to_resume" in the pattern,
otherwise, hardware_step is always zero, which is wrong.  Even
hardware_step is zero on x86, the test only fails once in about 10 runs.
That may be the reason we didn't find the mistake before.

-- 
Yao (齐尧)

Re: [PATCH] make "set debug target" take effect immediately

[Pedro Alves]

On 07/29/2014 08:18 AM, Yao Qi wrote:
> On 07/29/2014 03:04 AM, Tom Tromey wrote:
>> diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
>> index a196f68..dd793bd 100644
>> --- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
>> +++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
>> @@ -76,7 +76,7 @@ if { $hardware_step } {
>>      return
>>  }
>>  
>> -gdb_test_no_output "set debug target 0"
>> +gdb_test "set debug target 0" "->to_log_command.*\\)"
>>  
>>  set line_re "\[^\r\n\]*"
> 
> Tom,
> Beside this change, we still need to update the pattern to match the
> output of "set debug target 1".  We match "target_resume " nowadays,
> but it doesn't exist in the output at all, because of the recent target
> delegation changes.  In sss-bp-on-user-bp-2.exp, we have
> 
> gdb_test_no_output "set debug target 1"
> set hardware_step 0
> set test "probe target hardware step"
> gdb_test_multiple "si" $test {
>     -re "target_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
>          ^^^^^^^^^^^^^
> 	set hardware_step 1
> 	pass $test
>     }
>     -re "$gdb_prompt $" {
> 	pass $test
>     }
> }
> 
> We need to replace "target_resume" with "to_resume" in the pattern,
> otherwise, hardware_step is always zero, which is wrong.  

Definitely.  Thanks for noticing that.

> Even
> hardware_step is zero on x86, the test only fails once in about 10 runs.
> That may be the reason we didn't find the mistake before.

Yep.  For reference, git 829155c9 says:

    "The problem is that we had just resumed the target and the native
    GNU/Linux target can't read memory off of a running thread.  Most of
    the time, we get "lucky", because we manage to read memory before the
    kernel actually schedules the target to run."

-- 
Thanks,
Pedro Alves

Re: [PATCH] make "set debug target" take effect immediately

[Pedro Alves]

On 07/28/2014 08:04 PM, Tom Tromey wrote:
> Right now, "set debug target" acts a bit strangely.
> 
> Most target APIs only notice that it has changed when the target stack
> is changed in some way.  This is because many methods implement the
> setting using the special debug target.  However, a few spots do
> change their behavior immediately -- any place explicitly checking
> "targetdebug".
> 
> Some of this peculiar behavior is documented.  However, I think that
> it just isn't very useful for it to work this way.  So, This patch
> changes "set debug target" to take effect immediately in all cases.

Definitely.  Thanks!

> This is done by simply calling update_current_target when the setting
> is changed.


Pedro Alves

Re: [PATCH] make "set debug target" take effect immediately

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Beside this change, we still need to update the pattern to match the
Yao> output of "set debug target 1".  We match "target_resume " nowadays,
Yao> but it doesn't exist in the output at all, because of the recent target
Yao> delegation changes.

Thanks for noticing this.

What do you think of the appended?

Tom

2014-07-29  Tom Tromey  <tromey@redhat.com>

	* gdb.base/sss-bp-on-user-bp-2.exp: Match "to_resume", not
	"target_resume".

diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index a196f68..1a48116 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -62,7 +62,7 @@ gdb_test_no_output "set debug target 1"
 set hardware_step 0
 set test "probe target hardware step"
 gdb_test_multiple "si" $test {
-    -re "target_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
+    -re "to_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
 	set hardware_step 1
 	pass $test
     }

Re: [PATCH] make "set debug target" take effect immediately

[Yao Qi]

On 07/30/2014 03:34 AM, Tom Tromey wrote:
> What do you think of the appended?

We need to replace "target_resume" in the comments several lines above
too,

# detect that by looking for 'target_resume (..., step)' in "debug
                              ^^^^^^^^^^^^^
# target" output.

Otherwise, the patch looks good to me.

-- 
Yao (齐尧)

Re: [PATCH] make "set debug target" take effect immediately

[Tom Tromey]

Tom> What do you think of the appended?

Yao> We need to replace "target_resume" in the comments several lines above
Yao> too,

Thanks.  I've fixed it locally.

Tom

Re: [PATCH] make "set debug target" take effect immediately

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> b/gdb/ChangeLog:
Tom> 2014-07-28  Tom Tromey  <tromey@redhat.com>
Tom> 	* target.c (set_targetdebug): New function.
Tom> 	(initialize_targets): Pass set_targetdebug when creating "set
Tom> 	debug target".
Tom> b/gdb/doc/ChangeLog:
Tom> 2014-07-28  Tom Tromey  <tromey@redhat.com>
Tom> 	* gdb.texinfo (Debugging Output): Update for change to "set debug
Tom> 	target".
Tom> b/gdb/testsuite/ChangeLog:
Tom> 2014-07-28  Tom Tromey  <tromey@redhat.com>
Tom> 	* gdb.base/sss-bp-on-user-bp-2.exp: Expect output from "set debug
Tom> 	target 0".

I'm checking this in.

Tom

Re: [PATCH] make "set debug target" take effect immediately

[Tom Tromey]

>>>>> "Yao" == Tom Tromey <tromey@redhat.com> writes:

Tom> What do you think of the appended?

Yao> We need to replace "target_resume" in the comments several lines above
Yao> too,

Tom> Thanks.  I've fixed it locally.

Here is what I am checking in.

Tom

2014-08-04  Tom Tromey  <tromey@redhat.com>

	* gdb.base/sss-bp-on-user-bp-2.exp: Match "to_resume", not
	"target_resume".

diff --git a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
index a196f68..cb95da0 100644
--- a/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
+++ b/gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp
@@ -53,7 +53,7 @@ delete_breakpoints
 # traffic.  Hardware-step targets that can't access memory while the
 # target is running, either remote or native, are likewise affected.
 # So we just skip the test if not using software single-stepping.  We
-# detect that by looking for 'target_resume (..., step)' in "debug
+# detect that by looking for 'to_resume (..., step)' in "debug
 # target" output.
 
 # Probe for software single-step breakpoint use.
@@ -62,7 +62,7 @@ gdb_test_no_output "set debug target 1"
 set hardware_step 0
 set test "probe target hardware step"
 gdb_test_multiple "si" $test {
-    -re "target_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
+    -re "to_resume \\(\[^\r\n\]+, step, .*$gdb_prompt $" {
 	set hardware_step 1
 	pass $test
     }