[patch] Disassembly improvements

[patch] Disassembly improvements

[Abid, Hafiz]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

Hi Pedro,
I am attaching the patch that was mentioned in the following thread. I resurrected it from our internal repo, did a bit of manual testing and run the regression suite without any problem. It basically reads memory from the target in a buffer in gdb_disassembly and tries to use this buffer in dis_asm_read_memory instead of reading from the target. This saves us on repeated memory read calls. The problem was noted when eclipse was trying to fill its disassembly view.
https://sourceware.org/ml/gdb-patches/2013-10/msg00221.html

Regards,
Abid

2013-10-10  Taimoor Mirza  <taimoor_mirza@mentor.com>

	* disasm.c (DIS_BUF_SIZE): New define.
	(dis_asm_read_memory): Read from the disassembly buffer instead
	of target memory directly.
	(gdb_disassembly): Fill the disassembly buffer with a chunk of
	the memory to disassemble.



[-- Attachment #2: disasm.patch --]
[-- Type: application/octet-stream, Size: 2939 bytes --]

diff --git a/gdb/disasm.c b/gdb/disasm.c
index e643c2d..daedc98 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -42,11 +42,62 @@ struct dis_line_entry
   CORE_ADDR end_pc;
 };
 
+/* Size of the disassembly memory buffer.  */
+#define DIS_BUF_SIZE 1024
+
 /* Like target_read_memory, but slightly different parameters.  */
 static int
 dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
 		     struct disassemble_info *info)
 {
+  /* Assume the disassembler always read memory forwards.  If we
+     failed to read a buffer line in a previous call, assume we're
+     reading close to the end of a mapped page or section, and so it's
+     useless to keep retrying reading that buffer line.  Simply
+     fallback to reading directly from target memory.  */
+  if (info->buffer_length > 0)
+    {
+      while (len)
+	{
+	  if (memaddr >= info->buffer_vma
+	      && memaddr < info->buffer_vma + info->buffer_length)
+	    {
+	      unsigned int offset = (memaddr - info->buffer_vma);
+	      unsigned int l = min (len, info->buffer_length - offset);
+
+	      memcpy (myaddr, info->buffer + offset, l);
+
+	      memaddr += l;
+	      myaddr += l;
+	      len -= l;
+
+	      if (len == 0)
+		return 0;
+	    }
+	  else
+	    {
+	      int rval;
+	      unsigned int len = info->buffer_length;
+
+	      /* Try fetching a new buffer line from the target.  */
+
+	      /* If we fail reading memory halfway, we'll have clobbered
+		 the buffer, so don't trust it anymore, even on fail.  */
+	      info->buffer_length = 0;
+	      rval = target_read_memory (memaddr, info->buffer, len);
+	      if (rval == 0)
+		{
+		  info->buffer_vma = memaddr;
+		  info->buffer_length = len;
+		}
+	      else
+		{
+		  /* Read from target memory directly from now on.  */
+		  break;
+		}
+	    }
+	}
+    }
   return target_read_memory (memaddr, myaddr, len);
 }
 
@@ -415,6 +466,10 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   struct symtab *symtab = NULL;
   struct linetable_entry *le = NULL;
   int nlines = -1;
+  int buff_size = DIS_BUF_SIZE;
+  int req_size = high - low;
+  int err = 0;
+  gdb_byte *pbuffer = NULL;
 
   /* Assume symtab is valid for whole PC range.  */
   symtab = find_pc_symtab (low);
@@ -425,6 +480,23 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       le = symtab->linetable->item;
       nlines = symtab->linetable->nitems;
     }
+  if (req_size < buff_size)
+    buff_size = req_size;
+
+  if (req_size > 0)
+    {
+      /* Allocate buffer and read memory region in buffer.  */
+      pbuffer = xmalloc (buff_size);
+      make_cleanup (xfree, pbuffer);
+      err = target_read_memory (low, pbuffer, buff_size);
+      if (err == 0)
+	{
+	  di.buffer = pbuffer;
+	  di.buffer_length = buff_size;
+	  di.buffer_vma = low;
+	}
+    }
+
 
   if (!(flags & DISASSEMBLY_SOURCE) || nlines <= 0
       || symtab == NULL || symtab->linetable == NULL)

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/10/2013 02:14 PM, Abid, Hafiz wrote:
> Hi Pedro,
> I am attaching the patch that was mentioned in the following thread. I resurrected it from our internal repo, did a bit of manual testing and run the regression suite without any problem. It basically reads memory from the target in a buffer in gdb_disassembly and tries to use this buffer in dis_asm_read_memory instead of reading from the target. This saves us on repeated memory read calls. The problem was noted when eclipse was trying to fill its disassembly view.
> https://sourceware.org/ml/gdb-patches/2013-10/msg00221.html

Thanks.

> +  /* Assume the disassembler always read memory forwards.  If we

"always reads"

> +     failed to read a buffer line in a previous call, assume we're
> +     reading close to the end of a mapped page or section, and so it's
> +     useless to keep retrying reading that buffer line.  Simply
> +     fallback to reading directly from target memory.  */

Should be "retrying to read", I think.


> +  if (info->buffer_length > 0)
> +    {
> +      while (len)
> +	{
> +	  if (memaddr >= info->buffer_vma
> +	      && memaddr < info->buffer_vma + info->buffer_length)
> +	    {
> +	      unsigned int offset = (memaddr - info->buffer_vma);
> +	      unsigned int l = min (len, info->buffer_length - offset);
> +
> +	      memcpy (myaddr, info->buffer + offset, l);
> +
> +	      memaddr += l;
> +	      myaddr += l;
> +	      len -= l;
> +
> +	      if (len == 0)
> +		return 0;
> +	    }
> +	  else
> +	    {
> +	      int rval;
> +	      unsigned int len = info->buffer_length;
> +
> +	      /* Try fetching a new buffer line from the target.  */

Hmm, this seems to miss making sure LEN doesn't read beyond the
original requested memory range.  It'd be good to add that.

That "unsigned int len" variable shadows the function's "len"
parameter.  It'd be good to rename it.

> +
> +	      /* If we fail reading memory halfway, we'll have clobbered
> +		 the buffer, so don't trust it anymore, even on fail.  */
> +	      info->buffer_length = 0;
> +	      rval = target_read_memory (memaddr, info->buffer, len);
> +	      if (rval == 0)
> +		{
> +		  info->buffer_vma = memaddr;
> +		  info->buffer_length = len;
> +		}
> +	      else
> +		{
> +		  /* Read from target memory directly from now on.  */
> +		  break;
> +		}
> +	    }
> +	}
> +    }
>    return target_read_memory (memaddr, myaddr, len);
>  }


-- 
Pedro Alves

RE: [patch] Disassembly improvements

[Abid, Hafiz]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

Thanks for review. Here is updated patch.

> > +  /* Assume the disassembler always read memory forwards.  If we
> "always reads"
Fixed.

> > +     useless to keep retrying reading that buffer line.  Simply
> > +     fallback to reading directly from target memory.  */
> 
> Should be "retrying to read", I think.
Fixed.

> > +	      /* Try fetching a new buffer line from the target.  */
> 
> Hmm, this seems to miss making sure LEN doesn't read beyond the original
> requested memory range.  It'd be good to add that.
Changed to following line which should take care of this.
unsigned int length = min (len, info->buffer_length);

> 
> That "unsigned int len" variable shadows the function's "len"
> parameter.  It'd be good to rename it.
Fixed.

> 
> 
> --
> Pedro Alves

[-- Attachment #2: disasm.patch --]
[-- Type: application/octet-stream, Size: 2960 bytes --]

diff --git a/gdb/disasm.c b/gdb/disasm.c
index e643c2d..7e3d908 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -42,11 +42,62 @@ struct dis_line_entry
   CORE_ADDR end_pc;
 };
 
