[RFA]: Fix for do_mixed_source_and_assembly in disasm.c

[RFA]: Fix for do_mixed_source_and_assembly in disasm.c

[Jeff Johnston]

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

There are a few bugs in do_mixed_source_and_assembly() when dealing with 
the ia64.  One problem is that cleanups  for tuples and lists can 
possibly be deferred to a future iteration of a loop, however, the 
values are reinitialized to NULL each time at the start of the loop.  
Another problem is that the code to figure out if the list/tuple should 
be closed off is inside a block of code that is not always reached in 
every iteration.  These two problems combined to cause a SIGSEGV in gdb 
because a NULL pointer gets passed into do_cleanups() which causes all 
cleanups to be performed up the chain.  I have submitted a separate 
patch to prevent running the entire chain when NULL input is passed.

Ok to commit?

-- Jeff J.

2003-12-17  Jeff Johnston  <jjohnstn@redhat.com>

        * disasm.c (do_mixed_source_and_assembly): For uiout asm list
        and tuple cleanups, don't reset to NULL until we close off the
        tuple/list.  Also move check for whether to close off the
        asm tuple/list to where it will be run on each iteration of the
        loop.



[-- Attachment #2: disasm.patch --]
[-- Type: text/plain, Size: 1760 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	17 Dec 2003 20:37:25 -0000
@@ -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 = NULL;
+  struct cleanup *ui_out_list_chain = NULL;
 
   mle = (struct dis_line_entry *) alloca (nlines
 					  * sizeof (struct dis_line_entry));
@@ -221,8 +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.  */
@@ -275,19 +275,21 @@ 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;
 	}
 
+      /* 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)
 	{
 	  do_cleanups (ui_out_list_chain);
 	  do_cleanups (ui_out_tuple_chain);
+	  ui_out_tuple_chain = NULL;
+	  ui_out_list_chain = NULL;
 	  ui_out_text (uiout, "\n");
-	  close_list = 0;
 	}
       if (how_many >= 0)
 	if (num_displayed >= how_many)

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

[Jason Molenda]

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

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


[-- Attachment #2: pa.txt --]
[-- Type: text/plain, Size: 1992 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	18 Dec 2003 22:05:20 -0000
@@ -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 = NULL;
+  struct cleanup *ui_out_list_chain = NULL;
 
   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,22 +273,22 @@ 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 = NULL;
+          ui_out_list_chain = NULL;
 	  ui_out_text (uiout, "\n");
-	  close_list = 0;
 	}
-      if (how_many >= 0)
-	if (num_displayed >= how_many)
+      if (how_many >= 0 && num_displayed >= how_many)
 	  break;
     }
   do_cleanups (ui_out_chain);

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

[J. Johnston]

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.

-- Jeff J.

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

[Andrew Cagney]

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

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

[J. Johnston]

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