diff mbox

[review,Debugging,output] Make remote packet truncation length adjustable

Message ID gerrit.1574263562000.I2e871b37bfcaa6376537c3fe3db8f016dd806a7c@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 20, 2019, 3:26 p.m. UTC 
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................

[Debugging output] Make remote packet truncation length adjustable

While debugging, i felt the need to adjust the truncation length of remote
packets so i could see more or less data as needed. The default is currently
set to 512 bytes.

This patch makes this option adjustable through the new "set remote
packet-length-limit" command. It can be set to unlimited if we want to
completely disable truncation.

gdb/ChangeLog:

2019-11-20  Luis Machado  <luis.machado@linaro.org>

	* remote.c (REMOTE_DEBUG_MAX_CHAR): Remove.
	(remote_packet_length_limit): New static global.
	(show_packet_length_limit): New function.
	(remote_target::putpkt_binary): Adjust to use new
	adjustable option.
	(remote_target::getpkt_or_notif_sane_1): Likewise.
	(_initialize_remote): Register remote packet-length-limit option.

Signed-off-by: Luis Machado <luis.machado@linaro.org>
Change-Id: I2e871b37bfcaa6376537c3fe3db8f016dd806a7c
---
M gdb/remote.c
1 file changed, 42 insertions(+), 8 deletions(-)

Comments

Simon Marchi (Code Review) Nov. 20, 2019, 3:27 p.m. UTC | #1 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 1:

I'm not too sure packet-length-limit is good. Suggestions are welcome.
Simon Marchi (Code Review) Nov. 20, 2019, 6:12 p.m. UTC | #2 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> I'm not too sure packet-length-limit is good. Suggestions are welcome.

I tend to think something in the "set debug" namespace would be better,
since it's a setting related to "set debug remote".

How about "set debug remote-log-length"?

This patch also needs a documentation change and a NEWS entry.
Simon Marchi (Code Review) Nov. 20, 2019, 6:41 p.m. UTC | #3 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > I'm not too sure packet-length-limit is good. Suggestions are welcome.
> 
> I tend to think something in the "set debug" namespace would be better,
> since it's a setting related to "set debug remote".

I contemplated that, but upon looking at what sorts of options were available via "set debug", they were related to producing debugging output only, not adjusting how the debugging output was produced. So i went for more locality by putting it into "set remote".

To be honest, i don't think it fits in any of those two. But i'm okay with going for "set debug remote-log-length" based on feedback.

> 
> How about "set debug remote-log-length"?
> 
> This patch also needs a documentation change and a NEWS entry.

I'll put something together.
Simon Marchi (Code Review) Nov. 21, 2019, 2:41 p.m. UTC | #4 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

Thank you.  I have a small suggestion for the docs, but otherwise
everything looks good.

I'm marking this +1 since Eli should review the doc changes.

| --- gdb/doc/gdb.texinfo
| +++ gdb/doc/gdb.texinfo
| @@ -26273,7 +26273,20 @@ the serial line to the remote machine.  The info is printed on the
|  @value{GDBN} standard output stream. The default is off.
|  @item show debug remote
|  Displays the state of display of remote packets.
|  
| +@item set debug remote-log-length
| +Sets the number of characters to display for each remote packet when
| +@code{set debug remote} is on. This is useful to prevent GDB from
| +displaying lengthy remote packets and polluting the console.
| +
| +The default value is @code{512}, which means @value{GDBN} will truncate the
| +remote packet output after 512 bytes.

PS2, Line 26283:

I think maybe this should say something like "will truncate each
remote packet after 512 bytes" ... the current text makes it sound
a bit like the entirety of the output will be truncated.

| +
| +Setting this option to @code{unlimited} will disable truncation and will output
| +the full length of the remote packets.
| +@item show debug remote-log-length
| +Displays the number of bytes to output for remote packet debugging.
| +
|  @item set debug separate-debug-file
|  Turns on or off display of debug output about separate debug file search.
|  @item show debug separate-debug-file
Simon Marchi (Code Review) Nov. 21, 2019, 4:31 p.m. UTC | #5 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 3:

> > > I'm not too sure packet-length-limit is good. Suggestions are welcome.

I agree "set remote packet-length-limit" isn't good, since I'd expect a setting by that name to control the actual max size of gdb's packet buffer.

