Re: [PATCH 2/2] gdb: Ensure disassembler covers requested address range.

[Doug Evans <xdje42 at gmail dot com>]

Andrew Burgess <andrew.burgess@embecosm.com> writes:
> Doug,
>
> Thanks for the review.  I don't have a new revision of this patch yet,
> I would like to discuss one of the issue you raised first.  Feedback
> inline.
>
> * Doug Evans <xdje42@gmail.com> [2015-09-16 21:03:46 -0700]:
>
>> Andrew Burgess <andrew.burgess@embecosm.com> writes:
>> > If gdb is asked to disassemble a particular address range, but that
>> > address range spans the end of a symbol table, then currently gdb will
>> > exit the disassembler early.
>> >
>> > This commit makes the disassembler handle passing over the boundary
>> > between different symbol tables, and even over regions where there is no
>> > symbol table.
>> >
>> > The only reason that the disassembler will now not disassemble a
>> > complete address range as asked is if the inferior memory can't be
>> > read within that region.
>> >
>> > The existing test for disassembling over compilation unit boundary is
>> > not sufficient, in fact the behaviour has regressed since the test was
>> > first added in 2011 (commit 9011945e46bf8b) and the test failed to
>> > highlight this regression.
>> >
>> > This commit removes the old test in this area, and adds new, more
>> > comprehensive tests that cover the following features:
>> >
>> >  1. Disassembly over the end of a compilation unit and into the next
>> >     compilation unit should work.
>> >
>> >  2. When different compilation units do, or do not, have debug
>> >     information, we should still get some disassembly output, even when
>> >     asking for disassembly with source code.
>> >
>> >  3. When asking for disassembly with source code, we should, where
>> >     possible provide that source code, even when earlier compilation
>> >     units within the region being disassembled don't have debug
>> >     information.
>> >
>> > gdb/ChangeLog:
>> >
>> > 	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
>> > 	count, and take extra parameter.  Fill in final_pc parameter.
>> > 	(do_mixed_source_and_assembly): Likewise.
>> > 	(do_assembly_only): Likewise, also return from function when we've
>> > 	displayed enough instructions.
>> > 	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
>> > 	raw instructions as required.
>> >
>> > gdb/testsuite/ChangeLog:
>> >
>> > 	* gdb.base/disasm-end-cu-1.c: Deleted.
>> > 	* gdb.base/disasm-end-cu-2.c: Deleted.
>> > 	* gdb.base/disasm-end-cu.exp: Deleted.
>> > 	* gdb.base/disasm-multi-cu-1.c: New file.
>> > 	* gdb.base/disasm-multi-cu-2.c: New file.
>> > 	* gdb.base/disasm-multi-cu-3.c: New file.
>> > 	* gdb.base/disasm-multi-cu.exp: New file.
>> > 	* gdb.base/disasm-multi-cu.h: New file.
>> 
>> Hi. Some comments inline.
>> 
>> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> > index 463b1b9..7ff35a2 100644
>> > --- a/gdb/ChangeLog
>> > +++ b/gdb/ChangeLog
>> > @@ -1,5 +1,15 @@
>> >  2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
>> >  
>> > +	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
>> > +	count, and take extra parameter.  Fill in final_pc parameter.
>> > +	(do_mixed_source_and_assembly): Likewise.
>> > +	(do_assembly_only): Likewise, also return from function when we've
>> > +	displayed enough instructions.
>> > +	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
>> > +	raw instructions as required.
>> > +
>> > +2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
>> > +
>> >  	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
>> >  	asm_insns list.
>> >  	(do_mixed_source_and_assembly): Likewise.
>> > diff --git a/gdb/disasm.c b/gdb/disasm.c
>> > index 3032090..9199eb2 100644
>> > --- a/gdb/disasm.c
>> > +++ b/gdb/disasm.c
>> > @@ -274,12 +274,12 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >  
>> >     N.B. This view is deprecated.  */
>> >  
>> > -static void
>> > +static int
>> >  do_mixed_source_and_assembly_deprecated
>> >    (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >     struct disassemble_info *di, struct symtab *symtab,
>> >     CORE_ADDR low, CORE_ADDR high,
>> > -   int how_many, int flags, struct ui_file *stb)
>> > +   int how_many, int flags, struct ui_file *stb, CORE_ADDR *final_pc)
>> >  {
>> >    int newlines = 0;
>> >    int nlines;
>> > @@ -293,6 +293,7 @@ do_mixed_source_and_assembly_deprecated
>> >    enum print_source_lines_flags psl_flags = 0;
>> >    struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
>> >    struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
>> > +  CORE_ADDR next_pc = low;
>> >  
>> >    gdb_assert (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL);
>> >  
>> > @@ -411,7 +412,7 @@ do_mixed_source_and_assembly_deprecated
>> >  
>> >        num_displayed += dump_insns (gdbarch, uiout, di,
>> >  				   mle[i].start_pc, mle[i].end_pc,
>> > -				   how_many, flags, stb, NULL);
>> > +				   how_many, flags, stb, &next_pc);
>> >  
>> >        /* 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.  */
>> > @@ -426,6 +427,11 @@ do_mixed_source_and_assembly_deprecated
>> >        if (how_many >= 0 && num_displayed >= how_many)
>> >  	break;
>> >      }
>> > +
>> > +  if (final_pc != NULL)
>> > +    *final_pc = next_pc;
>> > +
>> > +  return num_displayed;
>> >  }
>> >  
>> >  /* The idea here is to present a source-O-centric view of a
>> > @@ -433,12 +439,13 @@ do_mixed_source_and_assembly_deprecated
>> >     in source order, with (possibly) out of order assembly
>> >     immediately following.  */
>> 
>> Bleah. This function comment needs updating. Mea culpa.
>> I'll get to it after this patch (to not interfere with it).
>
> If you're going to do that, do you think you could rename the
> function to deprecated_do_mixed_source_and_assembly.  All the other
> deprecated functions in gdb, and almost all of the deprecated
> variables use a prefix rather than suffix approach - it just makes
> finding deprecated stuff easier :)