+/* Size of the disassembly memory buffer.  */
+#define DIS_BUF_SIZE 1024
+
 /* Like target_read_memory, but slightly different parameters.  */
 static int
 dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
 		     struct disassemble_info *info)
 {
+  /* Assume the disassembler always reads memory forwards.  If we
+     failed to read a buffer line in a previous call, assume we're
+     reading close to the end of a mapped page or section, and so it's
+     useless to keep retrying to read that buffer line.  Simply
+     fallback to reading directly from target memory.  */
+  if (info->buffer_length > 0)
+    {
+      while (len)
+	{
+	  if (memaddr >= info->buffer_vma
+	      && memaddr < info->buffer_vma + info->buffer_length)
+	    {
+	      unsigned int offset = (memaddr - info->buffer_vma);
+	      unsigned int l = min (len, info->buffer_length - offset);
+
+	      memcpy (myaddr, info->buffer + offset, l);
+
+	      memaddr += l;
+	      myaddr += l;
+	      len -= l;
+
+	      if (len == 0)
+		return 0;
+	    }
+	  else
+	    {
+	      int rval;
+	      unsigned int length = min (len, info->buffer_length);
+
+	      /* Try fetching a new buffer line from the target.  */
+
+	      /* If we fail reading memory halfway, we'll have clobbered
+		 the buffer, so don't trust it anymore, even on fail.  */
+	      info->buffer_length = 0;
+	      rval = target_read_memory (memaddr, info->buffer, length);
+	      if (rval == 0)
+		{
+		  info->buffer_vma = memaddr;
+		  info->buffer_length = length;
+		}
+	      else
+		{
+		  /* Read from target memory directly from now on.  */
+		  break;
+		}
+	    }
+	}
+    }
   return target_read_memory (memaddr, myaddr, len);
 }
 
@@ -415,6 +466,10 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   struct symtab *symtab = NULL;
   struct linetable_entry *le = NULL;
   int nlines = -1;
+  int buff_size = DIS_BUF_SIZE;
+  int req_size = high - low;
+  int err = 0;
+  gdb_byte *pbuffer = NULL;
 
   /* Assume symtab is valid for whole PC range.  */
   symtab = find_pc_symtab (low);
@@ -425,6 +480,23 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       le = symtab->linetable->item;
       nlines = symtab->linetable->nitems;
     }
+  if (req_size < buff_size)
+    buff_size = req_size;
+
+  if (req_size > 0)
+    {
+      /* Allocate buffer and read memory region in buffer.  */
+      pbuffer = xmalloc (buff_size);
+      make_cleanup (xfree, pbuffer);
+      err = target_read_memory (low, pbuffer, buff_size);
+      if (err == 0)
+	{
+	  di.buffer = pbuffer;
+	  di.buffer_length = buff_size;
+	  di.buffer_vma = low;
+	}
+    }
+
 
   if (!(flags & DISASSEMBLY_SOURCE) || nlines <= 0
       || symtab == NULL || symtab->linetable == NULL)

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/10/2013 02:57 PM, Abid, Hafiz wrote:
>> > 
>> > Hmm, this seems to miss making sure LEN doesn't read beyond the original
>> > requested memory range.  It'd be good to add that.
> Changed to following line which should take care of this.
> unsigned int length = min (len, info->buffer_length);

But it seems to me that will just disable the optimization for
buffer line > 1.

LEN here I think will the disassembler considers to be the maximum
length of an instruction for the arquitecture it is disassembling.
We want to read _more_ than that from memory in one go, otherwise, we'll
not be buffering anything.  What we do not want, is for that over
fetching to read beyond the range that was passed to gdb_disassembly.

I think we'll need to derive from "struct disassemble_info",
and add the original range to that new struct, or record that info
directly in "struct disassemble_info", which is in include/dis-asm.h.

-- 
Pedro Alves

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/10/2013 03:52 PM, Pedro Alves wrote:
> On 10/10/2013 02:57 PM, Abid, Hafiz wrote:
>>>>
>>>> Hmm, this seems to miss making sure LEN doesn't read beyond the original
>>>> requested memory range.  It'd be good to add that.
>> Changed to following line which should take care of this.
>> unsigned int length = min (len, info->buffer_length);
> 
> But it seems to me that will just disable the optimization for
> buffer line > 1.
> 
> LEN here I think will the disassembler considers to be the maximum
> length of an instruction for the arquitecture it is disassembling.
> We want to read _more_ than that from memory in one go, otherwise, we'll
> not be buffering anything.  What we do not want, is for that over
> fetching to read beyond the range that was passed to gdb_disassembly.
> 
> I think we'll need to derive from "struct disassemble_info",
> and add the original range to that new struct, or record that info
> directly in "struct disassemble_info", which is in include/dis-asm.h.

I now noticed there's a struct disassemble_info->application_data field,
which GDB currently uses to put the gdbarch in.  We could put a
disasm.c specific structure there instead.

-- 
Pedro Alves

RE: [patch] Disassembly improvements

[Abid, Hafiz]

> > But it seems to me that will just disable the optimization for buffer
> > line > 1.
> >
> > LEN here I think will the disassembler considers to be the maximum
> > length of an instruction for the arquitecture it is disassembling.
> > We want to read _more_ than that from memory in one go, otherwise,
> > we'll not be buffering anything.  What we do not want, is for that
> > over fetching to read beyond the range that was passed to
> gdb_disassembly.
We come in the else case only if we have exhausted the initial buffer allocated
in gdb_disassembly. You are right that current code will read LEN bytes in that case
instead of reading the larger buffer but I think that should not be very common and
we don't lose the optimization gained from the buffering for the first ' DIS_BUF_SIZE'
bytes. So I think this patch is useful on its own and the other enhancement can be built
on top of it.

Also if user provides a range of address to the disassembly command then it is possible that
end address is in the middle of the instruction and gdb will end up reading beyond the end
address given in the command. So we probably cannot forbid that here anyway.

(gdb) disassemble /r $pc,$pc+12
Dump of assembler code from 0x40292c to 0x402938:
=> 0x000000000040292c <main+8>:	c7 45 fc 00 00 00 00	movl   $0x0,-0x4(%rbp)
   0x0000000000402933 <main+15>:	eb 78	jmp    0x4029ad <main+137>
   0x0000000000402935 <main+17>:	8b 15 f5 18 20 00	mov    0x2018f5(%rip),%edx        # 0x604230 <loops>

As you can see gdb reading beyond 0x402938 here.

> > I think we'll need to derive from "struct disassemble_info", and add
> > the original range to that new struct, or record that info directly in
> > "struct disassemble_info", which is in include/dis-asm.h.
> 
> I now noticed there's a struct disassemble_info->application_data field,
> which GDB currently uses to put the gdbarch in.  We could put a disasm.c
> specific structure there instead.
I noticed that this field is already being used in spu-tdep.c. Any changes
to it will break that code. We can add new fields but as I described above,
we may not want to check for the high address of the request.

Thanks,
Abid

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/11/2013 05:45 PM, Abid, Hafiz wrote:
> 
>>> But it seems to me that will just disable the optimization for buffer
>>> line > 1.
>>>
>>> LEN here I think will the disassembler considers to be the maximum
>>> length of an instruction for the arquitecture it is disassembling.
>>> We want to read _more_ than that from memory in one go, otherwise,
>>> we'll not be buffering anything.  What we do not want, is for that
>>> over fetching to read beyond the range that was passed to
>> gdb_disassembly.
> We come in the else case only if we have exhausted the initial buffer allocated
> in gdb_disassembly. You are right that current code will read LEN bytes in that case
> instead of reading the larger buffer but I think that should not be very common and
> we don't lose the optimization gained from the buffering for the first ' DIS_BUF_SIZE'
> bytes. So I think this patch is useful on its own and the other enhancement can be built
> on top of it.

But then that makes for a sort of a half baked implementation.

I just tried "disassemble main" against gdbserver, and I got more than
DIS_BUF_SIZE...  Enough for 3 buffer fetches actually.  I think it'll
be quite common to overflow DIS_BUF_SIZE, and it's worth it to re-fill
the buffer properly.

> Also if user provides a range of address to the disassembly command then it is possible that
> end address is in the middle of the instruction and gdb will end up reading beyond the end
> address given in the command. So we probably cannot forbid that here anyway.

