[PATCH 0/6] disasm, record: fix "record instruction-history /m"

[PATCH 0/6] disasm, record: fix "record instruction-history /m"

[Markus Metzger]

The /m modifier interleaves source lines with the disassembly of recorded
instructions.  This calls disasm.c's do_mixed_source_and_assembly once for
each recorded instruction to be printed.

The latter really does a source print with intermixed disassembly.  To that
purpose, it may reorder instructions to better match the source.  This doesn't
work for printing a single instruction.

The new DISASSEMBLY_SOURCE mode promises to improve that as it preserves the
order of instructions.  It still assumes a consecutive range of instructions
in some places.

Extend it so it can be used for source interleaving in the "record
instruction-history" command.

There is an open regarding MI output of "record instruction-history".  The
current version seems pretty broken as it will put each instruction into its
own "asm_insns" list.  This isn't that bad since there is no real MI support
for record btrace, yet.

This new version puts all instructions into the same list - unless there are
gaps in the trace.  For gaps, we will end up with one list per consecutive trace
segment plus the error message for the gaps.

To get this also into a single "asm_insns" list, we'd need to further extend
struct disas_insn to support gaps instead of instructions.  I have not done
this as I first wanted to see if the current changes are not already too
intrusive.

See also branch users/mmetzger/disasm-vec.

I tested this series on 64-bit IA running RHEL 7.


Markus Metzger (6):
  disasm: change dump_insns to print a single instruction
  disasm: add struct disas_insn to describe to-be-disassembled
    instruction
  disas: add gdb_disassembly_vec
  disasm: use entire line table in line_has_code_p
  disasm: determine preceding lines independent of last_line
  btrace: use gdb_disassembly_vec and new source interleaving method

 gdb/disasm.c        | 549 +++++++++++++++++++++++++++-------------------------
 gdb/disasm.h        |  24 +++
 gdb/record-btrace.c |  54 +++---
 gdb/record.c        |   2 +-
 4 files changed, 341 insertions(+), 288 deletions(-)

-- 
1.8.3.1

[PATCH 2/6] disasm: add struct disas_insn to describe to-be-disassembled instruction

[Markus Metzger]

Add a new struct disas_insn to add additional fields describing the
to-be-disassembled instruction.  The additional fields are:

  number            an optional instruction number, zero if omitted.
  is_speculative    a predicate saying whether the instruction was
                    executed speculatively.

Replace the PC parameter of dump_insn with a pointer to the above struct.

If non-zero, the instruction number is printed first.  It will also appear
as a new optional field "insn-number" in MI.  The field will be present if
insn_num is non-zero.

If is_speculative is set, speculative execution will be indicated by a "?"
following the new instruction number field.  Unless the PC is omitted, it
will overwrite the first byte of the PC prefix.  It will appear as a new
optional field "is-speculative" in MI.  The field will contain "?" and will
be present if is_speculative is set.

The speculative execution indication is guarded by a new flag
DISASSEMBLY_SPECULATION.

2015-09-21  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* disasm.h (DISASSEMBLY_SPECULATION): New.
	(struct disas_insn): New.
	* disasm.c (dump_insn): Replace parameter PC with INSN.  Update users.
	Print instruction number and indicate speculative execution.
---
 gdb/disasm.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 gdb/disasm.h | 14 ++++++++++++
 2 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 981ab7e..044dcac 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -170,13 +170,13 @@ compare_lines (const void *mle1p, const void *mle2p)
   return val;
 }
 
-/* Prints the instruction at PC into UIOUT and returns the length of the
+/* Prints the instruction INSN into UIOUT and returns the length of the
    printed instruction in bytes.  */
 
 static int
 dump_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
