[PATCH] Don't print too much if remote_debug is on

[PATCH] Don't print too much if remote_debug is on

[Yao Qi]

If we turn "remote debug" on and GDB does some vFile operations,
a lot of things will be printed in the screen, which makes
"remote debug" useless.

This patch changes the code that we don't print messages if
messages are too long, greater than 512.  Instead, print
"Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]".

gdb:

2016-11-29  Yao Qi  <yao.qi@linaro.org>

	* remote.c (REMOTE_DEBUG_MAX_CHAR): New macro.
	(putpkt_binary): Print if content length is less than
	REMOTE_DEBUG_MAX_CHAR.
	(getpkt_or_notif_sane_1): Likewise.
---
 gdb/remote.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index ef6c54e..7b4c168 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -283,6 +283,11 @@ typedef unsigned char threadref[OPAQUETHREADBYTES];
 
 #define MAXTHREADLISTRESULTS 32
 
+/* The max number of chars in debug output.  Output more than this number
+   of chars is omitted.  */
+
+#define REMOTE_DEBUG_MAX_CHAR 512
+
 /* Data for the vFile:pread readahead cache.  */
 
 struct readahead_cache
@@ -8749,9 +8754,16 @@ putpkt_binary (const char *buf, int cnt)
 	{
 	  *p = '\0';
 
-	  std::string str = escape_buffer (buf2, p - buf2);
+	  fprintf_unfiltered (gdb_stdlog, "Sending packet: ");
+	  ptrdiff_t len = p - buf2;
+	  if (len <= REMOTE_DEBUG_MAX_CHAR)
+	    {
+	      std::string str = escape_buffer (buf2, len);
 
-	  fprintf_unfiltered (gdb_stdlog, "Sending packet: %s...", str.c_str ());
+	      fprintf_unfiltered (gdb_stdlog, "%s...", str.c_str ());
+	    }
+	  else
+	    fprintf_unfiltered (gdb_stdlog, "[%td bytes omitted]...", len);
 	  gdb_flush (gdb_stdlog);
 	}
       remote_serial_write (buf2, p - buf2);
@@ -9179,9 +9191,15 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
 	{
 	  if (remote_debug)
 	    {
-	      std::string str = escape_buffer (*buf, val);
+	      fprintf_unfiltered (gdb_stdlog, "Packet received: ");
+	      if (val <= REMOTE_DEBUG_MAX_CHAR)
+		{
+		  std::string str = escape_buffer (*buf, val);
 
-	      fprintf_unfiltered (gdb_stdlog, "Packet received: %s\n", str.c_str ());
+		  fprintf_unfiltered (gdb_stdlog, "%s\n", str.c_str ());
+		}
+	      else
+		fprintf_unfiltered (gdb_stdlog, "[%d bytes omitted]\n", val);
 	    }
 
 	  /* Skip the ack char if we're in no-ack mode.  */
-- 
1.9.1

Re: [PATCH] Don't print too much if remote_debug is on

[Luis Machado]

On 11/29/2016 09:38 AM, Yao Qi wrote:
> If we turn "remote debug" on and GDB does some vFile operations,
> a lot of things will be printed in the screen, which makes
> "remote debug" useless.
>
> This patch changes the code that we don't print messages if
> messages are too long, greater than 512.  Instead, print
> "Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]".

How about not printing binary data at all and instead print some useful 
information about contents being sent? Like the number of bytes and 
maybe how many still need to be read?

Re: [PATCH] Don't print too much if remote_debug is on

[Yao Qi]

On Tue, Nov 29, 2016 at 09:45:59AM -0600, Luis Machado wrote:
> On 11/29/2016 09:38 AM, Yao Qi wrote:
> >If we turn "remote debug" on and GDB does some vFile operations,
> >a lot of things will be printed in the screen, which makes
> >"remote debug" useless.
> >
> >This patch changes the code that we don't print messages if
> >messages are too long, greater than 512.  Instead, print
> >"Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]".
> 
> How about not printing binary data at all and instead print some
> useful information about contents being sent? Like the number of
> bytes and maybe how many still need to be read?

In putpkt/getpkt, we don't know the data is binary or not, unless we
pass an additional parameter to indicate this.  Then, we need to
go through every calls to putpkt/getpkt, check the data is plain text
or binary, so pass the right value to putpkt/getpkt.

The binary and plain data is mixed in the buffer in some packets, like
"vFile:pwrite: fd, offset, data".  If we want to print
"Sending packet: $vFile:pwrite:5,e0d12,[16384 bytes]#c4" in the debug
output, we need to move the debugging output from buffer level to
packet level.  I agree it is better than
"Sending packet: [16384 bytes omitted]" which is what my patch does.

We can omit the received packet if it is more than REMOTE_DEBUG_MAX_CHAR
chars; if the sent packet is more than REMOTE_DEBUG_MAX_CHAR chars, only
print the first 50 chars, and omit the rest of them, so the debug
output is like,

Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]
Sending packet: $vFile:pwrite:5,e0d12,xxxyyyzzz[384 bytes omitted] ... Packet received: 358