Yes, but while it's still passable to assume a few instructions
off of the requested range are still text/code, it's not so
safe if we try to read much beyond that.

I still think that it's worth it from a correctness perspective to
not make GDB worse than what it is today.  Arguably that libopcodes
behavior could be changed, (or fixed, depending on perspective),
though it may be a bunch of work.

>>> I think we'll need to derive from "struct disassemble_info", and add
>>> the original range to that new struct, or record that info directly in
>>> "struct disassemble_info", which is in include/dis-asm.h.
>>
>> I now noticed there's a struct disassemble_info->application_data field,
>> which GDB currently uses to put the gdbarch in.  We could put a disasm.c
>> specific structure there instead.
> I noticed that this field is already being used in spu-tdep.c. Any changes
> to it will break that code. 

This shows that extending the struct is a better approach than a
void * field that is part of the structure.  With the "subclass"
approach, we can have multiple layers each stacking their own
data, and they'll all be happy.  With a single void* field, we'd
need to add coupling between the layers to sort that out.

I took a stab at it.  I ended up touching other things too, and
reworking some of the comments.  The patch below is the result.

Tested on x86_64 Fedora 17, native and gdbserver,
and also built with --enable-targets=all.

Manually debugging against gdbserver, enabling "set debug
remote", and trying out "disassemble" commands, I can see
the optimization triggering as expected.

Before:

(gdb) set debug remote 1
(gdb) disassemble main
Dump of assembler code for function main:
Sending packet: $m410766,1#02...Packet received: 55
   0x0000000000410766 <+0>:     push   %rbp
Sending packet: $m410767,1#03...Packet received: 48
Sending packet: $m410768,1#04...Packet received: 89
Sending packet: $m410769,1#05...Packet received: e5
   0x0000000000410767 <+1>:     mov    %rsp,%rbp
Sending packet: $m41076a,1#2d...Packet received: 53
   0x000000000041076a <+4>:     push   %rbx
Sending packet: $m41076b,1#2e...Packet received: 48
Sending packet: $m41076c,1#2f...Packet received: 83
Sending packet: $m41076d,1#30...Packet received: ec
Sending packet: $m41076e,1#31...Packet received: 68
   0x000000000041076b <+5>:     sub    $0x68,%rsp
Sending packet: $m41076f,1#32...Packet received: 89
Sending packet: $m410770,1#fd...Packet received: 7d
Sending packet: $m410771,1#fe...Packet received: 9c
   0x000000000041076f <+9>:     mov    %edi,-0x64(%rbp)
Sending packet: $m410772,1#ff...Packet received: 48
Sending packet: $m410773,1#00...Packet received: 89
Sending packet: $m410774,1#01...Packet received: 75
Sending packet: $m410775,1#02...Packet received: 90
   0x0000000000410772 <+12>:    mov    %rsi,-0x70(%rbp)
Sending packet: $m410776,1#03...Packet received: 48
Sending packet: $m410777,1#04...Packet received: 8b
Sending packet: $m410778,1#05...Packet received: 45
Sending packet: $m410779,1#06...Packet received: 90
=> 0x0000000000410776 <+16>:    mov    -0x70(%rbp),%rax
Sending packet: $m41077a,1#2e...Packet received: 48
Sending packet: $m41077b,1#2f...Packet received: 83
Sending packet: $m41077c,1#30...Packet received: c0
Sending packet: $m41077d,1#31...Packet received: 08
   0x000000000041077a <+20>:    add    $0x8,%rax
Sending packet: $m41077e,1#32...Packet received: 48
Sending packet: $m41077f,1#33...Packet received: 89
Sending packet: $m410780,1#fe...Packet received: 45
Sending packet: $m410781,1#ff...Packet received: e0
   0x000000000041077e <+24>:    mov    %rax,-0x20(%rbp)
Sending packet: $m410782,1#00...Packet received: c7
Sending packet: $m410783,1#01...Packet received: 45
Sending packet: $m410784,1#02...Packet received: a4
Sending packet: $m410785,4#06...Packet received: 00000000
   0x0000000000410782 <+28>:    movl   $0x0,-0x5c(%rbp)
Sending packet: $m410789,1#07...Packet received: c7
Sending packet: $m41078a,1#2f...Packet received: 45
Sending packet: $m41078b,1#30...Packet received: a0
Sending packet: $m41078c,4#34...Packet received: 00000000
   0x0000000000410789 <+35>:    movl   $0x0,-0x60(%rbp)
Sending packet: $m410790,1#ff...Packet received: e9
Sending packet: $m410791,4#03...Packet received: f5030000
   0x0000000000410790 <+42>:    jmpq   0x410b8a <main+1060>
... many more ...


After:

(gdb) disassemble main
Dump of assembler code for function main:
Sending packet: $m410766,400#65...Packet received: 554889e5534883ec68897d9c48897590488b45904883c008488945e0c745a400000000c745a000000000e9f5030000488b45e0488b00be2fd943004889c7e8471effff85c0750fe895fcffffbf00000000e8d421ffff488b45e0488b00be39d943004889c7e8201effff85c07519488b0525a624004889c7e888fcffffbf00000000e8a321ffff488b45e0488b00be40d943004889c7e8ef1dffff85c0750cc745a001000000e973030000488b45e0488b00be49d943004889c7e8cb1dffff85c0750cc745a401000000e94f030000488b45e0488b00be51d943004889c7e8a71dffff85c0757d488345e008488b45e04889055bc62400eb05488345e008488b45e0488b004885c07418488b45e0488b00be5bd943004889c7e86c1dffff85c075d7488b0529c62400483945e0740c488b45e0488b004885c07519488b0568a524004889c7e8bbfbffffbf01000000e8d620ffff488b45e048c70000000000e9ba020000488b45e0488b00be5ed943004889c7e8121dffff85c0750fc70564d5240001000000e993020000488b45e0488b00be66d943004889c7e8eb1cffff85c0750fc7054da5240001000000e96c020000488b45e0488b00be75d943004889c7e8c41cffff85c07519488b05c9a424004889c7e8!
 86fbffffbf
00000000e84720ffff488b45e0488b00ba11000000be86d943004889c7e8be19ffff85c00f855c010000488b45e0488b00488d5011488b45e0488910488b45e0488b00488945c0488b45c0be98d943004889c7e8081fffff488945d0e915010000488b45d04889c6bf9ad94300e83e1cffff85c0750fc70544da240001000000e9de000000488b45d04889c6bfa0d94300e81a1cffff85c0750fc7056cda240001000000e9ba000000488b45d04889c6bf35d34300e8f61bffff85c0750fc705f4d9240001000000e996000000488b45d04889c6bf42d34300e8d21bffff85c0750cc70520d4240001000000eb75488b45d04889c6bf39d24300e8b11bffff85c0752ac705b7d9240001000000c705f9d9240001000000c7059bd9240001000000c705e1d3240001000000eb36488b059ca32400488b55d0bea8d943004889c7b800000000e8961bffff488b057fa324004889c7e82cfaffffbf01000000e8ed1effffbe98d94300bf00000000e8ee1dffff488945d048837dd0000f85e0feffffe9be000000488b45e0488b00becad943004889c7e8161bffff85c07510488b45e048c700ccd94300e9ba000000488b45e0488b00bed2d943004889c7e8ee1affff85c0750cc705107d240001000000eb72488b45e0488b00beead943004889c7e8ca1affff85c0750cc705ec7c2!
 4000000000
0eb4e488b45e0488b00be05da43004889c7e8a61affff85c0750cc705e8d2240001000000eb2a488b45e0488b10488b05a8a2
   0x0000000000410766 <+0>:     push   %rbp
   0x0000000000410767 <+1>:     mov    %rsp,%rbp
   0x000000000041076a <+4>:     push   %rbx
   0x000000000041076b <+5>:     sub    $0x68,%rsp
   0x000000000041076f <+9>:     mov    %edi,-0x64(%rbp)
   0x0000000000410772 <+12>:    mov    %rsi,-0x70(%rbp)