-	   struct disassemble_info * di, CORE_ADDR pc, int flags,
-	   struct ui_file *stb)
+	   struct disassemble_info * di, const struct disas_insn *insn,
+	   int flags, struct ui_file *stb)
 {
   /* parts of the symbolic representation of the address */
   int unmapped;
@@ -186,10 +186,37 @@ dump_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
   struct cleanup *ui_out_chain;
   char *filename = NULL;
   char *name = NULL;
+  CORE_ADDR pc;
 
   ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+  pc = insn->addr;
+
+  if (insn->number != 0)
+    {
+      ui_out_field_fmt (uiout, "insn-number", "%u", insn->number);
+      ui_out_text (uiout, "\t");
+    }
+
+  if ((flags & DISASSEMBLY_SPECULATIVE) != 0)
+    {
+      if (insn->is_speculative)
+	{
+	  ui_out_field_string (uiout, "is-speculative", "?");
 
-  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+	  /* The speculative execution indication overwrites the first
+	     character of the PC prefix.
+	     We assume a PC prefix length of 3 characters.  */
+	  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+	    ui_out_text (uiout, pc_prefix (pc) + 1);
+	  else
+	    ui_out_text (uiout, "  ");
+	}
+      else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+	ui_out_text (uiout, pc_prefix (pc));
+      else
+	ui_out_text (uiout, "   ");
+    }
+  else if ((flags & DISASSEMBLY_OMIT_PC) == 0)
     ui_out_text (uiout, pc_prefix (pc));
   ui_out_field_core_addr (uiout, "address", gdbarch, pc);
 
@@ -347,7 +374,7 @@ do_mixed_source_and_assembly_deprecated
 
   for (i = 0; i < newlines; i++)
     {
-      CORE_ADDR pc;
+      struct disas_insn insn;
 
       /* Print out everything from next_line to the current line.  */
       if (mle[i].line >= next_line)
@@ -402,17 +429,20 @@ do_mixed_source_and_assembly_deprecated
 	    = make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
 	}
 
-      pc = mle[i].start_pc;
-      while (pc < mle[i].end_pc && (how_many < 0 || num_displayed < how_many))
+      memset (&insn, 0, sizeof (insn));
+      insn.addr = mle[i].start_pc;
+
+      while (insn.addr < mle[i].end_pc
+	     && (how_many < 0 || num_displayed < how_many))
 	{
 	  int size;
 
-	  size = dump_insn (gdbarch, uiout, di, pc, flags, stb);
+	  size = dump_insn (gdbarch, uiout, di, &insn, flags, stb);
 	  if (size <= 0)
 	    break;
 
 	  num_displayed += 1;
-	  pc += size;
+	  insn.addr += size;
 
 	  /* Allow user to bail out with ^C.  */
 	  QUIT;
@@ -551,6 +581,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       int start_preceding_line_to_display = 0;
       int end_preceding_line_to_display = 0;
       int new_source_line = 0;
+      struct disas_insn insn;
 
       sal = find_pc_line (pc, 0);
 
@@ -675,21 +706,26 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       else
 	end_pc = pc + 1;
 
-      while (pc < end_pc && (how_many < 0 || num_displayed < how_many))
+      memset (&insn, 0, sizeof (insn));
+      insn.addr = pc;
+
+      while (insn.addr < end_pc && (how_many < 0 || num_displayed < how_many))
 	{
 	  int size;
 
-	  size = dump_insn (gdbarch, uiout, di, pc, flags, stb);
+	  size = dump_insn (gdbarch, uiout, di, &insn, flags, stb);
 	  if (size <= 0)
 	    break;
 
 	  num_displayed += 1;
-	  pc += size;
+	  insn.addr += size;
 
 	  /* Allow user to bail out with ^C.  */
 	  QUIT;
 	}
 
+      pc = insn.addr;
+
       if (how_many >= 0 && num_displayed >= how_many)
 	break;
 
@@ -708,20 +744,24 @@ do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
 		  int how_many, int flags, struct ui_file *stb)
 {
   struct cleanup *ui_out_chain;
+  struct disas_insn insn;
   int num_displayed = 0;
 
   ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
 
-  while (low < high && (how_many < 0 || num_displayed < how_many))
+  memset (&insn, 0, sizeof (insn));
+  insn.addr = low;
+
+  while (insn.addr < high && (how_many < 0 || num_displayed < how_many))
     {
       int size;
 
-      size = dump_insn (gdbarch, uiout, di, low, flags, stb);
+      size = dump_insn (gdbarch, uiout, di, &insn, flags, stb);
       if (size <= 0)
 	break;
 
       num_displayed += 1;
-      low += size;
+      insn.addr += size;
 
       /* Allow user to bail out with ^C.  */
       QUIT;
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 7e6f1a2..3d6f5bb 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -27,11 +27,25 @@
 #define DISASSEMBLY_FILENAME	(0x1 << 3)
 #define DISASSEMBLY_OMIT_PC	(0x1 << 4)
 #define DISASSEMBLY_SOURCE	(0x1 << 5)
+#define DISASSEMBLY_SPECULATIVE	(0x1 << 6)
 
 struct gdbarch;
 struct ui_out;
 struct ui_file;
 
+/* An instruction to be disassembled.  */
+
+struct disas_insn {
+  /* The address of the memory containing the instruction.  */
+  CORE_ADDR addr;
+
+  /* An optional instruction number.  If non-zero, it is printed first.  */
+  unsigned int number;
+
+  /* A bit-field saying whether the instruction was executed speculatively.  */
+  unsigned int is_speculative:1;
+};
+
 /* Return a filled in disassemble_info object for use by gdb.  */
 
 extern struct disassemble_info gdb_disassemble_info (struct gdbarch *gdbarch,
-- 
1.8.3.1

Re: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-disassembled instruction

[Pedro Alves]

On 09/21/2015 03:54 PM, Markus Metzger wrote:
> Add a new struct disas_insn to add additional fields describing the
> to-be-disassembled instruction.  The additional fields are:
> 
>   number            an optional instruction number, zero if omitted.
>   is_speculative    a predicate saying whether the instruction was
>                     executed speculatively.
> 
> Replace the PC parameter of dump_insn with a pointer to the above struct.
> 
> If non-zero, the instruction number is printed first.  It will also appear
> as a new optional field "insn-number" in MI.  The field will be present if
> insn_num is non-zero.
> 
> If is_speculative is set, speculative execution will be indicated by a "?"
> following the new instruction number field.  Unless the PC is omitted, it
> will overwrite the first byte of the PC prefix.  It will appear as a new
> optional field "is-speculative" in MI.  The field will contain "?" and will
> be present if is_speculative is set.
> 

I think the log would be much clearer if the rationale was specified in
terms of why this is necessary, and if we saw a before/after example.

Also, being a user/frontend visible change, shouldn't these new
fields be documented and mentioned in NEWS?

> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -27,11 +27,25 @@
>  #define DISASSEMBLY_FILENAME	(0x1 << 3)
>  #define DISASSEMBLY_OMIT_PC	(0x1 << 4)
>  #define DISASSEMBLY_SOURCE	(0x1 << 5)
> +#define DISASSEMBLY_SPECULATIVE	(0x1 << 6)
>  
>  struct gdbarch;
>  struct ui_out;
>  struct ui_file;
>  
> +/* An instruction to be disassembled.  */
> +
> +struct disas_insn {

{ goes on the next line.

Thanks,
Pedro Alves

RE: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-disassembled instruction

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Friday, October 9, 2015 2:51 PM
> To: Metzger, Markus T; dje@google.com
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-
> disassembled instruction


> I think the log would be much clearer if the rationale was specified in
> terms of why this is necessary, and if we saw a before/after example.

I added the following to the beginning of the commit message to motivate
the patch:

The "record instruction-history" command prints for each instruction, in
addition to the instruction's disassembly:

  - the instruction number in the recorded execution trace
  - a '?' before the instruction if it was executed speculatively

To allow the "record instruction-history" command to use GDB's disassembly
infrastructure, we extend dump_insn to optionally print those additional
fields.


> Also, being a user/frontend visible change, shouldn't these new
> fields be documented and mentioned in NEWS?

There is no UI change and thus also no before/after example.

The new optional fields are currently not used.  They will be used by the
"record instruction-history" command in the last patch of this series.
Even then, there is no UI change.  Both the "record instruction-history"
and the "disassemble" command behave as they did before.

There is a change to the MI output of "record instruction-history".
As I didn't do any conscious MI support for record btrace, I don't expect
it to be working.  I'm using the ui_out_* functions so there might be some
form of MI support.  AFAIK it is not being used.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-disassembled instruction

[Pedro Alves]

(Closing the loop on v1...  Sorry for the delay.)

On 10/12/2015 09:44 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Friday, October 9, 2015 2:51 PM
>> To: Metzger, Markus T; dje@google.com
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-
>> disassembled instruction
> 
> 
>> I think the log would be much clearer if the rationale was specified in
>> terms of why this is necessary, and if we saw a before/after example.
> 
> I added the following to the beginning of the commit message to motivate
> the patch:
> 
> The "record instruction-history" command prints for each instruction, in
> addition to the instruction's disassembly:
> 
>   - the instruction number in the recorded execution trace
>   - a '?' before the instruction if it was executed speculatively
> 
> To allow the "record instruction-history" command to use GDB's disassembly
> infrastructure, we extend dump_insn to optionally print those additional
> fields.

Ah, that's clears things, thanks.

> 
> 
>> Also, being a user/frontend visible change, shouldn't these new
>> fields be documented and mentioned in NEWS?
> 
> There is no UI change and thus also no before/after example.
> 
> The new optional fields are currently not used.  They will be used by the
> "record instruction-history" command in the last patch of this series.
> Even then, there is no UI change.  Both the "record instruction-history"
> and the "disassemble" command behave as they did before.
> 

OK.

> There is a change to the MI output of "record instruction-history".
> As I didn't do any conscious MI support for record btrace, I don't expect
> it to be working.  I'm using the ui_out_* functions so there might be some
> form of MI support.  AFAIK it is not being used.

I see.  Note that when an MI frontend invokes a CLI command (through
-interpreter-exec console ..."), the output it gets is still CLI output.
The fields passed to ui_out_* functions only get converted to MI attributes
if the command entered was a real MI command.  IOW, if there's no MI command
equivalent of "record instruction-history", then there's no such thing as
'MI output of "record instruction-history"'.

This sentence:

 "If non-zero, the instruction number is printed first.  It will also appear
 as a new optional field "insn-number" in MI.  The field will be present if
 insn_num is non-zero."

was what made me believe there was some MI command that would now output
that field.  But it now sounds to me that e.g., -data-disassemble output does
not really change.  (If it does change, then we need to extend the manual where
it documents the "Result" of that command (see "GDB/MI Data Manipulation").)

Thanks,
Pedro Alves

[PATCH 6/6] btrace: use gdb_disassembly_vec and new source interleaving method

[Markus Metzger]

Use the new gdb_disassembly_vec interface for the "record instruction-
history" command.

Use the new source interleaving method.  We stick to the /m modifier.
The old version is broken and there's no point in keeping it as alternative
to this new version.

2015-09-21  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* record-btrace.c (btrace_insn_history): Call gdb_disassembly_vec
	instead of gdb_disassembly.  Set DISASSEMBLY_SPECULATIVE.
	* record.c (get_insn_history_modifiers): Set DISASSEMBLY_SOURCE
	instead of DISASSEMBLY_SOURCE_DEPRECATED.
---
 gdb/record-btrace.c | 54 ++++++++++++++++++++++++++---------------------------
 gdb/record.c        |  2 +-
 2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d14e6cf..ecb8085 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -538,11 +538,17 @@ btrace_insn_history (struct ui_out *uiout,
 {
   struct gdbarch *gdbarch;
   struct btrace_insn_iterator it;
+  VEC (disas_insn_t) *insns;
+  struct cleanup *cleanups;
 
   DEBUG ("itrace (0x%x): [%u; %u)", flags, btrace_insn_number (begin),
 	 btrace_insn_number (end));
 
+  flags |= DISASSEMBLY_SPECULATIVE;
+
   gdbarch = target_gdbarch ();
+  insns = NULL;
+  cleanups = make_cleanup (VEC_cleanup (disas_insn_t), &insns);
 
   for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
     {
@@ -560,42 +566,34 @@ btrace_insn_history (struct ui_out *uiout,
 	  /* We have trace so we must have a configuration.  */
 	  gdb_assert (conf != NULL);
 
+	  /* Print the instructions we collected, so far, then the error.
+
+	     This will result in two disjoint "asm_insns" lists with a
+	     stand-alone error in-between.  It's better than what we had
+	     before but still not OK for MI users.  */
+	  gdb_disassembly_vec (gdbarch, uiout, flags, insns);
+
 	  btrace_ui_out_decode_error (uiout, it.function->errcode,
 				      conf->format);
+
+	  /* Start over.  */
+	  VEC_free (disas_insn_t, insns);
 	}
       else
 	{
-	  char prefix[4];
-
-	  /* We may add a speculation prefix later.  We use the same space
-	     that is used for the pc prefix.  */
-	  if ((flags & DISASSEMBLY_OMIT_PC) == 0)
-	    strncpy (prefix, pc_prefix (insn->pc), 3);
-	  else
-	    {
-	      prefix[0] = ' ';
-	      prefix[1] = ' ';
-	      prefix[2] = ' ';
-	    }
-	  prefix[3] = 0;
+	  struct disas_insn *dinsn;
 
-	  /* Print the instruction index.  */
-	  ui_out_field_uint (uiout, "index", btrace_insn_number (&it));
-	  ui_out_text (uiout, "\t");
-
-	  /* Indicate speculative execution by a leading '?'.  */
-	  if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0)
-	    prefix[0] = '?';
-
-	  /* Print the prefix; we tell gdb_disassembly below to omit it.  */
-	  ui_out_field_fmt (uiout, "prefix", "%s", prefix);
-
-	  /* Disassembly with '/m' flag may not produce the expected result.
-	     See PR gdb/11833.  */
-	  gdb_disassembly (gdbarch, uiout, NULL, flags | DISASSEMBLY_OMIT_PC,
-			   1, insn->pc, insn->pc + 1);
+	  dinsn = VEC_safe_push (disas_insn_t, insns, NULL);
+	  dinsn->addr = insn->pc;
+	  dinsn->number = btrace_insn_number (&it);
+	  dinsn->is_speculative = (insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0;
 	}
     }
+
+  /* Print the instructions we collected.  */
+  gdb_disassembly_vec (gdbarch, uiout, flags, insns);
+
+  do_cleanups (cleanups);
 }
 
 /* The to_insn_history method of target record-btrace.  */
diff --git a/gdb/record.c b/gdb/record.c
index 71ef973..c17b199 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -458,7 +458,7 @@ get_insn_history_modifiers (char **arg)
 	  switch (*args)
 	    {
 	    case 'm':
-	      modifiers |= DISASSEMBLY_SOURCE_DEPRECATED;
+	      modifiers |= DISASSEMBLY_SOURCE;
 	      modifiers |= DISASSEMBLY_FILENAME;
 	      break;
 	    case 'r':
-- 
1.8.3.1

Re: [PATCH 6/6] btrace: use gdb_disassembly_vec and new source interleaving method

[Andrew Burgess]

* Markus Metzger <markus.t.metzger@intel.com> [2015-09-21 16:54:43 +0200]:

> Use the new source interleaving method.  We stick to the /m modifier.
> The old version is broken and there's no point in keeping it as alternative
> to this new version.
> 
>  /* The to_insn_history method of target record-btrace.  */
> diff --git a/gdb/record.c b/gdb/record.c
> index 71ef973..c17b199 100644
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -458,7 +458,7 @@ get_insn_history_modifiers (char **arg)
>  	  switch (*args)
>  	    {
>  	    case 'm':
> -	      modifiers |= DISASSEMBLY_SOURCE_DEPRECATED;
> +	      modifiers |= DISASSEMBLY_SOURCE;
>  	      modifiers |= DISASSEMBLY_FILENAME;
>  	      break;
>  	    case 'r':

I think that we should make /s the official modifier in order to match
the disassembler modifier.

We should definitely keep /m for backwards compatibility, though I
don't have an issue with it's behaviour changing to match /s.  My
concern is more about trying to keep the flags consistent as much as
possible.  The change in flags would need a NEWS and manual update.

Thanks,
Andrew

RE: [PATCH 6/6] btrace: use gdb_disassembly_vec and new source interleaving method

[Metzger, Markus T]

> -----Original Message-----
> From: Andrew Burgess [mailto:andrew.burgess@embecosm.com]
> Sent: Monday, September 21, 2015 11:48 PM
> To: Metzger, Markus T
> Cc: palves@redhat.com; dje@google.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH 6/6] btrace: use gdb_disassembly_vec and new source
> interleaving method

Hello Andrew,

Thanks for your feedback.


> > Use the new source interleaving method.  We stick to the /m modifier.
> > The old version is broken and there's no point in keeping it as alternative
> > to this new version.
> >
> >  /* The to_insn_history method of target record-btrace.  */
> > diff --git a/gdb/record.c b/gdb/record.c
> > index 71ef973..c17b199 100644
> > --- a/gdb/record.c
> > +++ b/gdb/record.c
> > @@ -458,7 +458,7 @@ get_insn_history_modifiers (char **arg)
> >  	  switch (*args)
> >  	    {
> >  	    case 'm':
> > -	      modifiers |= DISASSEMBLY_SOURCE_DEPRECATED;
> > +	      modifiers |= DISASSEMBLY_SOURCE;
> >  	      modifiers |= DISASSEMBLY_FILENAME;
> >  	      break;
> >  	    case 'r':
> 
> I think that we should make /s the official modifier in order to match
> the disassembler modifier.
> 
> We should definitely keep /m for backwards compatibility, though I
> don't have an issue with it's behaviour changing to match /s.  My
> concern is more about trying to keep the flags consistent as much as
> possible.  The change in flags would need a NEWS and manual update.

Adding '/s' is fine with me.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 1/6] disasm: change dump_insns to print a single instruction

[Markus Metzger]

In disasm.c, dump_insns is called to print a sequence of instructions.

Rename it to dump_insn and change it to print only a single instruction
and return its length in bytes.  Move the loop over the desired PC range
and number of instructions printed to its callers.

We will update all but the deprecated callers in subsequent patches.

2015-09-21  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* disasm.c (dump_insns): Rename to ...
	(dump_insn): ... this.  Change it to print a single instruction and
	return its length.  Change parameters and update users.
	(do_mixed_source_and_assembly_deprecated, do_mixed_source_and_assembly)
	(do_assembly_only): Loop over desired PC range and number of
	instructions.
---
 gdb/disasm.c | 202 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 114 insertions(+), 88 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 2b65c6a..981ab7e 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -170,101 +170,89 @@ 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)
+dump_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
+	   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);
 
-  for (pc = low; pc < high;)
+  if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
+			       &line, &unmapped))
     {
-      char *filename = NULL;
-      char *name = NULL;
+      /* 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");
 
-      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);
+  if (filename != NULL)
+    xfree (filename);
+  if (name != NULL)
+    xfree (name);
+
+  ui_file_rewind (stb);
+  if (flags & DISASSEMBLY_RAW_INSN)
+    {
+      CORE_ADDR end_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);
 
-      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");
 
-  if (end_pc != NULL)
-    *end_pc = pc;
-  return num_displayed;
+  return size;
 }
 
 /* The idea here is to present a source-O-centric view of a
@@ -359,6 +347,8 @@ do_mixed_source_and_assembly_deprecated
 
   for (i = 0; i < newlines; i++)
     {
+      CORE_ADDR pc;
+
       /* Print out everything from next_line to the current line.  */
       if (mle[i].line >= next_line)
 	{
@@ -412,9 +402,21 @@ do_mixed_source_and_assembly_deprecated
 	    = make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
 	}
 
-      num_displayed += dump_insns (gdbarch, uiout, di,
-				   mle[i].start_pc, mle[i].end_pc,
-				   how_many, flags, stb, NULL);
+      pc = mle[i].start_pc;
+      while (pc < mle[i].end_pc && (how_many < 0 || num_displayed < how_many))
+	{
+	  int size;
+
+	  size = dump_insn (gdbarch, uiout, di, pc, flags, stb);
+	  if (size <= 0)
+	    break;
+
+	  num_displayed += 1;
+	  pc += size;
+
+	  /* Allow user to bail out with ^C.  */
+	  QUIT;
+	}
 
       /* When we've reached the end of the mle array, or we've seen the last
          assembly range for this source line, close out the list/tuple.  */
@@ -672,9 +674,21 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 	end_pc = min (sal.end, high);
       else
 	end_pc = pc + 1;
-      num_displayed += dump_insns (gdbarch, uiout, di, pc, end_pc,
-				   how_many, flags, stb, &end_pc);
-      pc = end_pc;
+
+      while (pc < end_pc && (how_many < 0 || num_displayed < how_many))
+	{
+	  int size;
+
+	  size = dump_insn (gdbarch, uiout, di, pc, flags, stb);
+	  if (size <= 0)
+	    break;
+
+	  num_displayed += 1;
+	  pc += size;
+
+	  /* Allow user to bail out with ^C.  */
+	  QUIT;
+	}
 
       if (how_many >= 0 && num_displayed >= how_many)
 	break;
@@ -693,13 +707,25 @@ do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
 		  CORE_ADDR low, CORE_ADDR high,
 		  int how_many, int flags, struct ui_file *stb)
 {
-  int num_displayed = 0;
   struct cleanup *ui_out_chain;
+  int num_displayed = 0;
 
   ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
 
-  num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
-                              flags, stb, NULL);
+  while (low < high && (how_many < 0 || num_displayed < how_many))
+    {
+      int size;
+
+      size = dump_insn (gdbarch, uiout, di, low, flags, stb);
+      if (size <= 0)
+	break;
+
+      num_displayed += 1;
+      low += size;
+
+      /* Allow user to bail out with ^C.  */
+      QUIT;
+    }
 
   do_cleanups (ui_out_chain);
 }
-- 
1.8.3.1

[PATCH 3/6] disas: add gdb_disassembly_vec

[Markus Metzger]

Add a new function to disassemble a vector of instructions instead of a
contiguous range of instructions.  The instructions can be in any order
and may originate from different functions.

Change gdb_disassembly to create a vector of instructions from its low,
high, and how_many arguments.

In do_mixed_source_and_assembly, there's a special case for the opening
brace of a function that has its prologue removed.  I tried to preserve
this special case for the first instruction in the vector.

I am relying on the existing tests to make sure that the current "disassemble"
behaviour is preserved.

2015-09-21  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* disasm.h (disas_insn_t): New.  Instantiate a vector of such objects.
	(gdb_disassembly_vec): New.
	* disasm.c (do_mixed_source_and_assembly): Replace the low, high, and
	how_many parameters with a disas_insn_t-vector.  Update users.
	(do_assembly_only):  Replace the low, high, and how_many parameters
	with a disas_insn_t-vector.  Update users.
	(gdb_disassembly): Compute disas_insn_t-vector.  Call
	gdb_disassembly_vec for all but the deprecated source mode.
	(gdb_disassembly_vec): New.
---
 gdb/disasm.c | 233 +++++++++++++++++++++++++++++------------------------------
 gdb/disasm.h |  10 +++
 2 files changed, 125 insertions(+), 118 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 044dcac..f30ef4a 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -472,28 +472,24 @@ do_mixed_source_and_assembly_deprecated
 static void
 do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 			      struct disassemble_info *di,
-			      struct symtab *main_symtab,
-			      CORE_ADDR low, CORE_ADDR high,
-			      int how_many, int flags, struct ui_file *stb)
+			      VEC (disas_insn_t) *insns, int flags,
+			      struct ui_file *stb)
 {
   int newlines = 0;
-  const struct linetable_entry *le, *first_le;
   struct symtab_and_line sal;
-  int i, nlines;
+  int i;
   int out_of_order = 0;
   int next_line = 0;
-  int num_displayed = 0;
   enum print_source_lines_flags psl_flags = 0;
   struct cleanup *cleanups;
   struct cleanup *ui_out_chain;
   struct cleanup *ui_out_tuple_chain;
   struct cleanup *ui_out_list_chain;
-  CORE_ADDR pc;
   struct symtab *last_symtab;
   int last_line;
   htab_t dis_line_table;
-
-  gdb_assert (main_symtab != NULL && SYMTAB_LINETABLE (main_symtab) != NULL);
+  struct disas_insn *insn;
+  unsigned int ix;
 
   /* First pass: collect the list of all source files and lines.
      We do this so that we can only print lines containing code once.
@@ -504,34 +500,12 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   dis_line_table = allocate_dis_line_table ();
   cleanups = make_cleanup_htab_delete (dis_line_table);
 
-  pc = low;
-
-  /* The prologue may be empty, but there may still be a line number entry
-     for the opening brace which is distinct from the first line of code.
-     If the prologue has been eliminated find_pc_line may return the source
-     line after the opening brace.  We still want to print this opening brace.
-     first_le is used to implement this.  */
-
-  nlines = SYMTAB_LINETABLE (main_symtab)->nitems;
-  le = SYMTAB_LINETABLE (main_symtab)->item;
-  first_le = NULL;
-
-  /* Skip all the preceding functions.  */
-  for (i = 0; i < nlines && le[i].pc < low; i++)
-    continue;
-
-  if (i < nlines && le[i].pc < high)
-    first_le = &le[i];
-
-  /* Add lines for every pc value.  */
-  while (pc < high)
+  /* Add lines for every PC value.  */
+  for (ix = 0; VEC_iterate (disas_insn_t, insns, ix, insn); ++ix)
     {
       struct symtab_and_line sal;
-      int length;
 
-      sal = find_pc_line (pc, 0);
-      length = gdb_insn_length (gdbarch, pc);
-      pc += length;
+      sal = find_pc_line (insn->addr, 0);
 
       if (sal.symtab != NULL)
 	maybe_add_dis_line_entry (dis_line_table, sal.symtab, sal.line);
@@ -571,33 +545,47 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 
   last_symtab = NULL;
   last_line = 0;
-  pc = low;
 
-  while (pc < high)
+  for (ix = 0; VEC_iterate (disas_insn_t, insns, ix, insn); ++ix)
     {
       struct linetable_entry *le = NULL;
       struct symtab_and_line sal;
-      CORE_ADDR end_pc;
       int start_preceding_line_to_display = 0;
       int end_preceding_line_to_display = 0;
       int new_source_line = 0;
-      struct disas_insn insn;
 
-      sal = find_pc_line (pc, 0);
+      sal = find_pc_line (insn->addr, 0);
 
       if (sal.symtab != last_symtab)
 	{
 	  /* New source file.  */
 	  new_source_line = 1;
 
-	  /* If this is the first line of output, check for any preceding
-	     lines.  */
-	  if (last_line == 0
-	      && first_le != NULL
-	      && first_le->line < sal.line)
+	  /* The prologue may be empty, but there may still be a line number
+	     entry for the opening brace which is distinct from the first line
+	     of code.  If the prologue has been eliminated find_pc_line may
+	     return the source line after the opening brace.  We still want to
+	     print this opening brace.
+
+	     We print it only once.  Should we encounter the same PC again, we
+	     will just print the corresponding source lines.  */
+	  if (last_line == 0)
 	    {
-	      start_preceding_line_to_display = first_le->line;
-	      end_preceding_line_to_display = sal.line;
+	      const struct linetable_entry *le;
+	      int nlines;
+
+	      nlines = SYMTAB_LINETABLE (sal.symtab)->nitems;
+	      le = SYMTAB_LINETABLE (sal.symtab)->item;
+
+	      /* Skip all the preceding functions.  */
+	      for (i = 0; i < nlines && le[i].pc < insn->addr; i++)
+		continue;
+
+	      if (i < nlines && le[i].pc == insn->addr && le[i].line < sal.line)
+		{
+		  start_preceding_line_to_display = le[i].line;
+		  end_preceding_line_to_display = sal.line;
+		}
 	    }
 	}
       else
@@ -635,7 +623,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       if (new_source_line)
 	{
 	  /* Skip the newline if this is the first instruction.  */
-	  if (pc > low)
+	  if (ix > 0)
 	    ui_out_text (uiout, "\n");
 	  if (ui_out_tuple_chain != NULL)
 	    {
@@ -692,45 +680,14 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 	  ui_out_list_chain
 	    = make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
 	}
-      else
-	{
-	  /* Here we're appending instructions to an existing line.
-	     By construction the very first insn will have a symtab
-	     and follow the new_source_line path above.  */
-	  gdb_assert (ui_out_tuple_chain != NULL);
-	  gdb_assert (ui_out_list_chain != NULL);
-	}
-
-      if (sal.end != 0)
-	end_pc = min (sal.end, high);
-      else
-	end_pc = pc + 1;
-
-      memset (&insn, 0, sizeof (insn));
-      insn.addr = pc;
-
-      while (insn.addr < end_pc && (how_many < 0 || num_displayed < how_many))
-	{
-	  int size;
-
-	  size = dump_insn (gdbarch, uiout, di, &insn, flags, stb);
-	  if (size <= 0)
-	    break;
-
-	  num_displayed += 1;
-	  insn.addr += size;
-
-	  /* Allow user to bail out with ^C.  */
-	  QUIT;
-	}
 
-      pc = insn.addr;
-
-      if (how_many >= 0 && num_displayed >= how_many)
-	break;
+      dump_insn (gdbarch, uiout, di, insn, flags, stb);
 
       last_symtab = sal.symtab;
       last_line = sal.line;
+
+      /* Allow user to bail out with ^C.  */
+      QUIT;
     }
 
   do_cleanups (ui_out_chain);
@@ -739,29 +696,18 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 
 static void
 do_assembly_only (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)
+		  struct disassemble_info * di, VEC (disas_insn_t) *insns,
+		  int flags, struct ui_file *stb)
 {
   struct cleanup *ui_out_chain;
-  struct disas_insn insn;
-  int num_displayed = 0;
+  struct disas_insn *insn;
+  unsigned int ix;
 
   ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
 
-  memset (&insn, 0, sizeof (insn));
-  insn.addr = low;
-
-  while (insn.addr < high && (how_many < 0 || num_displayed < how_many))
+  for (ix = 0; VEC_iterate (disas_insn_t, insns, ix, insn); ++ix)
     {
-      int size;
-
-      size = dump_insn (gdbarch, uiout, di, &insn, flags, stb);
-      if (size <= 0)
-	break;
-
-      num_displayed += 1;
-      insn.addr += size;
+      dump_insn (gdbarch, uiout, di, insn, flags, stb);
 
       /* Allow user to bail out with ^C.  */
       QUIT;
@@ -817,30 +763,81 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 		 char *file_string, int flags, int how_many,
 		 CORE_ADDR low, CORE_ADDR high)
 {
-  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 symtab *symtab;
-  struct linetable_entry *le = NULL;
-  int nlines = -1;
+  VEC (disas_insn_t) *insns;
+  struct cleanup *cleanups;
+  int num_displayed;
+
+  if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
+    {
+      struct symtab *symtab;
 
-  /* Assume symtab is valid for whole PC range.  */
-  symtab = find_pc_line_symtab (low);
+      /* Assume symtab is valid for whole PC range.  */
+      symtab = find_pc_line_symtab (low);
 
-  if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL)
-    nlines = SYMTAB_LINETABLE (symtab)->nitems;
+      if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL
+	  && SYMTAB_LINETABLE (symtab)->nitems > 0)
+	{
+	  struct ui_file *stb;
+	  struct disassemble_info di;
+
+	  stb = mem_fileopen ();
+	  cleanups = make_cleanup_ui_file_delete (stb);
+	  di = gdb_disassemble_info (gdbarch, stb);
+
+	  do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
+						   low, high, how_many, flags,
+						   stb);
+	  do_cleanups (cleanups);
+	  gdb_flush (gdb_stdout);
+	  return;
+	}
 
-  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
-      || nlines <= 0)
-    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
+      /* If we don't have sources, fall through to gdb_disassembly_vec.  */
+      flags &= ~DISASSEMBLY_SOURCE_DEPRECATED;
+    }
 
-  else if (flags & DISASSEMBLY_SOURCE)
-    do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
-				  how_many, flags, stb);
+  insns = NULL;
+  cleanups = make_cleanup (VEC_cleanup (disas_insn_t), &insns);
 
-  else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
-    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
-					     low, high, how_many, flags, stb);
+  num_displayed = 0;
+  while (low < high && (how_many < 0 || num_displayed < how_many))
+    {
+      struct disas_insn *insn;
+      int size;
+
+      insn = VEC_safe_push (disas_insn_t, insns, NULL);
+      insn->addr = low;
+      insn->number = 0;
+      insn->is_speculative = 0;
+
+      size = gdb_insn_length (gdbarch, low);
+      if (size <= 0)
+	break;
+
+      num_displayed += 1;
+      low += size;
+    }
+
+  gdb_disassembly_vec (gdbarch, uiout, flags, insns);
+  do_cleanups (cleanups);
+}
+
+/* See disasm.h.  */
+
+void gdb_disassembly_vec (struct gdbarch *gdbarch, struct ui_out *uiout,
+			  int flags, VEC (disas_insn_t) *insns)
+{
+  struct ui_file *stb = mem_fileopen ();
+  struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
+  struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
+
+  /* We don't support the deprecated mixed source and disassembly mode.  */
+  gdb_assert ((flags & DISASSEMBLY_SOURCE_DEPRECATED) == 0);
+
+  if ((flags & DISASSEMBLY_SOURCE) != 0)
+    do_mixed_source_and_assembly (gdbarch, uiout, &di, insns, flags, stb);
+  else
+    do_assembly_only (gdbarch, uiout, &di, insns, flags, stb);
 
   do_cleanups (cleanups);
   gdb_flush (gdb_stdout);
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 3d6f5bb..138f54c 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -46,6 +46,11 @@ struct disas_insn {
   unsigned int is_speculative:1;
 };
 
+/* A vector of disas_insn.  */
+
+typedef struct disas_insn disas_insn_t;
+DEF_VEC_O (disas_insn_t);
+
 /* Return a filled in disassemble_info object for use by gdb.  */
 
 extern struct disassemble_info gdb_disassemble_info (struct gdbarch *gdbarch,
@@ -55,6 +60,11 @@ 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);
 
+/* Print disassembled instructions in INSNS in vector order.  */
+
+extern void gdb_disassembly_vec (struct gdbarch *gdbarch, struct ui_out *uiout,
+				 int flags, VEC (disas_insn_t) *insns);
+
 /* Print the instruction at address MEMADDR in debugged memory,
    on STREAM.  Returns the length of the instruction, in bytes,
    and, if requested, the number of branch delay slot instructions.  */
-- 
1.8.3.1

Re: [PATCH 3/6] disas: add gdb_disassembly_vec

[Pedro Alves]

On 09/21/2015 03:54 PM, Markus Metzger wrote:
> Add a new function to disassemble a vector of instructions instead of a
> contiguous range of instructions.  The instructions can be in any order
> and may originate from different functions.
> 
> Change gdb_disassembly to create a vector of instructions from its low,
> high, and how_many arguments.

I wonder whether the representation could/should be based on a vector
of struct mem_range's instead of a vector of instructions.  I'm assuming
the normal case is that we're disassembling ranges of more than
one instruction.  Just seems wasteful for something like

  (gdb) disassemble 0x3000000000,0x7000000000

to allocate so much memory.  But maybe I simply misunderstood.

Thanks,
Pedro Alves

RE: [PATCH 3/6] disas: add gdb_disassembly_vec

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, October 9, 2015 2:50 PM
> To: Metzger, Markus T; dje@google.com
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 3/6] disas: add gdb_disassembly_vec
> 
> On 09/21/2015 03:54 PM, Markus Metzger wrote:
> > Add a new function to disassemble a vector of instructions instead of a
> > contiguous range of instructions.  The instructions can be in any order
> > and may originate from different functions.
> >
> > Change gdb_disassembly to create a vector of instructions from its low,
> > high, and how_many arguments.
> 
> I wonder whether the representation could/should be based on a vector
> of struct mem_range's instead of a vector of instructions.  I'm assuming
> the normal case is that we're disassembling ranges of more than
> one instruction.  Just seems wasteful for something like
> 
>   (gdb) disassemble 0x3000000000,0x7000000000
> 
> to allocate so much memory.  But maybe I simply misunderstood.