> > 
> > I tend to think something in the "set debug" namespace would be better,
> > since it's a setting related to "set debug remote".
> 
> I contemplated that, but upon looking at what sorts of options were available via "set debug", they were related to producing debugging output only, not adjusting how the debugging output was produced. So i went for more locality by putting it into "set remote".
> 
> To be honest, i don't think it fits in any of those two. But i'm okay with going for "set debug remote-log-length" based on feedback.
> 
> > 
> > How about "set debug remote-log-length"?
> > 
> > This patch also needs a documentation change and a NEWS entry.
> 
> I'll put something together.

I'm not thrilled with "remote-log-length" because "length" alone doesn't convey "max", or "limit".
Your original "packet-length-limit" did, but it was under the wrong namespace.

Including "log" in the setting name seems redundant, since "set debug" settings all control logging in some form.

"packet-length-limit" wasn't 100% correct, since it's not really the size of the packet that counts, it's how much you'd display, including escaping, but that seems like a minor issue.

Let's look at how the option is described:

 "Sets the number of characters to display for each remote packet"

Note that it doesn't talk about "log" and doesn't say "length".

If you start from that description, then the obvious setting name would be around:

  set debug remote-max-chars
  set debug remote-packet-max-characters
  set debug remote-packet-max-chars
  set debug remote-max-packet-chars

I think my preferred one would be "remote-packet-max-chars", since the limit only applies to packet logging.

Which is not unlike the existing macro name:

 /* The max number of chars in debug output.  The rest of chars are
    omitted.  */

 #define REMOTE_DEBUG_MAX_CHAR 512
Simon Marchi (Code Review) Nov. 21, 2019, 6:54 p.m. UTC | #6 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 4:

(5 comments)

Thanks.  A few comments more.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +19,19 @@ gdb/ChangeLog:
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* NEWS (New Commands): Mention "set debug remote-packet-max-chars".
| +	* remote.c (REMOTE_DEBUG_MAX_CHAR): Remove.
| +	(remote_packet_max_chars): New static global.
| +	(show_remote_packet_max_chars): New function.
| +	(remote_target::putpkt_binary): Adjust to use new
| +	remote_packet_max_chars option.
| +	(remote_target::getpkt_or_notif_sane_1): Likewise.
| +	(_initialize_remote): Register new remote_packet_max_chars option.

PS4, Line 28:

remote_packet_max_chars -> remote-packet-max-chars

| +
| +gdb/doc/ChangeLog:
| +
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb.texinfo (Debugging Output): Document set debug
| +	remote-packet-max-chars.
| +
| +Signed-off-by: Luis Machado <luis.machado@linaro.org>
| --- gdb/NEWS
| +++ gdb/NEWS
| @@ -183,14 +183,19 @@ info module variables [-q] [-m MODULE_REGEXP] [-t TYPE_REGEXP] [REGEXP]
|    Return a list of variables within all modules, grouped by module.
|    The list of variables can be restricted with the optional regular
|    expressions.  MODULE_REGEXP matches against the module name,
|    TYPE_REGEXP matches against the variable type, and REGEXP matches
|    against the variable name.
|  
| +set debug remote-packet-max-chars
| +show debug remote-packet-max-chars
| +  Controls the amount of characters to output when using "set debug remote".
| +  The default is 512 bytes.

PS4, Line 192:

Should mention "packet" somewhere in this description.  Maybe just
"amount of packet characters".

| +
|  * Changed commands
|  
|  help
|    The "help" command uses the title style to enhance the
|    readibility of its output by styling the classes and
|    command names.
|  
|  apropos [-v] REGEXP
| --- gdb/remote.c
| +++ gdb/remote.c
| @@ -1037,19 +1037,17 @@ /* For "set remote" and "show remote".  */
|  static struct cmd_list_element *remote_set_cmdlist;
|  static struct cmd_list_element *remote_show_cmdlist;
|  
|  /* Controls whether GDB is willing to use range stepping.  */
|  
|  static bool use_range_stepping = true;
|  
|  /* The max number of chars in debug output.  The rest of chars are
|     omitted.  */
|  

PS4, Line 1046:

This comment above...