=> 0x0000000000410776 <+16>:    mov    -0x70(%rbp),%rax
   0x000000000041077a <+20>:    add    $0x8,%rax
   0x000000000041077e <+24>:    mov    %rax,-0x20(%rbp)
   0x0000000000410782 <+28>:    movl   $0x0,-0x5c(%rbp)
   0x0000000000410789 <+35>:    movl   $0x0,-0x60(%rbp)
   0x0000000000410790 <+42>:    jmpq   0x410b8a <main+1060>
... lots more insns printed before the next fetch ...
   0x0000000000410b5e <+1016>:  mov    (%rax),%rdx
Sending packet: $m410b66,400#90...Packet received: 2400be0cda43004889c7b800000000e8a61affffbf01000000e80c1effff488345e00890488b45e0488b004885c07412488b45e0488b000fb6003c2d0f84edfbffffbf60de6500e81e1affff85c07428488b0553a224004889c1ba08000000be01000000bf22da4300e8dc1dffffbf01000000e8b21dffff488b45e0488b00488945b8488345e00848837db800741a8b45a085c0752c8b45a485c07525488b45e0488b004885c07519488b05faa124004889c7e84df8ffffbf01000000e8681dffffe8272a0100488b45b84889c7e81a6bffffc745ec00000000c745e800000000488b45e0488b004885c07424488b45e0488b00be40d943004889c7e88919ffff85c0750cc745a001000000488345e0088b45a085c07460488b45e0488b004885c0744d488b45e0488b000fb60084c0743f488b45e0488b00488d4da8ba000000004889ce4889c7e83d1cffff8945e8837de800741b488b45a80fb60084c07510488b45e04883c008488b004885c07407c745ec01000000837dec007419488b0525a124004889c7e878f7ffffbf01000000e8931cffffe8f77cffffb800000000e8d0dc0100e8ee690000488b0548d72400488b80200100004885c07419488b0535d72400488b8020010000ffd085c07405e87509!
 0100bf0140
0000e8990b0100488905aac12400bf00400000e8880b0100488905a1c12400837de8000f85c4000000488b45e0488b004885c00f84b40000008b559c488b4de0488b45904889cb4829c34889d848c1f80389d129c189c88945b48b45b483c001489848c1e0034889c7e8320b0100488905fbc02400c745cc00000000eb3b488b05ebc024008b55cc4863d248c1e203488d1c108b45cc4898488d14c500000000488b45e04801d0488b004889c7e8512600004889038345cc018b45cc3b45b47cbd488b05a8c024008b55cc4863d248c1e2034801d048c70000000000488b058dc024004889c7e8589affffeb62837de800741e8b45e889c7e876a0ffff83f8ff754dbf30da4300b800000000e87c270000c70565c0240000000000c70563c0240000000000488b05447c24004889056dc02400488b053e7c240048890567c02400488b05387c240048890561c02400e8b1300100c70592d6240000000000bf60de6500e82817ffff85c0743dbf60de6500e81a17ffff85c07507e8fef7ffffeb1e488b05489f24004889c1ba20000000be01000000bf58da4300e8d11affffbf01000000e8a71affff8b05d1bf240085c0740b8b05c7bf240083f8027509c745dc00000000eb07c745dc01000000837ddc0075308b45a485c07529488b05ee9e24004889c1ba29000000be0100000!
 0bf80da430
0e8771affffbf01000000e84d1affff90c705169f240000000000c705fcd4240000000000c7053e7b2400ffffffff488b45b8
   0x0000000000410b61 <+1019>:  mov    0x24a2a8(%rip),%rax        # 0x65ae10 <stderr@@GLIBC_2.2.5>
   0x0000000000410b68 <+1026>:  mov    $0x43da0c,%esi
... lots more insns printed before the next fetch, etc. ...



I'll write a proper commit log if people agree this is good.

--------
2013-10-11  Taimoor Mirza  <taimoor_mirza@mentor.com>
	    Pedro Alves  <palves@redhat.com>

	* disasm.c: Don't include "dis-asm.h" here.
	(DIS_BUF_SIZE): New define.
	(dis_asm_read_memory): Over-fetch into a buffer, and serve from
	that buffer.
	(gdb_disassemble_info): Adjust to return a struct
	gdb_disassemble_info object.
	(gdb_disassembly): Adjust to allocate a struct
	gdb_disassemble_info object with a buffer instead of a plain
	disassemble_info object.
	(gdb_print_insn): Adjust to allocate a struct gdb_disassemble_info
	object instead of a plain disassemble_info object.
	* disasm.h: Include "dis-asm.h" here.
	(struct gdb_disassemble_info): New type.
	* arm-tdep.c: Include "disasm.h".
	(gdb_print_insn_arm): Adjust.
	* mips-tdep.c: Include "disasm.h".
	(gdb_print_insn_mips): Adjust.
	* spu-tdep.c: Include "disasm.h".
	(struct spu_dis_asm_data): Delete.
	(struct spu_disassemble_info): New type.
	(spu_dis_asm_print_address): Adjust.
	(gdb_print_insn_spu): Adjust to wrap the incoming struct
	gdb_disassemble_info object in a new spu_disassemble_info object
	instead of using the disassemble_info->application_data field.
---

 gdb/arm-tdep.c  |    4 +
 gdb/disasm.c    |  153 ++++++++++++++++++++++++++++++++++++++++++++++---------
 gdb/disasm.h    |   23 ++++++++
 gdb/mips-tdep.c |    4 +
 gdb/spu-tdep.c  |   24 ++++-----
 5 files changed, 169 insertions(+), 39 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7c78a61..edc4f31 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -57,6 +57,7 @@
 
 #include "record.h"
 #include "record-full.h"
+#include "disasm.h"
 
 #include "features/arm-with-m.c"
 #include "features/arm-with-m-fpa-layout.c"