What do you think?
-- 
Yao (齐尧)

Re: [PATCH] Don't print too much if remote_debug is on

[Luis Machado]

On 11/30/2016 06:54 AM, Yao Qi wrote:
> On Tue, Nov 29, 2016 at 09:45:59AM -0600, Luis Machado wrote:
>> On 11/29/2016 09:38 AM, Yao Qi wrote:
>>> If we turn "remote debug" on and GDB does some vFile operations,
>>> a lot of things will be printed in the screen, which makes
>>> "remote debug" useless.
>>>
>>> This patch changes the code that we don't print messages if
>>> messages are too long, greater than 512.  Instead, print
>>> "Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]".
>>
>> How about not printing binary data at all and instead print some
>> useful information about contents being sent? Like the number of
>> bytes and maybe how many still need to be read?
>
> In putpkt/getpkt, we don't know the data is binary or not, unless we
> pass an additional parameter to indicate this.  Then, we need to
> go through every calls to putpkt/getpkt, check the data is plain text
> or binary, so pass the right value to putpkt/getpkt.

Ah, that's unfortunate.

>
> The binary and plain data is mixed in the buffer in some packets, like
> "vFile:pwrite: fd, offset, data".  If we want to print
> "Sending packet: $vFile:pwrite:5,e0d12,[16384 bytes]#c4" in the debug
> output, we need to move the debugging output from buffer level to
> packet level.  I agree it is better than
> "Sending packet: [16384 bytes omitted]" which is what my patch does.
>
> We can omit the received packet if it is more than REMOTE_DEBUG_MAX_CHAR
> chars; if the sent packet is more than REMOTE_DEBUG_MAX_CHAR chars, only
> print the first 50 chars, and omit the rest of them, so the debug
> output is like,
>
> Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]
> Sending packet: $vFile:pwrite:5,e0d12,xxxyyyzzz[384 bytes omitted] ... Packet received: 358
>
> What do you think?
>

I think it is an improvement nonetheless. Personally i still find 
particular lengthy replies useful, like the XML descriptions. But all 
the binary data is too distracting, hence why i was suggesting only 
binary streams being restricted.

I'm fine with your version.

Re: [PATCH] Don't print too much if remote_debug is on

[Pedro Alves]

