From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17564 invoked by alias); 6 Jan 2004 19:11:44 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 17466 invoked from network); 6 Jan 2004 19:11:41 -0000 Received: from unknown (HELO touchme.toronto.redhat.com) (216.129.200.20) by sources.redhat.com with SMTP; 6 Jan 2004 19:11:41 -0000 Received: from redhat.com (toocool.toronto.redhat.com [172.16.14.72]) by touchme.toronto.redhat.com (Postfix) with ESMTP id 856A28001BD; Tue, 6 Jan 2004 14:11:41 -0500 (EST) Message-ID: <3FFB086D.9020307@redhat.com> Date: Tue, 06 Jan 2004 19:11:00 -0000 From: "J. Johnston" Organization: Red Hat Inc. User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 MIME-Version: 1.0 To: Andrew Cagney Cc: Jason Molenda , gdb-patches@sources.redhat.com Subject: Re: [RFA]: Fix for do_mixed_source_and_assembly in disasm.c References: <3FE0C778.8010606@redhat.com> <4C0916E8-31A7-11D8-BFBD-000393D457E2@apple.com> <3FE23319.9000009@redhat.com> <3FF598BA.1040504@gnu.org> In-Reply-To: <3FF598BA.1040504@gnu.org> Content-Type: multipart/mixed; boundary="------------000305030508070104070204" X-SW-Source: 2004-01/txt/msg00147.txt.bz2 This is a multi-part message in MIME format. --------------000305030508070104070204 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2957 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. > --------------000305030508070104070204 Content-Type: text/plain; name="disasm.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="disasm.patch" Content-length: 2320 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); } --------------000305030508070104070204--