@@ -8702,7 +8703,8 @@ arm_displaced_step_fixup (struct gdbarch *gdbarch,
 static int
 gdb_print_insn_arm (bfd_vma memaddr, disassemble_info *info)
 {
-  struct gdbarch *gdbarch = info->application_data;
+  struct gdb_disassemble_info *gdb_info = (struct gdb_disassemble_info *) info;
+  struct gdbarch *gdbarch = gdb_info->gdbarch;
 
   if (arm_pc_is_thumb (gdbarch, memaddr))
     {
diff --git a/gdb/disasm.c b/gdb/disasm.c
index e643c2d..f5fa70c 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -24,7 +24,6 @@
 #include "gdb_string.h"
 #include "disasm.h"
 #include "gdbcore.h"
-#include "dis-asm.h"
 
 /* Disassemble functions.
    FIXME: We should get rid of all the duplicate code in gdb that does
@@ -42,11 +41,95 @@ struct dis_line_entry
   CORE_ADDR end_pc;
 };
 
-/* Like target_read_memory, but slightly different parameters.  */
+/* Size of the disassembly over-read memory buffer.  */
+#define DIS_BUF_SIZE 1024
+
+/* Interface adjustment callback installed in libopcodes' disassembly
+   routine, to fetch memory off the target.  */
+
 static int
 dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
 		     struct disassemble_info *info)
 {
+  struct gdb_disassemble_info *gdb_info = (struct gdb_disassemble_info *) info;
+
+  /* The libopcodes disassembler fetches instructions off of memory
+     one at a time.  This results in lots of roundtrips to the target
+     to fetch many tiny chunks of memory.  As we can assume the
+     disassembler always reads memory forwards, and we can also
+     reasonably assume that fetching a reasonable medium sized buffer
+     takes as much as reading a small buffer (IOW, roundtrip latency
+     dominates), we can optimize disassembly performance by
+     over-fetching larger chunks into a buffer, and serving libopcodes
+     off of that buffer most of the time.  Note we avoid over reading
+     beyond the original requested range, as we don't know what might
+     be there --- could be memory mapped registers, etc.  */
+  if (gdb_info->low != gdb_info->high)
+    {
+      unsigned int org_len = len;
+      unsigned int org_memaddr = memaddr;
+
+      while (len != 0)
+	{
+	  if (info->buffer_vma <= memaddr
+	      && memaddr < info->buffer_vma + info->buffer_length)
+	    {
+	      unsigned int offset = (memaddr - info->buffer_vma);
+	      unsigned int l = min (len, info->buffer_length - offset);
+
+	      memcpy (myaddr, info->buffer + offset, l);
+
+	      memaddr += l;
+	      myaddr += l;
+	      len -= l;
+
+	      if (len == 0)
+		return 0;
+	    }
+	  else if (gdb_info->low <= memaddr && memaddr < gdb_info->high)
+	    {
+	      /* Refill the buffer, taking care to not read beyond the
+		 originally requested range.  */
+	      int rval;
+	      unsigned int left = gdb_info->high - memaddr;
+	      unsigned int buffer_length = min (left, DIS_BUF_SIZE);
+
+	      /* If we fail to read memory halfway, we'll have
+		 clobbered the buffer, so don't trust it anymore, even
+		 on fail.  */
+	      info->buffer_length = 0;
+	      rval = target_read_memory (memaddr, info->buffer, buffer_length);
+	      if (rval != 0)
+		{
+		  /* Over fetching failed.  Try reading only what is
+		     necessary to fulfill the caller's request.  We
+		     don't disable buffering completely as following
+		     calls may still be within the original requested
+		     range passed to gdb_disassembly (e.g., mixed
+		     source disassembly mode).  */
+		  buffer_length = org_len;
+		  memaddr = org_memaddr;
+		  rval = target_read_memory (memaddr, info->buffer, buffer_length);
+		  if (rval != 0)
+		    return rval;
+		}
+
+	      info->buffer_vma = memaddr;
+	      info->buffer_length = buffer_length;
+	    }
+	  else
+	    {
+	      /* libopcodes is trying to read beyond the originally
+		 requested range...  This will happen e.g., if the
+		 last instruction is a multi-byte instruction that
+		 starts within the range, but extends beyond it.  */
+	      len = org_len;
+	      memaddr = org_memaddr;
+	      break;
+	    }
+	}
+    }
+
   return target_read_memory (memaddr, myaddr, len);
 }
 
@@ -376,15 +459,16 @@ fprintf_disasm (void *stream, const char *format, ...)
   return 0;
 }
 
-static struct disassemble_info
+static struct gdb_disassemble_info
 gdb_disassemble_info (struct gdbarch *gdbarch, struct ui_file *file)
 {
-  struct disassemble_info di;
+  struct gdb_disassemble_info gdb_di;
+  struct disassemble_info *di = &gdb_di.di;
 
-  init_disassemble_info (&di, file, fprintf_disasm);
-  di.flavour = bfd_target_unknown_flavour;
-  di.memory_error_func = dis_asm_memory_error;
-  di.print_address_func = dis_asm_print_address;
+  init_disassemble_info (di, file, fprintf_disasm);
+  di->flavour = bfd_target_unknown_flavour;
+  di->memory_error_func = dis_asm_memory_error;
+  di->print_address_func = dis_asm_print_address;
   /* NOTE: cagney/2003-04-28: The original code, from the old Insight
      disassembler had a local optomization here.  By default it would
      access the executable file, instead of the target memory (there
@@ -393,14 +477,19 @@ gdb_disassemble_info (struct gdbarch *gdbarch, struct ui_file *file)
      didn't work as they relied on the access going to the target.
      Further, it has been supperseeded by trust-read-only-sections
      (although that should be superseeded by target_trust..._p()).  */
-  di.read_memory_func = dis_asm_read_memory;
-  di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di.endian = gdbarch_byte_order (gdbarch);
-  di.endian_code = gdbarch_byte_order_for_code (gdbarch);
-  di.application_data = gdbarch;
-  disassemble_init_for_target (&di);
-  return di;
+  di->read_memory_func = dis_asm_read_memory;
+  di->arch = gdbarch_bfd_arch_info (gdbarch)->arch;
+  di->mach = gdbarch_bfd_arch_info (gdbarch)->mach;
+  di->endian = gdbarch_byte_order (gdbarch);
+  di->endian_code = gdbarch_byte_order_for_code (gdbarch);
+  di->application_data = gdbarch;
+  disassemble_init_for_target (di);
+
+  gdb_di.gdbarch = gdbarch;
+  gdb_di.low = 0;
+  gdb_di.high = 0;
+
+  return gdb_di;
 }
 
 void
@@ -410,7 +499,7 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 {
   struct ui_file *stb = mem_fileopen ();
   struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
-  struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
+  struct gdb_disassemble_info gdb_di = gdb_disassemble_info (gdbarch, stb);
   /* To collect the instruction outputted from opcodes.  */
   struct symtab *symtab = NULL;
   struct linetable_entry *le = NULL;
@@ -426,13 +515,27 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       nlines = symtab->linetable->nitems;
     }
 
+  /* Set things up for the read-ahead buffer optimization in
+     dis_asm_read_memory.  We always allocate the maximum buffer size,
+     as mixed source disassembly may hop back and forth.  Even though
+     over-reading a buffer chunk might fail for a line, it could
+     succeed for the next line (and then we could need to grow the
+     buffer).  struct disassemble_info is a value struct (it's passed
+     around by copy at places), so this is the simplest, as we don't
+     need to care for special memory management.  */
+  gdb_di.di.buffer = xmalloc (DIS_BUF_SIZE);
+  make_cleanup (xfree, gdb_di.di.buffer);
+  gdb_di.low = low;
+  gdb_di.high = high;
+
   if (!(flags & DISASSEMBLY_SOURCE) || nlines <= 0
       || symtab == NULL || symtab->linetable == NULL)
-    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
+    do_assembly_only (gdbarch, uiout, &gdb_di.di,
+		      low, high, how_many, flags, stb);
 
   else if (flags & DISASSEMBLY_SOURCE)
-    do_mixed_source_and_assembly (gdbarch, uiout, &di, nlines, le, low,
-				  high, symtab, how_many, flags, stb);
+    do_mixed_source_and_assembly (gdbarch, uiout, &gdb_di.di, nlines, le,
+				  low, high, symtab, how_many, flags, stb);
 
   do_cleanups (cleanups);
   gdb_flush (gdb_stdout);
@@ -446,15 +549,15 @@ int
 gdb_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
 		struct ui_file *stream, int *branch_delay_insns)
 {
-  struct disassemble_info di;
+  struct gdb_disassemble_info gdb_di;
   int length;
 
-  di = gdb_disassemble_info (gdbarch, stream);
-  length = gdbarch_print_insn (gdbarch, memaddr, &di);
+  gdb_di = gdb_disassemble_info (gdbarch, stream);
+  length = gdbarch_print_insn (gdbarch, memaddr, &gdb_di.di);
   if (branch_delay_insns)
     {
-      if (di.insn_info_valid)
-	*branch_delay_insns = di.branch_delay_insns;
+      if (gdb_di.di.insn_info_valid)
+	*branch_delay_insns = gdb_di.di.branch_delay_insns;
       else
 	*branch_delay_insns = 0;
     }
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 3743ccc..6a63eee 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -25,9 +25,32 @@
 #define DISASSEMBLY_FILENAME	(0x1 << 3)
 #define DISASSEMBLY_OMIT_PC	(0x1 << 4)
 
+#include "dis-asm.h"
+
 struct ui_out;
 struct ui_file;
 
+/* A "subclass" of libopcodes' struct disassemble_info.  GDB always
+   passes uses this type instead of plain struct disassemble_info.  It
+   includes a "struct disassemble_info" as a kind of base class; users
+   downcast to "struct disassemble_info *" when needed.  */
+
+struct gdb_disassemble_info
+{
+  /* The base class.  */
+  struct disassemble_info di;
+
+  /* The architecture being used to interpret memory.  */
+  struct gdbarch *gdbarch;
+
+  /* The range of memory the caller of gdb_disassembly wanted
+     disassembled.  HIGH is exclusive.  GDB over-fetches memory off
+     the target for performance (by reducing roundtrips to the
+     target), but for safeness, won't try outside this range.  */
+  CORE_ADDR low;
+  CORE_ADDR high;
+};
+
 extern void gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 			     char *file_string, int flags, int how_many,
 			     CORE_ADDR low, CORE_ADDR high);
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index bcbdcc5..56bfe4c 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -57,6 +57,7 @@
 #include "user-regs.h"
 #include "valprint.h"
 #include "ax.h"
