Re: [RFA]: Fix for do_mixed_source_and_assembly in disasm.c

["J. Johnston" <jjohnstn@redhat.com>]

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

Andrew Cagney wrote:
>>> Jason Molenda wrote:
>>> Hi Jeff,
>>>
>>> On Dec 17, 2003, at 1:15 PM, Jeff Johnston wrote:
>>>
>>> There are a few bugs in do_mixed_source_and_assembly() when dealing 
>>> with the ia64.
>>>
>>>
>>> I hit one of the two bugs you're fixing last week, namely the double 
>>> call to close off the list/tuple.  My first fix looked like yours, 
>>> but I think a slight reworking of the loop is very desirable here.  
>>> Right now this loop looks approximately like this:
>>>
>>>
>>>    Set close_list to 1 (the list/tuple will be closed at the end of 
>>> this loop)
>>>
>>>    First entry of a new source line number:
>>>      Start a "src_and_asm_line" tuple, print the source line.
>>>      Start a "line_asm_insn" list where instructions will be emitted.
>>>      If we're not at the end of the mle array, and the next mle 
>>> entry's source line number is not greater than the current source 
>>> line entry, DON'T close off the list (close_list = 0)
>>>
>>>    Print the assembly instructions for the current address range.
>>>
>>>    If close_list is 1, close the tuple/list.
>>>
>>>
>>> Which is a very convoluted way of writing a loop, and more 
>>> importantly, this only works correctly for one or two assembly ranges 
>>> for a single source line.  If you have a third, on the 2nd iter the 
>>> tuple/list are closed and then on the 3rd you close them again.
>>>
>>> Instead, this is more clear:
>>>
>>>
>>>   First entry of a new source line number:
>>>      Start a "src_and_asm_line" tuple, print the source line.
>>>      Start a "line_asm_insn" list where instructions will be emitted.
>>>
>>>   Print the assembly instructions for the current address range.
>>>
>>>   If we're at the end of the mle array or the next entry in the array 
>>> is a new source line, close off the list and tuple.
>>>
>>>
>>> I also took the opportunity to combine two conditional statements - I 
>>> won't push hard for that part of the change, but the rest is a clear 
>>> improvement IMHO.
>>>
>>> We only had this code path executed when we were using a compiler 
>>> with a bug in it internally, so it's not too easy for me to 
>>> reproduce/test this.  I've tried to combine my patch and yours 
>>> against the current FSF TOT.  What do you think of my suggested 
>>> additional change?  Can you try it on the IA64 test case you have?
>>>
>>>
>>> Jason
>>>
> 
>>
>> Works fine.  I like the revision you made.  I don't know if the 
>> powers-that-be will review this patch before the New Year. 
> 
> 
> Since it works for you Jeff, yes, why not (just a shame that the powers 
> that didn't take a holiday didn't think to review it).
> 
> Andrew
> 
>

Thanks Andrew.  I have checked in the accompanying modified patch.  The 
modification was simply to set the initial values of the cleanups to be
null_cleanup rather than NULL per the discussion we had on the other thread.  As 
well, I updated the copyright year.

-- Jeff J.

> 

[-- Attachment #2: disasm.patch --]
[-- Type: text/plain, Size: 2320 bytes --]

Index: disasm.c
===================================================================
RCS file: /cvs/src/src/gdb/disasm.c,v
retrieving revision 1.17
diff -u -p -r1.17 disasm.c
--- disasm.c	24 Oct 2003 17:37:03 -0000	1.17
+++ disasm.c	6 Jan 2004 19:05:23 -0000
@@ -1,6 +1,6 @@
 /* Disassemble support for GDB.
 
-   Copyright 2000, 2001, 2002, 2003 Free Software Foundation, Inc.
+   Copyright 2000, 2001, 2002, 2003, 2004 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -164,6 +164,8 @@ do_mixed_source_and_assembly (struct ui_
   CORE_ADDR pc;
   int num_displayed = 0;
   struct cleanup *ui_out_chain;
+  struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
+  struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
 
   mle = (struct dis_line_entry *) alloca (nlines
 					  * sizeof (struct dis_line_entry));
@@ -221,10 +223,6 @@ do_mixed_source_and_assembly (struct ui_
 
   for (i = 0; i < newlines; i++)
     {
-      struct cleanup *ui_out_tuple_chain = NULL;
-      struct cleanup *ui_out_list_chain = NULL;
-      int close_list = 1;
-      
       /* Print out everything from next_line to the current line.  */
       if (mle[i].line >= next_line)
 	{
@@ -275,23 +273,23 @@ do_mixed_source_and_assembly (struct ui_
 	  next_line = mle[i].line + 1;
 	  ui_out_list_chain
 	    = make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
-	  /* Don't close the list if the lines are not in order. */
-	  if (i < (newlines - 1) && mle[i + 1].line <= mle[i].line)
-	    close_list = 0;
 	}
 
       num_displayed += dump_insns (uiout, di, mle[i].start_pc, mle[i].end_pc,
 				   how_many, stb);
-      if (close_list)
+
+      /* 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.  */
+      if (i == (newlines - 1) || mle[i + 1].line > mle[i].line)
 	{
 	  do_cleanups (ui_out_list_chain);
 	  do_cleanups (ui_out_tuple_chain);
+	  ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
+	  ui_out_list_chain = make_cleanup (null_cleanup, 0);
 	  ui_out_text (uiout, "\n");
-	  close_list = 0;
 	}
-      if (how_many >= 0)
-	if (num_displayed >= how_many)
-	  break;
+      if (how_many >= 0 && num_displayed >= how_many)
+	break;
     }
   do_cleanups (ui_out_chain);
 }