RE: [patch] Disassembly improvements

Abid, Hafiz
2013-10-10T13:57:54Z

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