You didn't.  It really is a vector of instructions - we do need additional information
for each instruction (see [PATCH 2/6]).  Also the instructions come in the order
in which they were executed; not in the order of their memory address.

I expected the normal usage of "disassemble" to be one function or maybe
a few dozen instructions at a time.

If we really want to support many thousands of instructions, the whole idea
breaks down.  We'd rather keep the two algorithms separate or we break
the disassemble algorithm apart so that the components can be reused.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 3/6] disas: add gdb_disassembly_vec

[Andrew Burgess]

* Metzger, Markus T <markus.t.metzger@intel.com> [2015-10-09 13:16:30 +0000]:

> > -----Original Message-----
> > From: Pedro Alves [mailto:palves@redhat.com]
> > Sent: Friday, October 9, 2015 2:50 PM
> > To: Metzger, Markus T; dje@google.com
> > Cc: gdb-patches@sourceware.org
> > Subject: Re: [PATCH 3/6] disas: add gdb_disassembly_vec
> > 
> > On 09/21/2015 03:54 PM, Markus Metzger wrote:
> > > Add a new function to disassemble a vector of instructions instead of a
> > > contiguous range of instructions.  The instructions can be in any order
> > > and may originate from different functions.
> > >
> > > Change gdb_disassembly to create a vector of instructions from its low,
> > > high, and how_many arguments.
> > 
> > I wonder whether the representation could/should be based on a vector
> > of struct mem_range's instead of a vector of instructions.  I'm assuming
> > the normal case is that we're disassembling ranges of more than
> > one instruction.  Just seems wasteful for something like
> > 
> >   (gdb) disassemble 0x3000000000,0x7000000000
> > 
> > to allocate so much memory.  But maybe I simply misunderstood.
> 
> You didn't.  It really is a vector of instructions - we do need additional information
> for each instruction (see [PATCH 2/6]).  Also the instructions come in the order
> in which they were executed; not in the order of their memory address.
> 
> I expected the normal usage of "disassemble" to be one function or maybe
> a few dozen instructions at a time.