+#include "disasm.h"
 
 static const struct objfile_data *mips_pdr_data;
 
@@ -6773,7 +6774,8 @@ reinit_frame_cache_sfunc (char *args, int from_tty,
 static int
 gdb_print_insn_mips (bfd_vma memaddr, struct disassemble_info *info)
 {
-  struct gdbarch *gdbarch = info->application_data;
+  struct gdb_disassemble_info *gdb_info = (struct gdb_disassemble_info *) info;
+  struct gdbarch *gdbarch = gdb_info->gdbarch;
 
   /* FIXME: cagney/2003-06-26: Is this even necessary?  The
      disassembler needs to be able to locally determine the ISA, and
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index 46f3e2c..423cdc8 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -46,6 +46,7 @@
 #include "dwarf2.h"
 #include "exceptions.h"
 #include "spu-tdep.h"
+#include "disasm.h"
 
 
 /* The list of available "set spu " and "show spu " commands.  */
@@ -1662,17 +1663,17 @@ spu_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 
 /* Disassembler.  */
 
-struct spu_dis_asm_data
+struct spu_disassemble_info
 {
-  struct gdbarch *gdbarch;
+  struct gdb_disassemble_info gdb_di;
   int id;
 };
 
 static void
 spu_dis_asm_print_address (bfd_vma addr, struct disassemble_info *info)
 {
-  struct spu_dis_asm_data *data = info->application_data;
-  print_address (data->gdbarch, SPUADDR (data->id, addr), info->stream);
+  struct spu_disassemble_info *spu_info = (struct spu_disassemble_info *) info;
+  print_address (spu_info->gdb_di.gdbarch, SPUADDR (spu_info->id, addr), info->stream);
 }
 
 static int
@@ -1681,14 +1682,13 @@ gdb_print_insn_spu (bfd_vma memaddr, struct disassemble_info *info)
   /* The opcodes disassembler does 18-bit address arithmetic.  Make
      sure the SPU ID encoded in the high bits is added back when we
      call print_address.  */
-  struct disassemble_info spu_info = *info;
-  struct spu_dis_asm_data data;
-  data.gdbarch = info->application_data;
-  data.id = SPUADDR_SPU (memaddr);
-
-  spu_info.application_data = &data;
-  spu_info.print_address_func = spu_dis_asm_print_address;
-  return print_insn_spu (memaddr, &spu_info);
+  struct gdb_disassemble_info *gdb_info = (struct gdb_disassemble_info *) info;
+  struct spu_disassemble_info spu_info;
+
+  spu_info.gdb_di = *gdb_info;
+  spu_info.id = SPUADDR_SPU (memaddr);
+  spu_info.gdb_di.di.print_address_func = spu_dis_asm_print_address;
+  return print_insn_spu (memaddr, (struct disassemble_info *) &spu_info);
 }
 
 

Re: [patch] Disassembly improvements

[Doug Evans]

On Thu, Oct 10, 2013 at 6:14 AM, Abid, Hafiz <Hafiz_Abid@mentor.com> wrote:
> Hi Pedro,
> I am attaching the patch that was mentioned in the following thread. I resurrected it from our internal repo, did a bit of manual testing and run the regression suite without any problem. It basically reads memory from the target in a buffer in gdb_disassembly and tries to use this buffer in dis_asm_read_memory instead of reading from the target. This saves us on repeated memory read calls. The problem was noted when eclipse was trying to fill its disassembly view.
> https://sourceware.org/ml/gdb-patches/2013-10/msg00221.html
>
> Regards,
> Abid
>
> 2013-10-10  Taimoor Mirza  <taimoor_mirza@mentor.com>
>
>         * disasm.c (DIS_BUF_SIZE): New define.
>         (dis_asm_read_memory): Read from the disassembly buffer instead
>         of target memory directly.
>         (gdb_disassembly): Fill the disassembly buffer with a chunk of
>         the memory to disassemble.

This is a specific fix to a general problem.
Question: How much more of the general problem can we fix without
having a fix baked into the disassembler?

RE: [patch] Disassembly improvements

[Abid, Hafiz]

> Question: How much more of the general problem can we fix without having
> a fix baked into the disassembler?
Perhaps we can improve GDB memory caching to handle this case instead of baking
a solution into disassembler. I am not very familiar with dcache.c but I will 
investigate if we can extend that approach. Any guidance is welcome offcourse.

> -----Original Message-----
> From: Doug Evans [mailto:dje@google.com]
> Sent: 11 October 2013 22:34
> To: Abid, Hafiz
> Cc: Pedro Alves <palves@redhat.com> (palves@redhat.com); gdb-
> patches@sourceware.org; Mirza, Taimoor
> Subject: Re: [patch] Disassembly improvements
> 
> On Thu, Oct 10, 2013 at 6:14 AM, Abid, Hafiz <Hafiz_Abid@mentor.com>
> wrote:
> > Hi Pedro,
> > I am attaching the patch that was mentioned in the following thread. I
> resurrected it from our internal repo, did a bit of manual testing and run the
> regression suite without any problem. It basically reads memory from the
> target in a buffer in gdb_disassembly and tries to use this buffer in
> dis_asm_read_memory instead of reading from the target. This saves us on
> repeated memory read calls. The problem was noted when eclipse was trying
> to fill its disassembly view.
> > https://sourceware.org/ml/gdb-patches/2013-10/msg00221.html
> >
> > Regards,
> > Abid
> >
> > 2013-10-10  Taimoor Mirza  <taimoor_mirza@mentor.com>
> >
> >         * disasm.c (DIS_BUF_SIZE): New define.
> >         (dis_asm_read_memory): Read from the disassembly buffer instead
> >         of target memory directly.
> >         (gdb_disassembly): Fill the disassembly buffer with a chunk of
> >         the memory to disassemble.
> 
> This is a specific fix to a general problem.
> Question: How much more of the general problem can we fix without having
> a fix baked into the disassembler?

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/11/2013 10:34 PM, Doug Evans wrote:

> This is a specific fix to a general problem.

I don't know that this is a general problem.  It may look like one,
but it's not super clear to me.  Yes, we might have a similar problem
caused by lots of tiny reads from the target during prologue analysis.
But the approach there might be different from the right approach for
disassembly, or we could also come to the conclusion the problem
there is not exactly the same.

> Question: How much more of the general problem can we fix without
> having a fix baked into the disassembler?

The disassembly use case is one where GDB is being
told by the user "treat this range of addresses that I'll be
reading sequencially, as code".  If that happens to trip on some
memory mapped registers or some such, then it's garbage-in,
garbage-out, it was the user's fault.

As I mentioned in <https://sourceware.org/ml/gdb-patches/2013-09/msg01013.html>,
I'd rather we analyze the use cases independently (I'm not saying
they're not the same).

If we find commonalities, we can certainly factor things out and
come up with more general abstractions then.

If I were to try one, I think it would be along the lines of
a new TARGET_OBJECT_DISASM_MEMORY, and somehow pass more info down
the target_xfer interface so that the the core memory reading code
handles the caching.  Probably, that'd be done with a new pair of
'begin/end code caching' functions that would be called at the
appropriate places.  The new code in dis_asm_read_memory would
then be pushed to target.c, close to where stack cache is handled.

The main point there should be consensus on, is that a caching
scheme is a better solution for the disassembly use case, than trusting
read only sessions is, for the later doesn't have the problem with
self-modifying code, and, in addition, it also speeds up disassembling
when there is _no_ corresponding binary/'text section'.

I don't think there's any need to hold considering this quite
localized fix for slow disassembling as is while we investigate
other use cases.  IOW, we should let the code start small, and
grow/evolve from there.

-- 
Pedro Alves

Re: [patch] Disassembly improvements

[Doug Evans]

Pedro Alves writes:
 > On 10/11/2013 10:34 PM, Doug Evans wrote:
 > 
 > > This is a specific fix to a general problem.
 > 
 > I don't know that this is a general problem.

The general problem I'm referring to is efficient access of target memory.
[Otherwise we wouldn't have things like the dcache,
trust-readonly, explicit caching support for stack requests, etc.]

 >  It may look like one,
 > but it's not super clear to me.  Yes, we might have a similar problem
 > caused by lots of tiny reads from the target during prologue analysis.
 > But the approach there might be different from the right approach for
 > disassembly, or we could also come to the conclusion the problem
 > there is not exactly the same.
 >
 > > Question: How much more of the general problem can we fix without
 > > having a fix baked into the disassembler?
 > 
 > The disassembly use case is one where GDB is being
 > told by the user "treat this range of addresses that I'll be
 > reading sequencially, as code".  If that happens to trip on some
 > memory mapped registers or some such, then it's garbage-in,
 > garbage-out, it was the user's fault.

Though if gdb doesn't provide a range to constrain the caching,
the caching doesn't come into play in the current version of the patch
(the patch still avoids trying to prefetch too much).
In the case of, e.g., "disas main" gdb does provide a range.
The patch makes "disas main" efficient but doesn't help "x/5i main".
[No claim is made that improving the latter case is necessarily as easy,
but I think there is a case to be made that this patch fixes
a specific case (disas) of a specific case (reading code memory
for disassembly) of a general problem (reading target memory) :-).]

