Re: [PATCH v2 1/4] disasm: split dump_insns

[Doug Evans <xdje42 at gmail dot com>]

[sorry for the dupe]

Markus Metzger <markus.t.metzger@intel.com> writes:
> Split disasm.c's dump_insn into two parts:
>
>   - print a single instruction
>   - loop over the specified address range
>
> The first part will be refined in subsequent patches so it can be reused.
>
> 2015-10-19  Markus Metzger <markus.t.metzger@intel.com>
>
> gdb/
> 	* disasm.c (dump_insns):  Split into this and ...
> 	(gdb_print_insn_tuple): ... this.
> ---
>  gdb/disasm.c | 172 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 95 insertions(+), 77 deletions(-)
>
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 6e3d6c1..4c80c5f 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -170,100 +170,118 @@ compare_lines (const void *mle1p, const void *mle2p)
>    return val;
>  }
>  
> +/* Prints the instruction at PC into UIOUT and returns the length of the
> +   printed instruction in bytes.  */
> +
>  static int
> -dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
> -	    struct disassemble_info * di,
> -	    CORE_ADDR low, CORE_ADDR high,
> -	    int how_many, int flags, struct ui_file *stb,
> -	    CORE_ADDR *end_pc)
> +gdb_print_insn_tuple (struct gdbarch *gdbarch, struct ui_out *uiout,

The word "tuple" in the name here is odd.
Intuitively, since there is still "dump_insns" I'd expect this function
to be named "dump_insn".
Maybe it'll make sense once I read the entire patch set.
[I could do that first, but I often forget things I want to say
from the first time through, so sending them now is just easier.]

> +		      struct disassemble_info * di, CORE_ADDR pc, int flags,
> +		      struct ui_file *stb)
>  {
> -  int num_displayed = 0;
> -  CORE_ADDR pc;
> -
>    /* parts of the symbolic representation of the address */
>    int unmapped;
>    int offset;
>    int line;
> +  int size;
>    struct cleanup *ui_out_chain;
> +  char *filename = NULL;
> +  char *name = NULL;
> +
> +  ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +
> +  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> +    ui_out_text (uiout, pc_prefix (pc));
> +  ui_out_field_core_addr (uiout, "address", gdbarch, pc);
> +
> +  if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
> +			       &line, &unmapped))
> +    {
> +      /* We don't care now about line, filename and unmapped.  But we might in
> +	 the future.  */
> +      ui_out_text (uiout, " <");
> +      if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
> +	ui_out_field_string (uiout, "func-name", name);
> +      ui_out_text (uiout, "+");
> +      ui_out_field_int (uiout, "offset", offset);
> +      ui_out_text (uiout, ">:\t");
> +    }
> +  else
> +    ui_out_text (uiout, ":\t");
>  
> -  for (pc = low; pc < high;)
> +  if (filename != NULL)
> +    xfree (filename);
> +  if (name != NULL)
> +    xfree (name);
> +
> +  ui_file_rewind (stb);
> +  if (flags & DISASSEMBLY_RAW_INSN)
>      {
> -      char *filename = NULL;
> -      char *name = NULL;
> +      CORE_ADDR end_pc;
> +      bfd_byte data;
> +      int status;
> +      const char *spacer = "";
>  
> -      QUIT;
> -      if (how_many >= 0)
> -	{
> -	  if (num_displayed >= how_many)
> -	    break;
> -	  else
> -	    num_displayed++;
> -	}
> -      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +      /* Build the opcodes using a temporary stream so we can
> +	 write them out in a single go for the MI.  */
> +      struct ui_file *opcode_stream = mem_fileopen ();
> +      struct cleanup *cleanups =
> +	make_cleanup_ui_file_delete (opcode_stream);
>  
> -      if ((flags & DISASSEMBLY_OMIT_PC) == 0)
> -	ui_out_text (uiout, pc_prefix (pc));
> -      ui_out_field_core_addr (uiout, "address", gdbarch, pc);
> +      size = gdbarch_print_insn (gdbarch, pc, di);
> +      end_pc = pc + size;
>  
> -      if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
> -				   &line, &unmapped))
> +      for (;pc < end_pc; ++pc)
>  	{
> -	  /* We don't care now about line, filename and
> -	     unmapped. But we might in the future.  */
> -	  ui_out_text (uiout, " <");
> -	  if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
> -	    ui_out_field_string (uiout, "func-name", name);
> -	  ui_out_text (uiout, "+");
> -	  ui_out_field_int (uiout, "offset", offset);
> -	  ui_out_text (uiout, ">:\t");
> +	  status = (*di->read_memory_func) (pc, &data, 1, di);
> +	  if (status != 0)
> +	    (*di->memory_error_func) (status, pc, di);
> +	  fprintf_filtered (opcode_stream, "%s%02x",
> +			    spacer, (unsigned) data);
> +	  spacer = " ";
>  	}
> -      else
> -	ui_out_text (uiout, ":\t");
> -
> -      if (filename != NULL)
> -	xfree (filename);
> -      if (name != NULL)
> -	xfree (name);
> -
> -      ui_file_rewind (stb);
> -      if (flags & DISASSEMBLY_RAW_INSN)
> -        {
> -          CORE_ADDR old_pc = pc;
> -          bfd_byte data;
> -          int status;
> -          const char *spacer = "";
> -
> -          /* Build the opcodes using a temporary stream so we can
> -             write them out in a single go for the MI.  */
> -          struct ui_file *opcode_stream = mem_fileopen ();
> -          struct cleanup *cleanups =
> -            make_cleanup_ui_file_delete (opcode_stream);
> -
> -          pc += gdbarch_print_insn (gdbarch, pc, di);
> -          for (;old_pc < pc; old_pc++)
> -            {
> -              status = (*di->read_memory_func) (old_pc, &data, 1, di);
> -              if (status != 0)
> -                (*di->memory_error_func) (status, old_pc, di);
> -              fprintf_filtered (opcode_stream, "%s%02x",
> -                                spacer, (unsigned) data);
> -              spacer = " ";
> -            }
> -          ui_out_field_stream (uiout, "opcodes", opcode_stream);
> -          ui_out_text (uiout, "\t");
> -
> -          do_cleanups (cleanups);
> -        }
> -      else
> -        pc += gdbarch_print_insn (gdbarch, pc, di);
> -      ui_out_field_stream (uiout, "inst", stb);
> -      ui_file_rewind (stb);
> -      do_cleanups (ui_out_chain);
> -      ui_out_text (uiout, "\n");
> +      ui_out_field_stream (uiout, "opcodes", opcode_stream);
> +      ui_out_text (uiout, "\t");
> +
> +      do_cleanups (cleanups);
> +    }
> +  else
> +    size = gdbarch_print_insn (gdbarch, pc, di);
> +
> +  ui_out_field_stream (uiout, "inst", stb);
> +  ui_file_rewind (stb);
> +  do_cleanups (ui_out_chain);
> +  ui_out_text (uiout, "\n");
> +
> +  return size;
> +}
> +
> +static int
> +dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
> +	    struct disassemble_info * di,
> +	    CORE_ADDR low, CORE_ADDR high,
> +	    int how_many, int flags, struct ui_file *stb,
> +	    CORE_ADDR *end_pc)
> +{
> +  int num_displayed = 0;
> +
> +  while (low < high && (how_many < 0 || num_displayed < how_many))
> +    {
> +      int size;
> +
> +      size = gdb_print_insn_tuple (gdbarch, uiout, di, low, flags, stb);
> +      if (size <= 0)
> +	break;
> +
> +      num_displayed += 1;

I'm guessing the "+= 1" is used to be consistent with the next line.
My impression is that the community prefers ++num_displayed,
but in this particular case I could go with either.

> +      low += size;
> +
> +      /* Allow user to bail out with ^C.  */
> +      QUIT;
>      }
>  
>    if (end_pc != NULL)
> -    *end_pc = pc;
> +    *end_pc = low;
> +
>    return num_displayed;
>  }