This is definitely not correct.

This is probably a sensible assumption for CLI users, however,
disassemble requests from the MI are frequently larger, and based
around address ranges, happily spanning both function and compilation
unit boundaries.

The reason is that the driver might be trying to fill a possibly large
window (with look ahead) with disassembled instructions; at the same
time the driver will ask for the source information so that the user
can click a disassembled instruction and instantly be taken to the
corresponding source line.

I guess whether this is a "large" number of instruction is up for
debate, but I think we should assume that a couple of hundred is not
out of the question.

I don't know if this makes much of a difference to your proposed
change, but please do bear this in mind.

Thanks,
Andrew

Re: [PATCH 3/6] disas: add gdb_disassembly_vec

[Doug Evans]

On Fri, Oct 9, 2015 at 6:16 AM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Friday, October 9, 2015 2:50 PM
>> To: Metzger, Markus T; dje@google.com
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 3/6] disas: add gdb_disassembly_vec
>>
>> On 09/21/2015 03:54 PM, Markus Metzger wrote:
>> > Add a new function to disassemble a vector of instructions instead of a
>> > contiguous range of instructions.  The instructions can be in any order
>> > and may originate from different functions.
>> >
>> > Change gdb_disassembly to create a vector of instructions from its low,
>> > high, and how_many arguments.
>>
>> I wonder whether the representation could/should be based on a vector
>> of struct mem_range's instead of a vector of instructions.  I'm assuming
>> the normal case is that we're disassembling ranges of more than
>> one instruction.  Just seems wasteful for something like
>>
>>   (gdb) disassemble 0x3000000000,0x7000000000
>>
>> to allocate so much memory.  But maybe I simply misunderstood.
>
> You didn't.  It really is a vector of instructions - we do need additional information
> for each instruction (see [PATCH 2/6]).  Also the instructions come in the order
> in which they were executed; not in the order of their memory address.
>
> I expected the normal usage of "disassemble" to be one function or maybe
> a few dozen instructions at a time.
>
> If we really want to support many thousands of instructions, the whole idea
> breaks down.  We'd rather keep the two algorithms separate or we break
> the disassemble algorithm apart so that the components can be reused.