| -#define REMOTE_DEBUG_MAX_CHAR 512
| -
|  /* Private data that we'll store in (struct thread_info)->priv.  */
|  struct remote_thread_info : public private_thread_info
|  {
|    std::string extra;
|    std::string name;
|    int core = -1;
|  

 ...

| @@ -1706,16 +1704,30 @@ /* Show the number of hardware breakpoints that can be used.  */
|  static void
|  show_hardware_breakpoint_limit (struct ui_file *file, int from_tty,
|  				struct cmd_list_element *c,
|  				const char *value)
|  {
|    fprintf_filtered (file, _("The maximum number of target hardware "
|  			    "breakpoints is %s.\n"), value);
|  }
|  
| +static int remote_packet_max_chars = 512;

PS4, Line 1713:

... should be moved here.

| +
| +/* Show the maximum number of characters to display for a remote packet when
| +   remote debugging is enabled.  */
| +
| +static void
| +show_remote_packet_max_chars (struct ui_file *file, int from_tty,
| +			      struct cmd_list_element *c,
| +			      const char *value)
| +{
| +  fprintf_filtered (file, _("Remote packet output will be truncated at %s "
| +			    "characters.\n"), value);

PS4, Line 1724:

Pedantically, this string doesn't work well with singular / 1
character.
Come to think of it, nor with "unlimited"?  Does it say:

  "Remote packet output will be truncated at unlimited characters."

Better rephrase that as:

  "something something something is %s.\n", value

| +}
| +
|  long
|  remote_target::get_memory_write_packet_size ()
|  {
|    return get_memory_packet_size (&memory_write_packet_config);
|  }
|  
|  static struct memory_packet_config memory_read_packet_config =
Simon Marchi (Code Review) Nov. 21, 2019, 9:49 p.m. UTC | #7 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 4:

(4 comments)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +19,19 @@ gdb/ChangeLog:
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* NEWS (New Commands): Mention "set debug remote-packet-max-chars".
| +	* remote.c (REMOTE_DEBUG_MAX_CHAR): Remove.
| +	(remote_packet_max_chars): New static global.
| +	(show_remote_packet_max_chars): New function.
| +	(remote_target::putpkt_binary): Adjust to use new
| +	remote_packet_max_chars option.
| +	(remote_target::getpkt_or_notif_sane_1): Likewise.
| +	(_initialize_remote): Register new remote_packet_max_chars option.

PS4, Line 28:

This was actually supposed to refer to the variable itself. Though
thinking again, it may be better to refer to the debug option itself.

| +
| +gdb/doc/ChangeLog:
| +
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb.texinfo (Debugging Output): Document set debug
| +	remote-packet-max-chars.
| +
| +Signed-off-by: Luis Machado <luis.machado@linaro.org>
| --- gdb/NEWS
| +++ gdb/NEWS
| @@ -183,14 +183,19 @@ info module variables [-q] [-m MODULE_REGEXP] [-t TYPE_REGEXP] [REGEXP]
|    Return a list of variables within all modules, grouped by module.
|    The list of variables can be restricted with the optional regular
|    expressions.  MODULE_REGEXP matches against the module name,
|    TYPE_REGEXP matches against the variable type, and REGEXP matches
|    against the variable name.
|  
| +set debug remote-packet-max-chars
| +show debug remote-packet-max-chars
| +  Controls the amount of characters to output when using "set debug remote".
| +  The default is 512 bytes.

PS4, Line 192:

Indeed. I'll fix this.

| +
|  * Changed commands
|  
|  help
|    The "help" command uses the title style to enhance the
|    readibility of its output by styling the classes and
|    command names.
|  
|  apropos [-v] REGEXP
| --- gdb/remote.c
| +++ gdb/remote.c
| @@ -1706,16 +1704,30 @@ /* Show the number of hardware breakpoints that can be used.  */
|  static void
|  show_hardware_breakpoint_limit (struct ui_file *file, int from_tty,
|  				struct cmd_list_element *c,
|  				const char *value)
|  {
|    fprintf_filtered (file, _("The maximum number of target hardware "
|  			    "breakpoints is %s.\n"), value);
|  }
|  
| +static int remote_packet_max_chars = 512;

PS4, Line 1713:

... and moved it here then. Thanks for spotting this.

| +
| +/* Show the maximum number of characters to display for a remote packet when
| +   remote debugging is enabled.  */
| +
| +static void
| +show_remote_packet_max_chars (struct ui_file *file, int from_tty,
| +			      struct cmd_list_element *c,
| +			      const char *value)
| +{
| +  fprintf_filtered (file, _("Remote packet output will be truncated at %s "
| +			    "characters.\n"), value);

PS4, Line 1724:

"Number of remote packet characters to display is %s.\n"?

| +}
| +
|  long
|  remote_target::get_memory_write_packet_size ()
|  {
|    return get_memory_packet_size (&memory_write_packet_config);
|  }
|  
|  static struct memory_packet_config memory_read_packet_config =
Simon Marchi (Code Review) Nov. 21, 2019, 9:49 p.m. UTC | #8 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 4:

(1 comment)

| --- gdb/remote.c
| +++ gdb/remote.c
| @@ -1037,19 +1037,17 @@ /* For "set remote" and "show remote".  */
|  static struct cmd_list_element *remote_set_cmdlist;
|  static struct cmd_list_element *remote_show_cmdlist;
|  
|  /* Controls whether GDB is willing to use range stepping.  */
|  
|  static bool use_range_stepping = true;
|  
|  /* The max number of chars in debug output.  The rest of chars are
|     omitted.  */
|  

PS4, Line 1046:

I took the liberty to make this comment a bit more grammatically
correct...

| -#define REMOTE_DEBUG_MAX_CHAR 512
| -
|  /* Private data that we'll store in (struct thread_info)->priv.  */
|  struct remote_thread_info : public private_thread_info
|  {
|    std::string extra;
|    std::string name;
|    int core = -1;
|
Simon Marchi (Code Review) Nov. 22, 2019, 3:22 p.m. UTC | #9 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 5: Code-Review+2

(2 comments)

Thanks.  This looks good.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +23,19 @@ gdb/ChangeLog:
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* NEWS (New Commands): Mention "set debug remote-packet-max-chars".
| +	* remote.c (REMOTE_DEBUG_MAX_CHAR): Remove.
| +	(remote_packet_max_chars): New static global.
| +	(show_remote_packet_max_chars): New function.
| +	(remote_target::putpkt_binary): Adjust to use new
| +	remote-packet-max-chars option.
| +	(remote_target::getpkt_or_notif_sane_1): Likewise.
| +	(_initialize_remote): Register new remote_packet_max_chars option.

PS5, Line 32:

> (_initialize_remote): Register new remote_packet_max_chars option.

I guess you mean to use hyphens here as well.  This is actually the
case that I was referring to earlier.

| +
| +gdb/doc/ChangeLog:
| +
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb.texinfo (Debugging Output): Document set debug
| +	remote-packet-max-chars.
| +
| +Signed-off-by: Luis Machado <luis.machado@linaro.org>
| --- gdb/remote.c
| +++ gdb/remote.c
| @@ -1715,7 +1715,19 @@ static int remote_packet_max_chars = 512;
| +/* Show the maximum number of characters to display for a remote packet when
| +   remote debugging is enabled.  */
| +
| +static void
| +show_remote_packet_max_chars (struct ui_file *file, int from_tty,
| +			      struct cmd_list_element *c,
| +			      const char *value)
| +{
| +  fprintf_filtered (file, _("Remote packet output will be truncated at %s "
| +			    "characters.\n"), value);

PS4, Line 1724:

> "Number of remote packet characters to display is %s.\n"?

That looks good.

| +}
| +
|  long
|  remote_target::get_memory_write_packet_size ()
|  {
|    return get_memory_packet_size (&memory_write_packet_config);
|  }
|  
|  static struct memory_packet_config memory_read_packet_config =
Simon Marchi (Code Review) Nov. 25, 2019, 1:33 p.m. UTC | #10 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/691
......................................................................


Patch Set 6:

(1 comment)

Thanks for the review. I'll push this later today.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +23,19 @@ gdb/ChangeLog:
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* NEWS (New Commands): Mention "set debug remote-packet-max-chars".
| +	* remote.c (REMOTE_DEBUG_MAX_CHAR): Remove.
| +	(remote_packet_max_chars): New static global.
| +	(show_remote_packet_max_chars): New function.
| +	(remote_target::putpkt_binary): Adjust to use new
| +	remote-packet-max-chars option.
| +	(remote_target::getpkt_or_notif_sane_1): Likewise.
| +	(_initialize_remote): Register new remote_packet_max_chars option.

PS5, Line 32:

Ah, got it. Sorry, i missed this in e-mail form for some reason.

| +
| +gdb/doc/ChangeLog:
| +
| +2019-11-21  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb.texinfo (Debugging Output): Document set debug
| +	remote-packet-max-chars.
| +
| +Signed-off-by: Luis Machado <luis.machado@linaro.org>
diff mbox

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 1ac9013..80457ad 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1044,8 +1044,6 @@ 
 /* The max number of chars in debug output.  The rest of chars are
    omitted.  */
 
-#define REMOTE_DEBUG_MAX_CHAR 512
-
 /* Private data that we'll store in (struct thread_info)->priv.  */
 struct remote_thread_info : public private_thread_info
 {
@@ -1712,6 +1710,20 @@ 
 			    "breakpoints is %s.\n"), value);
 }
 