Sure.

>> 
>> >  
>> > -static void
>> > +static int
>> >  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)
>> > +			      int how_many, int flags, struct ui_file *stb,
>> > +			      CORE_ADDR *final_pc)
>> >  {
>> >    int newlines = 0;
>> >    const struct linetable_entry *le, *first_le;
>> > @@ -455,6 +462,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >    struct symtab *last_symtab;
>> >    int last_line;
>> >    htab_t dis_line_table;
>> > +  CORE_ADDR end_pc = low;
>> >  
>> >    gdb_assert (main_symtab != NULL && SYMTAB_LINETABLE (main_symtab) != NULL);
>> >  
>> > @@ -537,7 +545,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >      {
>> >        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;
>> > @@ -676,18 +683,39 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >      }
>> >  
>> >    do_cleanups (cleanups);
>> > +
>> > +  if (final_pc != NULL)
>> > +    *final_pc = end_pc;
>> > +
>> > +  return num_displayed;
>> >  }
>> >  
>> > -static void
>> > +static int
>> >  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)
>> > +		  int how_many, int flags, struct ui_file *stb,
>> > +		  CORE_ADDR *final_pc)
>> >  {
>> > -  int num_displayed = 0;
>> > +  CORE_ADDR npc;
>> > +  int total = 0;
>> > +
>> > +  do
>> > +    {
>> > +      total += dump_insns (gdbarch, uiout, di, low, high,
>> > +			   how_many, flags, stb, &npc);
>> > +      if (npc >= high || total >= how_many)
>> > +        break;
>> > +      if (low == npc)
>> > +	low = npc + 1;
>> 
>> npc + 1?
>> Perhaps it's correct, but at a glance I don't see how.
>
> No you're not.  This should have more comments.  I think on revision
> I'll remove this change for now.
>
> These disassembler changes were pulled out of an even larger
> disassembler patch that I have had say around for ages.
>
> The addition of +1 both here, and further down are part of addressing
> disassembling over unreadable memory.  The +1 was part of ensuring
> that the disassembler continued to make progress.
>
> With out the supporting changes though the +1 alone does not make a
> lot of sense, my only defence for leaving it in was that it makes the
> gdb side of the disassembler super-paranoid: if for any reason the
> disassembler fails to make progress gdb will still nudge the address
> on.  However, in general gdb is not written that way so I should
> probably drop this change.
>
>>
>> > +      else
>> > +	low = npc;
>> > +    } while (1);
>> 
>> I think(!) convention requires the "while (1);" to go on the next line.
>> 
>> The higher order question, though, is why is this loop needed?
>> IOW, what does the loop do that dump_insns doesn't already do?
>> Am I missing something?
>
> This relates to the above point and should be removed (for now), the
> assumption will be that dump_insns will always cover the requested
> range.
>
>> > +
>> > +  if (final_pc != NULL)
>> > +    *final_pc = npc;
>> >  
>> > -  num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
>> > -                              flags, stb, NULL);
>> > +  return total;
>> >  }
>> >  
>> >  /* Initialize the disassemble info struct ready for the specified
>> > @@ -740,31 +768,72 @@ 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 symtab *symtab;
>> > -  struct linetable_entry *le = NULL;
>> > -  int nlines = -1;
>> > -
>> > -  /* 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;
>> >  
>> >    /* This cleanup is inner to CLEANUPS, so we don't need a separate
>> >       variable to track it.  */
>> >    (void) make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
>> >  
>> > -  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
>> > -      || nlines <= 0)
>> > -    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
>> > +  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE)))
>> > +    (void) do_assembly_only (gdbarch, uiout, &di, low, high,
>> > +			     how_many, flags, stb, NULL);
>> > +  else if (flags & (DISASSEMBLY_SOURCE | DISASSEMBLY_SOURCE_DEPRECATED))
>> > +    {
>> > +      /* We want to disassemble with source information, however, if such
>> > +	 information is not available then we'd rather have raw assembly
>> > +	 than nothing.  Here we loop until the entire range is
>> > +	 disassembled, giving source information where possible.  */
>> > +      while (low < high)
>> > +	{
>> > +	  int nlines = -1;
>> 
>> No need to init nlines to -1 twice.
>> I'd say delete this init.
>
> My mistake.  Will fix.
>
>> 
>> > +	  CORE_ADDR prev_low = low;
>> > +	  struct symtab *symtab;
>> > +	  struct linetable_entry *le = NULL;
>> > +	  int num_displayed = 0;
>> >  
>> > -  else if (flags & DISASSEMBLY_SOURCE)
>> > -    do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
>> > -				  how_many, flags, stb);
>> > +	  /* Attempt to find a symtab for the PC range.  */
>> > +	  nlines = -1;
>> > +	  symtab = find_pc_line_symtab (low);
>> >  
>> > -  else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
>> > -    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
>> > -					     low, high, how_many, flags, stb);
>> > +	  if (symtab != NULL && symtab->linetable != NULL)
>> > +	    {
>> > +	      /* Convert the linetable to a bunch of my_line_entry's.  */
>> > +	      le = SYMTAB_LINETABLE (symtab)->item;
>> > +	      nlines = SYMTAB_LINETABLE (symtab)->nitems;
>> > +
>> > +	      if (flags & DISASSEMBLY_SOURCE)
>> > +		num_displayed +=
>> > +		  do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab,
>> > +						low, high, how_many, flags,
>> > +						stb, &low);
>> 
>> Storing final_pc in low here is awkward.
>> Can you store it in end_pc or some such first?
>
> OK, will rename some of the variable around this area.
>
>> 
>> > +	      else
>> > +		num_displayed +=
>> > +		  do_mixed_source_and_assembly_deprecated (gdbarch, uiout,
>> > +							   &di, symtab,
>> > +							   low, high,
>> > +							   how_many,
>> > +							   flags, stb,
>> > +							   &low);
>> 
>> Ditto.
>> 
>> > +	    }
>> > +
>> > +	  /* Disassemble a single instruction if we couldn't find a
>> > +	     suitable symtab, or if, for any reason, the above disassembly
>> > +	     didn't move the LOW address on at all.  */
>> 
>> I think a reasonable case can be made that this is the wrong thing to
>> do for the deprecated case (based on my observations of how it works,
>> and does not work). Consider that it doesn't print inlined function calls
>> that come from another file. If it doesn't do that, why go to any effort
>> to print non-source-based disassembly in the gaps between source
>> files.
>
> I guess I see the question the other way around; why should we
> maintain a separate way of doing things just to preserve (what I see
> as) a bug?
>
> By placing the choice between the old and new right into the middle of
> my new disassembly control loop both old and new pick up the fix.  To
> do what you suggest I see two choices (I'm open to alternative
> suggestions though):
>
>   1. Move the call to the old, deprecated, function outside of the new
>      disassembly control loop.  We'd probably end up with code
>      duplication, so extra maintenance overhead, just to support a
>      deprecated method.
>
>   2. Add a 'break;' immediately after the call to the deprecated
>      disassembly function (within the else block).  This would ensure
>      we only get once chance to disassemble.  Sure, if that's what it
>      takes to get my patch in I can do that, but it feels kind of
>      petty to me, like we're leaving a bug in place just to force
>      people to move to the new method.

As a counter proposal, let's just get rid of /m,
and make /s the new /m (I'm ok with then getting rid of /s).

>> [Over time I can imagine attempts to try to morph the deprecated
>> case into the new non-deprecated case, i.e., changing /m to be more like /s.
>> How about leaving /m the way it is, and just encourage everyone to
>> use /s.]
>
> Is that really so bad?  I'm going from memory here so I may have the
> history wrong, but isn't the story that you found the inline-functions
> issue with the /m case, looked into fixing it and discovered that
> you'd be better off writing a brand new function, hence /s.
>
> But surely, if someone put forward a patch that "fixed" /m (without
> making it exactly the same as /s) then would that really be so bad?

If someone can make a reasonable case that the intent behind /m output
is usable I could considerate it.
I'm just not in favor of making other code more complicated just
to placate /m.

Btw, there is another patch in the works for refactoring a lot of this.
https://sourceware.org/ml/gdb-patches/2015-09/msg00510.html
et.al.
Heads up.