Thousands of instructions is something we need to handle,
as is very large source files (I've seen machine generated source
files that aren't small).
For reference sake, one concern I have is a very large source file
with a very large function at the end. IIUC, the current patch is O(NxM).
[N = #lines with code in source file, M = #instructions being disassembled]

[PATCH 4/6] disasm: use entire line table in line_has_code_p

[Markus Metzger]

In do_mixed_source_and_assembly, we first collect all line entries matching
the to-be-printed PC range.  In a second pass, we use the collected lines to
determine whether there is code for a given source line.

Given a consecutive range of instructions, we are guaranteed to have seen
all the relevant code.

Given a vector of non-consecutive instructions, however, we have seen only
a fraction of the relevant code.

Change line_has_code_p to consider the entire line table.  This obsoletes the
first pass.  Remove it.

This will make the line-has-code check a lot more expensive.  I don't think
that the slowdown will be noticeable on today's processors, though.

2015-09-21  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* disasm.c (struct dis_line_entry, hash_dis_line_entry)
	(eq_dis_line_entry, allocate_dis_line_table)
	(maybe_add_dis_line_entry): Remove.
	(line_has_code_p): Remove table parameter.  Update users.
	Use entire line table.
	(do_mixed_source_and_assembly): Remove first pass.
---
 gdb/disasm.c | 112 ++++++++---------------------------------------------------
 1 file changed, 14 insertions(+), 98 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index f30ef4a..32e4bc1 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -43,80 +43,24 @@ struct deprecated_dis_line_entry
   CORE_ADDR end_pc;
 };
 
-/* This Structure is used to store line number information.
-   We need a different sort of line table from the normal one cuz we can't
-   depend upon implicit line-end pc's for lines to do the
-   reordering in this function.  */
-
-struct dis_line_entry
-{
-  struct symtab *symtab;
-  int line;
-};
-
-/* Hash function for dis_line_entry.  */
-
-static hashval_t
-hash_dis_line_entry (const void *item)
-{
-  const struct dis_line_entry *dle = item;
-
-  return htab_hash_pointer (dle->symtab) + dle->line;
-}
-
-/* Equal function for dis_line_entry.  */
+/* Return non-zero if LINE appears in SYMTAB's line table.  */
 
 static int
-eq_dis_line_entry (const void *item_lhs, const void *item_rhs)
+line_has_code_p (struct symtab *symtab, int line)
 {
-  const struct dis_line_entry *lhs = item_lhs;
-  const struct dis_line_entry *rhs = item_rhs;
-
-  return (lhs->symtab == rhs->symtab
-	  && lhs->line == rhs->line);
-}
+  struct linetable *ltable;
+  int nlines, ix;
 
-/* Create the table to manage lines for mixed source/disassembly.  */
+  ltable = SYMTAB_LINETABLE (symtab);
+  if (ltable == NULL)
+    return 0;
 
-static htab_t
-allocate_dis_line_table (void)
-{
-  return htab_create_alloc (41,
-			    hash_dis_line_entry, eq_dis_line_entry,
-			    xfree, xcalloc, xfree);
-}
+  nlines = ltable->nitems;
+  for (ix = 0; ix < nlines; ++ix)
+    if (ltable->item[ix].line == line)
+      return 1;
 
-/* Add DLE to TABLE.
-   Returns 1 if added, 0 if already present.  */
-
-static void
-maybe_add_dis_line_entry (htab_t table, struct symtab *symtab, int line)
-{
-  void **slot;
-  struct dis_line_entry dle, *dlep;
-
-  dle.symtab = symtab;
-  dle.line = line;
-  slot = htab_find_slot (table, &dle, INSERT);
-  if (*slot == NULL)
-    {
-      dlep = XNEW (struct dis_line_entry);
-      dlep->symtab = symtab;
-      dlep->line = line;
-      *slot = dlep;
-    }
-}
-
-/* Return non-zero if SYMTAB, LINE are in TABLE.  */
-
-static int
-line_has_code_p (htab_t table, struct symtab *symtab, int line)
-{
-  struct dis_line_entry dle;
-
-  dle.symtab = symtab;
-  dle.line = line;
-  return htab_find (table, &dle) != NULL;
+  return 0;
 }
 
 /* Like target_read_memory, but slightly different parameters.  */
@@ -481,39 +425,15 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   int out_of_order = 0;
   int next_line = 0;
   enum print_source_lines_flags psl_flags = 0;
-  struct cleanup *cleanups;
   struct cleanup *ui_out_chain;
   struct cleanup *ui_out_tuple_chain;
   struct cleanup *ui_out_list_chain;
   struct symtab *last_symtab;
   int last_line;
-  htab_t dis_line_table;
   struct disas_insn *insn;
   unsigned int ix;
 
-  /* First pass: collect the list of all source files and lines.
-     We do this so that we can only print lines containing code once.
-     We try to print the source text leading up to the next instruction,
-     but if that text is for code that will be disassembled later, then
-     we'll want to defer printing it until later with its associated code.  */
-
-  dis_line_table = allocate_dis_line_table ();
-  cleanups = make_cleanup_htab_delete (dis_line_table);
-
-  /* Add lines for every PC value.  */
-  for (ix = 0; VEC_iterate (disas_insn_t, insns, ix, insn); ++ix)
-    {
-      struct symtab_and_line sal;
-
-      sal = find_pc_line (insn->addr, 0);
-
-      if (sal.symtab != NULL)
-	maybe_add_dis_line_entry (dis_line_table, sal.symtab, sal.line);
-    }
-
-  /* Second pass: print the disassembly.
-
-     Output format, from an MI perspective:
+  /* Output format, from an MI perspective:
        The result is a ui_out list, field name "asm_insns", where elements have
        name "src_and_asm_line".
        Each element is a tuple of source line specs (field names line, file,
@@ -525,9 +445,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
      which is where we put file name and source line contents output.
 
      Cleanup usage:
-     cleanups:
-       For things created at the beginning of this function and need to be
-       kept until the end of this function.
      ui_out_chain
        Handles the outer "asm_insns" list.
      ui_out_tuple_chain
@@ -601,7 +518,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 		     not associated with code that we'll print later.  */
 		  for (l = sal.line - 1; l > last_line; --l)
 		    {
-		      if (line_has_code_p (dis_line_table, sal.symtab, l))
+		      if (line_has_code_p (sal.symtab, l))
 			break;
 		    }
 		  if (l < sal.line - 1)
@@ -691,7 +608,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
     }
 
   do_cleanups (ui_out_chain);