On 11/30/2016 12:54 PM, Yao Qi wrote:
> On Tue, Nov 29, 2016 at 09:45:59AM -0600, Luis Machado wrote:
>> On 11/29/2016 09:38 AM, Yao Qi wrote:
>>> If we turn "remote debug" on and GDB does some vFile operations,
>>> a lot of things will be printed in the screen, which makes
>>> "remote debug" useless.
>>>
>>> This patch changes the code that we don't print messages if
>>> messages are too long, greater than 512.  Instead, print
>>> "Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]".
>>
>> How about not printing binary data at all and instead print some
>> useful information about contents being sent? Like the number of
>> bytes and maybe how many still need to be read?
> 
> In putpkt/getpkt, we don't know the data is binary or not, unless we
> pass an additional parameter to indicate this.  Then, we need to
> go through every calls to putpkt/getpkt, check the data is plain text
> or binary, so pass the right value to putpkt/getpkt.
> 
> The binary and plain data is mixed in the buffer in some packets, like
> "vFile:pwrite: fd, offset, data".  If we want to print
> "Sending packet: $vFile:pwrite:5,e0d12,[16384 bytes]#c4" in the debug
> output, we need to move the debugging output from buffer level to
> packet level.  I agree it is better than
> "Sending packet: [16384 bytes omitted]" which is what my patch does.
> 
> We can omit the received packet if it is more than REMOTE_DEBUG_MAX_CHAR
> chars; if the sent packet is more than REMOTE_DEBUG_MAX_CHAR chars, only
> print the first 50 chars, and omit the rest of them, so the debug
> output is like,
> 
> Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]
> Sending packet: $vFile:pwrite:5,e0d12,xxxyyyzzz[384 bytes omitted] ... Packet received: 358
> 
> What do you think?

I was just now writing a reply to the OP to suggest something like
that, not noticing this reply.  So +1 from me.

But I'm not sure I follow the "50" in "50 chars".
Why not instead:

 If REMOTE_DEBUG_MAX_CHAR is more than REMOTE_DEBUG_MAX_CHAR chars,
 only print the first REMOTE_DEBUG_MAX_CHAR chars.

Thanks,
Pedro Alves

Re: [PATCH] Don't print too much if remote_debug is on

[Gary Benson]

Luis Machado wrote:
> On 11/30/2016 06:54 AM, Yao Qi wrote:
> > The binary and plain data is mixed in the buffer in some packets, like
> > "vFile:pwrite: fd, offset, data".  If we want to print
> > "Sending packet: $vFile:pwrite:5,e0d12,[16384 bytes]#c4" in the debug
> > output, we need to move the debugging output from buffer level to
> > packet level.  I agree it is better than
> > "Sending packet: [16384 bytes omitted]" which is what my patch does.
> > 
> > We can omit the received packet if it is more than REMOTE_DEBUG_MAX_CHAR
> > chars; if the sent packet is more than REMOTE_DEBUG_MAX_CHAR chars, only
> > print the first 50 chars, and omit the rest of them, so the debug
> > output is like,
> > 
> > Sending packet: $vFile:pread:5,3fff,e0d12#c4...Packet received: [16384 bytes omitted]
> > Sending packet: $vFile:pwrite:5,e0d12,xxxyyyzzz[384 bytes omitted] ... Packet received: 358
> > 
> > What do you think?
> 
> I think it is an improvement nonetheless. Personally i still find
> particular lengthy replies useful, like the XML descriptions. But
> all the binary data is too distracting, hence why i was suggesting
> only binary streams being restricted.
> 
> I'm fine with your version.

I haven't checked, but it might be trivial to spot and not strip XML
by looking for "<?xml" in the first few bytes if the packet.

However this happens, removing the huge binary packets gets my +1.
(I know I'm responsible for a large increase in those recently,
sorry!)

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH V2] Don't print too much if remote_debug is on

[Yao Qi]

v2: Only print the first REMOTE_DEBUG_MAX_CHAR chars in debug
    output.

If we turn "remote debug" on and GDB does some vFile operations,
a lot of things will be printed in the screen, which makes
"remote debug" useless.

This patch changes the code that we only print 512 chars in max in
debugging messages, like this,

Sending packet: $qXfer:features:read:target.xml:0,fff#7d...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2010-2016 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!-- AMD64 with AVX - Includes Linux-only special "register".  -->\n\n<!DOCTYPE target SYSTEM "gdb-target.dtd">\n<target>\n  <architecture>i386:x86-64</architecture>\n  <osabi>GNU/Linux</osabi>\n  <xi:include href="64bit-core.xml"/>\n  <xi:[14 bytes omitted]