Presumably gdb can use function bounds or something else from the
debug info to constrain the affected memory space for other requests
so those can be sped up too.

"b main" on amd64 is instructive.
The stack align machinery blindly fetches 18 bytes,
and then prologue skipping ignores that and fetches a piece at a time.
And we do that twice (once for main from dwarf, once for main from elf).

(gdb) b main
Sending packet: $m4007b4,12#5d...Packet received: 554889e5be1c094000bf40106000e8e1feff
Sending packet: $m4007b4,1#2b...Packet received: 55
Sending packet: $m4007b5,3#2e...Packet received: 4889e5
Sending packet: $m4007b4,12#5d...Packet received: 554889e5be1c094000bf40106000e8e1feff
Sending packet: $m4007b4,1#2b...Packet received: 55
Sending packet: $m4007b5,3#2e...Packet received: 4889e5
Sending packet: $m4007b8,1#2f...Packet received: be
Breakpoint 1 at 0x4007b8: file hello.cc, line 6.

There's only a handful of calls to gdbarch_skip_prologue.
They could all be updated to employ whatever caching/prefetching
is appropriate.

 > As I mentioned in <https://sourceware.org/ml/gdb-patches/2013-09/msg01013.html>,
 > I'd rather we analyze the use cases independently (I'm not saying
 > they're not the same).
 > 
 > If we find commonalities, we can certainly factor things out and
 > come up with more general abstractions then.

IME this community frowns on such approaches.
I may have a biased data set though, and the general case is different.
It would be good to get some clarity here.

 > If I were to try one, I think it would be along the lines of
 > a new TARGET_OBJECT_DISASM_MEMORY, and somehow pass more info down
 > the target_xfer interface so that the the core memory reading code
 > handles the caching.  Probably, that'd be done with a new pair of
 > 'begin/end code caching' functions that would be called at the
 > appropriate places.  The new code in dis_asm_read_memory would
 > then be pushed to target.c, close to where stack cache is handled.

How hard would it be to do that now?

 > The main point there should be consensus on, is that a caching
 > scheme is a better solution for the disassembly use case, than trusting
 > read only sessions is, for the later doesn't have the problem with
 > self-modifying code, and, in addition, it also speeds up disassembling
 > when there is _no_ corresponding binary/'text section'.

How often do we see bug reports of slow disassembly when there is no
corresponding binary/text section? Another thing the community
frowns on IME is adding code to fix theoretical performance problems.
[but again, I may have a biased data set]

Plus self modifying code won't always provide the bounds necessary
to trigger the prefetching this patch does (not all jitters use
gdb's jit interface to register all instances of self-modified code).
"x/10i $random_address" is still slow.

 > I don't think there's any need to hold considering this quite
 > localized fix for slow disassembling as is while we investigate
 > other use cases.  IOW, we should let the code start small, and
 > grow/evolve from there.

I'm certainly willing to agree, but I just want to make sure
we're following established community rules.
Plus, how hard would it be to fix prologue skipping too?
It doesn't seem that hard, and if we go the TARGET_OBJECT_DISASM_MEMORY
route most of this patch will get tossed anyway.

Also, I feel I need to point out that we rejected an early version
of Yao's varobj patch because it used casting to effect baseclass/subclassing.
Perhaps there's less of it here, and there's some minimal amount that we
allow.  But again, I want to make sure we employ the rules consistently.
[And get some clarity. :-)]

Re: [patch] Disassembly improvements

[Yao Qi]

On 10/16/2013 09:16 AM, Doug Evans wrote:
>   > If I were to try one, I think it would be along the lines of
>   > a new TARGET_OBJECT_DISASM_MEMORY, and somehow pass more info down
>   > the target_xfer interface so that the the core memory reading code
>   > handles the caching.  Probably, that'd be done with a new pair of
>   > 'begin/end code caching' functions that would be called at the
>   > appropriate places.  The new code in dis_asm_read_memory would
>   > then be pushed to target.c, close to where stack cache is handled.
>
> How hard would it be to do that now?

AFAICS, it is not hard.  We've already had TARGET_OBJECT_STACK_MEMORY, 
so it is straightforward to add TARGET_OBJECT_CODE_MEMORY, and disas and 
skip_prologue can use it.

-- 
Yao (齐尧)

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/16/2013 08:51 AM, Yao Qi wrote:
> On 10/16/2013 09:16 AM, Doug Evans wrote:
>>   > If I were to try one, I think it would be along the lines of
>>   > a new TARGET_OBJECT_DISASM_MEMORY, and somehow pass more info down
>>   > the target_xfer interface so that the the core memory reading code
>>   > handles the caching.  Probably, that'd be done with a new pair of
>>   > 'begin/end code caching' functions that would be called at the
>>   > appropriate places.  The new code in dis_asm_read_memory would
>>   > then be pushed to target.c, close to where stack cache is handled.
>>
>> How hard would it be to do that now?
> 
> AFAICS, it is not hard.  We've already had TARGET_OBJECT_STACK_MEMORY, 
> so it is straightforward to add TARGET_OBJECT_CODE_MEMORY, and disas and 
> skip_prologue can use it.

Are you going to give it a try?  That'd be great.

Yeah, adding the new target object part is straightforward.  What
may not be, is either adjusting the dcache.c to the specifics of
disassembly, and range limiting, and making sure the cache is bounded
correctly, and flushed at the appropriate times.  It's one of those
"must try it to tell" things, I think.

-- 
Pedro Alves

Re: [patch] Disassembly improvements

[Yao Qi]

On 10/16/2013 08:08 PM, Pedro Alves wrote:
> Are you going to give it a try?  That'd be great.
>

I'd like to have a try after I finished the patch to change the way 
dcache reading target memory.

-- 
Yao (齐尧)

Re: [patch] Disassembly improvements

[Yao Qi]

On 10/16/2013 08:08 PM, Pedro Alves wrote:
> Yeah, adding the new target object part is straightforward.  What
> may not be, is either adjusting the dcache.c to the specifics of
> disassembly, and range limiting, and making sure the cache is bounded
> correctly, and flushed at the appropriate times.  It's one of those
> "must try it to tell" things, I think.

Pedro,
I start to think about it today.  I don't see we have to adjust dcache.c 
for disassembly and worry about the range.  From  GDB's point of view, 
the process of reading a piece of stack memory should be identical to 
reading a piece of code memory.  We are using '
target_dcache' to cache stack memory, so we can also reuse it to cache 
code memory.  Am I missing something?