-  do_cleanups (cleanups);
 }
 
 static void
-- 
1.8.3.1

[PATCH 5/6] disasm: determine preceding lines independent of last_line

[Markus Metzger]

In do_mixed_source_and_assembly, we check for source lines without code
between the last line that was printed and the next line to be printed.
We print those extra source lines before the line for the next instruction.

Since instructions can now be in any order, it is no longer guaranteed that
last_line corresponds to the previous instruction.

Change the boundary from last_line to the first line in the same function and
remove the first-line-to-be-printed check.

This will now print source lines without code also before the first
instruction.

2015-09-21  Markus Metzger <markus.t.metzger@intel.com>

	* disasm.c: Include block.h
	(first_line_in_pc_function): New.
	(do_mixed_source_and_assembly): Search for lines without code until the
	first line in the same function.
---
 gdb/disasm.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 32e4bc1..2503657 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -25,6 +25,7 @@
 #include "gdbcore.h"
 #include "dis-asm.h"
 #include "source.h"
+#include "block.h"
 
 /* Disassemble functions.
    FIXME: We should get rid of all the duplicate code in gdb that does
@@ -63,6 +64,54 @@ line_has_code_p (struct symtab *symtab, int line)
   return 0;
 }
 
+/* Return the first line in the function that contains PC or zero if the line
+   can't be determined.  */
+
+static int
+first_line_in_pc_function (struct symtab *symtab, CORE_ADDR pc)
+{
+  const struct symbol *sym;
+  const struct block *block;
+  struct linetable *ltable;
+  CORE_ADDR high, low;
+  int nlines, ix, first;
+
+  sym = find_pc_function (pc);
+  if (sym == NULL)
+    return 0;
+
+  block = SYMBOL_BLOCK_VALUE (sym);
+  if (block == NULL)
+    return 0;
+
+  ltable = SYMTAB_LINETABLE (symtab);
+  if (ltable == NULL)
+    return 0;
+
+  low = BLOCK_START (block);
+  high = BLOCK_END (block);
+  nlines = ltable->nitems;
+  first = INT_MAX;
+
+  /* Skip preceding functions.  */
+  for (ix = 0; ix < nlines && ltable->item[ix].pc < low ; ++ix);
+
+  /* Determine the first line in the function PC range.  */
+  for (; ix < nlines && ltable->item[ix].pc < high; ++ix)
+    {
+      int line = ltable->item[ix].line;
+
+      if (line != 0 && line < first)
+	first = line;
+    }
+
+  /* Return zero if we didn't find any line entry.  */
+  if (first == INT_MAX)
+    first = 0;
+
+ return first;
+}
+
 /* Like target_read_memory, but slightly different parameters.  */
 static int
 dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