+static int remote_packet_length_limit = 512;
+
+/* Show the maximum number of characters to display for a remote packet when
+   remote debugging is enabled.  */
+
+static void
+show_packet_length_limit (struct ui_file *file, int from_tty,
+			  struct cmd_list_element *c,
+			  const char *value)
+{
+  fprintf_filtered (file, _("Remote packet output will be truncated at %s "
+			    "characters.\n"), value);
+}
+
 long
 remote_target::get_memory_write_packet_size ()
 {
@@ -9119,15 +9131,21 @@ 
 	  *p = '\0';
 
 	  int len = (int) (p - buf2);
+	  int max_chars;
+
+	  if (remote_packet_length_limit < 0)
+	    max_chars = len;
+	  else
+	    max_chars = remote_packet_length_limit;
 
 	  std::string str
-	    = escape_buffer (buf2, std::min (len, REMOTE_DEBUG_MAX_CHAR));
+	    = escape_buffer (buf2, std::min (len, max_chars));
 
 	  fprintf_unfiltered (gdb_stdlog, "Sending packet: %s", str.c_str ());
 
-	  if (len > REMOTE_DEBUG_MAX_CHAR)
+	  if (len > max_chars)
 	    fprintf_unfiltered (gdb_stdlog, "[%d bytes omitted]",
-				len - REMOTE_DEBUG_MAX_CHAR);
+				len - max_chars);
 
 	  fprintf_unfiltered (gdb_stdlog, "...");
 