-- 
Yao (齐尧)

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/18/2013 11:22 AM, Yao Qi wrote:
> On 10/16/2013 08:08 PM, Pedro Alves wrote:
>> Yeah, adding the new target object part is straightforward.  What
>> may not be, is either adjusting the dcache.c to the specifics of
>> disassembly, and range limiting, and making sure the cache is bounded
>> correctly, and flushed at the appropriate times.  It's one of those
>> "must try it to tell" things, I think.
> 
> Pedro,
> I start to think about it today.  I don't see we have to adjust dcache.c 
> for disassembly and worry about the range.  From  GDB's point of view, 
> the process of reading a piece of stack memory should be identical to 
> reading a piece of code memory.  We are using '
> target_dcache' to cache stack memory, so we can also reuse it to cache 
> code memory.  Am I missing something?

Hmm, the idea was that having "disassemble $foo, $bar" read outside
[$foo,$bar) might not be safe (particularly so if the line size is
set large), as it might trip on memory mapped registers, which
might have side effects when read.  I guess I could be convinced that
this is overzealous?

BTW, how will your "Read memory in multiple lines in dcache_xfer_memory"
series help disassembly if the disassembler, today, without that other
patch that caches things in disasm.c, fetches memory from the target
instruction by instruction?  Seems to me it'll end up always fetching
a single line at a time.

-- 
Pedro Alves

Re: [patch] Disassembly improvements

[Yao Qi]

On 10/19/2013 02:25 AM, Pedro Alves wrote:
> BTW, how will your "Read memory in multiple lines in dcache_xfer_memory"
> series help disassembly if the disassembler, today, without that other
> patch that caches things in disasm.c, fetches memory from the target
> instruction by instruction?  Seems to me it'll end up always fetching
> a single line at a time.

That series is to optimize dcache, since disassembly doesn't use dcache, 
that series doesn't help disassembly now.
Once we use dcache in disassembly (that is what I am doing), that series 
helps when users disassembly large functions.

-- 
Yao (齐尧)

Re: [patch] Disassembly improvements

[Doug Evans]

On Fri, Oct 18, 2013 at 6:54 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 10/19/2013 02:25 AM, Pedro Alves wrote:
>>
>> BTW, how will your "Read memory in multiple lines in dcache_xfer_memory"
>> series help disassembly if the disassembler, today, without that other
>> patch that caches things in disasm.c, fetches memory from the target
>> instruction by instruction?  Seems to me it'll end up always fetching
>> a single line at a time.
>
>
> That series is to optimize dcache, since disassembly doesn't use dcache,
> that series doesn't help disassembly now.
> Once we use dcache in disassembly (that is what I am doing), that series
> helps when users disassembly large functions.

You'll need to add some sort of prefetcher called from a higher level
in the disassembler that knows the memory range being disassembled.

Re: [patch] Disassembly improvements

[Pedro Alves]

On 10/16/2013 02:16 AM, Doug Evans wrote:
> Pedro Alves writes:
>  > On 10/11/2013 10:34 PM, Doug Evans wrote:
>  > 
>  > > This is a specific fix to a general problem.
>  > 
>  > I don't know that this is a general problem.
> 
> The general problem I'm referring to is efficient access of target memory.
> [Otherwise we wouldn't have things like the dcache,
> trust-readonly, explicit caching support for stack requests, etc.]
> 
>  >  It may look like one,
>  > but it's not super clear to me.  Yes, we might have a similar problem
>  > caused by lots of tiny reads from the target during prologue analysis.
>  > But the approach there might be different from the right approach for
>  > disassembly, or we could also come to the conclusion the problem
>  > there is not exactly the same.
>  >
>  > > Question: How much more of the general problem can we fix without
>  > > having a fix baked into the disassembler?
>  > 
>  > The disassembly use case is one where GDB is being
>  > told by the user "treat this range of addresses that I'll be
>  > reading sequencially, as code".  If that happens to trip on some
>  > memory mapped registers or some such, then it's garbage-in,
>  > garbage-out, it was the user's fault.
> 
> Though if gdb doesn't provide a range to constrain the caching,
> the caching doesn't come into play in the current version of the patch
> (the patch still avoids trying to prefetch too much).
> In the case of, e.g., "disas main" gdb does provide a range.

Yes, that's what I was talking about.

> The patch makes "disas main" efficient but doesn't help "x/5i main".
> [No claim is made that improving the latter case is necessarily as easy,
> but I think there is a case to be made that this patch fixes
> a specific case (disas) of a specific case (reading code memory
> for disassembly) of a general problem (reading target memory) :-).]

Thanks, that's clearer.

x/5i isn't a pressing use case, like "disassembly", IME.
Where disassembly slowness gets noticeable, is with frontends (like
eclipse) that display a memory disassemble window, that gets
updated/refreshed quite frequently, basically after every
single-step, or user command.  When x/5i is typed by the user
interactively, a 5 reads vs 1 read won't really be noticeable.

TBC, I'm not advocating against a more general fix, if somebody's
going to work on it.  I'd love that.

> Presumably gdb can use function bounds or something else from the
> debug info to constrain the affected memory space for other requests
> so those can be sped up too.

Yeah.

> "b main" on amd64 is instructive.
> The stack align machinery blindly fetches 18 bytes,
> and then prologue skipping ignores that and fetches a piece at a time.
> And we do that twice (once for main from dwarf, once for main from elf).
> 
> (gdb) b main
> Sending packet: $m4007b4,12#5d...Packet received: 554889e5be1c094000bf40106000e8e1feff
> Sending packet: $m4007b4,1#2b...Packet received: 55
> Sending packet: $m4007b5,3#2e...Packet received: 4889e5
> Sending packet: $m4007b4,12#5d...Packet received: 554889e5be1c094000bf40106000e8e1feff
> Sending packet: $m4007b4,1#2b...Packet received: 55
> Sending packet: $m4007b5,3#2e...Packet received: 4889e5
> Sending packet: $m4007b8,1#2f...Packet received: be
> Breakpoint 1 at 0x4007b8: file hello.cc, line 6.
> 
> There's only a handful of calls to gdbarch_skip_prologue.
> They could all be updated to employ whatever caching/prefetching
> is appropriate.

Sure.

>  > If I were to try one, I think it would be along the lines of
>  > a new TARGET_OBJECT_DISASM_MEMORY, and somehow pass more info down
>  > the target_xfer interface so that the the core memory reading code
>  > handles the caching.  Probably, that'd be done with a new pair of
>  > 'begin/end code caching' functions that would be called at the
>  > appropriate places.  The new code in dis_asm_read_memory would
>  > then be pushed to target.c, close to where stack cache is handled.
> 
> How hard would it be to do that now?

I'm not personally going to do it now, so "impossible" for me.  :-)
But if Yao or Hafiz, or Taimoor or someone else can spend the
effort, then of course that'd be great.

>  > The main point there should be consensus on, is that a caching
>  > scheme is a better solution for the disassembly use case, than trusting
>  > read only sessions is, for the later doesn't have the problem with
>  > self-modifying code, and, in addition, it also speeds up disassembling
>  > when there is _no_ corresponding binary/'text section'.
> 
> How often do we see bug reports of slow disassembly when there is no
> corresponding binary/text section?

The original use case that motivated this caching, that is,
a frontend that has a disassembly window that gets
refreshed/updated very frequently, should trigger that.

> Plus self modifying code won't always provide the bounds necessary
> to trigger the prefetching this patch does (not all jitters use
> gdb's jit interface to register all instances of self-modified code).

But "disassemble $random_address, +400" (or the MI equivalent) does.

If we don't have bounds to work with, then, what could we do,
if we're playing it safe?  What are you suggesting?

> Also, I feel I need to point out that we rejected an early version
> of Yao's varobj patch because it used casting to effect baseclass/subclassing.

I haven't followed that thread.  Was subclassing really the reason, or
was it because subclassing (whatever the language) didn't make sense
for that particular case?  We certainly use baseclass/subclassing today
in several places.  E.g., breakpoint.c.

-- 
Pedro Alves