@@ -510,13 +559,16 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 	  /* Same source file as last time.  */
 	  if (sal.symtab != NULL)
 	    {
-	      if (sal.line > last_line + 1 && last_line != 0)
+	      int first_line;
+
+	      /* Print preceding source lines not associated with code.
+		 Restrict the search to the current function.  */
+	      first_line = first_line_in_pc_function (sal.symtab, insn->addr);
+	      if (first_line > 0)
 		{
 		  int l;
 
-		  /* Several preceding source lines.  Print the trailing ones
-		     not associated with code that we'll print later.  */
-		  for (l = sal.line - 1; l > last_line; --l)
+		  for (l = sal.line - 1; l >= first_line; --l)
 		    {
 		      if (line_has_code_p (sal.symtab, l))
 			break;
-- 
1.8.3.1

RE: [PATCH 0/6] disasm, record: fix "record instruction-history /m"

[Metzger, Markus T]

> -----Original Message-----
> From: Metzger, Markus T
> Sent: Monday, September 21, 2015 4:55 PM
> To: palves@redhat.com; dje@google.com
> Cc: gdb-patches@sourceware.org
> Subject: [PATCH 0/6] disasm, record: fix "record instruction-history /m"


> Markus Metzger (6):
>   disasm: change dump_insns to print a single instruction
>   disasm: add struct disas_insn to describe to-be-disassembled
>     instruction
>   disas: add gdb_disassembly_vec
>   disasm: use entire line table in line_has_code_p
>   disasm: determine preceding lines independent of last_line
>   btrace: use gdb_disassembly_vec and new source interleaving method

Given the concerns about increased memory consumption and run-time
overhead in patch 3 and the changes to the source interleaving algorithm
in patches 4 and 5, I'd go with a modified version of my original RFC, i.e.

  - patches 1 and 2 from this series
  - the rfc patch to interleave sources in record-btrace.c
  - patch 6 from this series

This will leave us with two source interleaving algorithms, one for a
consecutive range of memory, and one for a sequence of instructions
in the order in which they were recorded.

Both will use a slightly modified dump_insn to print instruction tuples.

I'm dropping the idea of preparing a vector of instructions to print and
and of trying to shoehorn record instruction-history's source interleaving
into do_mixed_source_and_assembly.

Does that sound OK?

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 0/6] disasm, record: fix "record instruction-history /m"

[Doug Evans]

On Mon, Oct 12, 2015 at 7:16 AM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>> -----Original Message-----
>> From: Metzger, Markus T
>> Sent: Monday, September 21, 2015 4:55 PM
>> To: palves@redhat.com; dje@google.com
>> Cc: gdb-patches@sourceware.org
>> Subject: [PATCH 0/6] disasm, record: fix "record instruction-history /m"
>
>
>> Markus Metzger (6):
>>   disasm: change dump_insns to print a single instruction
>>   disasm: add struct disas_insn to describe to-be-disassembled
>>     instruction
>>   disas: add gdb_disassembly_vec
>>   disasm: use entire line table in line_has_code_p
>>   disasm: determine preceding lines independent of last_line
>>   btrace: use gdb_disassembly_vec and new source interleaving method
>
> Given the concerns about increased memory consumption and run-time
> overhead in patch 3 and the changes to the source interleaving algorithm
> in patches 4 and 5, I'd go with a modified version of my original RFC, i.e.
>
>   - patches 1 and 2 from this series
>   - the rfc patch to interleave sources in record-btrace.c
>   - patch 6 from this series
>
> This will leave us with two source interleaving algorithms, one for a
> consecutive range of memory, and one for a sequence of instructions
> in the order in which they were recorded.
>
> Both will use a slightly modified dump_insn to print instruction tuples.
>
> I'm dropping the idea of preparing a vector of instructions to print and
> and of trying to shoehorn record instruction-history's source interleaving
> into do_mixed_source_and_assembly.
>
> Does that sound OK?

I think so (devil is in the details).
Supporting a vector of instructions sounds fine to me though.
There doesn't have to be one entry point to the disassembler API.
[5 would be bad, but 2 is ok: one that takes a range, one that takes a vector.]

RE: [PATCH 0/6] disasm, record: fix "record instruction-history /m"

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Doug Evans
> Sent: Sunday, October 18, 2015 11:17 PM
> To: Metzger, Markus T
> Cc: palves@redhat.com; dje@google.com; gdb-patches@sourceware.org
> Subject: Re: [PATCH 0/6] disasm, record: fix "record instruction-history /m"

Hi Doug,

> > Given the concerns about increased memory consumption and run-time
> > overhead in patch 3 and the changes to the source interleaving algorithm
> > in patches 4 and 5, I'd go with a modified version of my original RFC, i.e.
> >
> >   - patches 1 and 2 from this series
> >   - the rfc patch to interleave sources in record-btrace.c
> >   - patch 6 from this series
> >
> > This will leave us with two source interleaving algorithms, one for a
> > consecutive range of memory, and one for a sequence of instructions
> > in the order in which they were recorded.
> >
> > Both will use a slightly modified dump_insn to print instruction tuples.
> >
> > I'm dropping the idea of preparing a vector of instructions to print and
> > and of trying to shoehorn record instruction-history's source interleaving
> > into do_mixed_source_and_assembly.
> >
> > Does that sound OK?
> 
> I think so (devil is in the details).
> Supporting a vector of instructions sounds fine to me though.
> There doesn't have to be one entry point to the disassembler API.
> [5 would be bad, but 2 is ok: one that takes a range, one that takes a vector.]

Patches 4 and 5 of this series modify the algorithm to make it work for instructions
in random order.  I have to drop the source line hash table, for example, and
perform a much more expensive search.

Also if we don't want to create a temporary vector of instructions, I'd have to
extract the local variables into an iteration state struct and split the algorithm
into small pieces.

It's possible, of course, but I'm afraid you won't recognize your algorithm,
afterwards.

I sent v2 of the series as outlined above.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928