Sending packet: $qXfer:auxv:read::0,1000#6b...Packet received: l!\000\000\000\000\000\000\000\000d\000\000\000\000\000\000\000\003\000\000\000\000\000\000\000@\000@\000\000\000\000\000\004\000\000\000\000\000\000\0008\000\000\000\000\000\000\000\005\000\000\000\000\000\000\000\t\000\000\000\000\000\000\000\a\000\000\000\000\000\000\000\177\000\000\b\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\t\000\000\000\000\000\000\000\000\004@\000\000\000\000\000\013\000\000\000\000\000\000\003\000\000\000\000\000\000\f\000\000\000\000\000\000\003\000\000\000\000\000\000\r\000\000\000\000\000\000\003\000\000\000\000\000\000\016\000\000\000\000\000\000\003\000\000\000\000\000\000\027\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\031\000\000\000\000\000\000\177\000\000\037\000\000\000\000\000\000\000\000\017\000\000\000\000\000\000\00\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000[582 bytes omitted]

gdb:

2016-12-06  Yao Qi  <yao.qi@linaro.org>

	* remote.c (REMOTE_DEBUG_MAX_CHAR): New macro.
	(putpkt_binary): Print only REMOTE_DEBUG_MAX_CHAR chars in debug
	output.
	(getpkt_or_notif_sane_1): Likewise.
---
 gdb/remote.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index ef6c54e..6833038 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -283,6 +283,11 @@ typedef unsigned char threadref[OPAQUETHREADBYTES];
 
 #define MAXTHREADLISTRESULTS 32
 
+/* The max number of chars in debug output.  The rest of chars are
+   omitted.  */
+
+#define REMOTE_DEBUG_MAX_CHAR 512
+
 /* Data for the vFile:pread readahead cache.  */
 
 struct readahead_cache
@@ -8749,9 +8754,21 @@ putpkt_binary (const char *buf, int cnt)
 	{
 	  *p = '\0';
 
-	  std::string str = escape_buffer (buf2, p - buf2);
+	  int len = (int) (p - buf2);
+
+	  std::string str
+	    = escape_buffer (buf2, std::min (len, REMOTE_DEBUG_MAX_CHAR));
+
+	  fprintf_unfiltered (gdb_stdlog, "Sending packet: %s", str.c_str ());
+
+	  if (str.length () > REMOTE_DEBUG_MAX_CHAR)
+	    {
+	      fprintf_unfiltered (gdb_stdlog, "[%zu bytes omitted]",
+				  str.length () - REMOTE_DEBUG_MAX_CHAR);
+	    }
+
+	  fprintf_unfiltered (gdb_stdlog, "...");
 
-	  fprintf_unfiltered (gdb_stdlog, "Sending packet: %s...", str.c_str ());
 	  gdb_flush (gdb_stdlog);
 	}
       remote_serial_write (buf2, p - buf2);
@@ -9179,9 +9196,20 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
 	{
 	  if (remote_debug)
 	    {
-	      std::string str = escape_buffer (*buf, val);
+	      std::string str
+		= escape_buffer (*buf,
+				 std::min (val, REMOTE_DEBUG_MAX_CHAR));
+
+	      fprintf_unfiltered (gdb_stdlog, "Packet received: %s",
+				  str.c_str ());
+
+	      if (str.length () >  REMOTE_DEBUG_MAX_CHAR)
+		{
+		  fprintf_unfiltered (gdb_stdlog, "[%zu bytes omitted]",
+				      str.length () - REMOTE_DEBUG_MAX_CHAR);
+		}
 
-	      fprintf_unfiltered (gdb_stdlog, "Packet received: %s\n", str.c_str ());
+	      fprintf_unfiltered (gdb_stdlog, "\n");
 	    }
 
 	  /* Skip the ack char if we're in no-ack mode.  */
-- 
1.9.1

Re: [PATCH V2] Don't print too much if remote_debug is on

[Yao Qi]

On 16-12-06 14:21:58, Yao Qi wrote:
> gdb:
> 
> 2016-12-06  Yao Qi  <yao.qi@linaro.org>
> 
> 	* remote.c (REMOTE_DEBUG_MAX_CHAR): New macro.
> 	(putpkt_binary): Print only REMOTE_DEBUG_MAX_CHAR chars in debug
> 	output.
> 	(getpkt_or_notif_sane_1): Likewise.

I pushed it in.

-- 
Yao (齐尧)