@@ -9563,16 +9581,23 @@ 
 	{
 	  if (remote_debug)
 	    {
+	      int max_chars;
+
+	      if (remote_packet_length_limit < 0)
+		max_chars = val;
+	      else
+		max_chars = remote_packet_length_limit;
+
 	      std::string str
 		= escape_buffer (buf->data (),
-				 std::min (val, REMOTE_DEBUG_MAX_CHAR));
+				 std::min (val, max_chars));
 
 	      fprintf_unfiltered (gdb_stdlog, "Packet received: %s",
 				  str.c_str ());
 
-	      if (val > REMOTE_DEBUG_MAX_CHAR)
+	      if (val > max_chars)
 		fprintf_unfiltered (gdb_stdlog, "[%d bytes omitted]",
-				    val - REMOTE_DEBUG_MAX_CHAR);
+				    val - max_chars);
 
 	      fprintf_unfiltered (gdb_stdlog, "\n");
 	    }
@@ -14723,6 +14748,15 @@ 
 			    show_watchdog,
 			    &setlist, &showlist);
 
+  add_setshow_zuinteger_unlimited_cmd ("packet-length-limit", no_class,
+				      &remote_packet_length_limit, _("\
+Set the maximum number of characters to display for remote packet debugging."), _("\
+Show the maximum number of characters to display for remote packet debugging."), _("\
+Specify \"unlimited\" to display all the characters."),
+				      NULL, show_packet_length_limit,
+				      &remote_set_cmdlist,
+				      &remote_show_cmdlist);
+
   /* Eventually initialize fileio.  See fileio.c */
   initialize_remote_fileio (remote_set_cmdlist, remote_show_cmdlist);
 }