From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15982 invoked by alias); 3 Mar 2009 00:54:44 -0000 Received: (qmail 15971 invoked by uid 22791); 3 Mar 2009 00:54:41 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 Mar 2009 00:54:35 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 6A9452BAB8F; Mon, 2 Mar 2009 19:54:36 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id PCzqfShzK6PL; Mon, 2 Mar 2009 19:54:36 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 0C6592BAB86; Mon, 2 Mar 2009 19:54:36 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id CD310E7ACD; Mon, 2 Mar 2009 16:54:30 -0800 (PST) Date: Tue, 03 Mar 2009 00:54:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [patch] Correct `gcc -D' macros get ignored (PR 9873) Message-ID: <20090303005430.GL26056@adacore.com> References: <20090219214540.GA32244@host0.dyn.jankratochvil.net> <20090228071514.GA25299@adacore.com> <20090228100225.GA15788@host0.dyn.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090228100225.GA15788@host0.dyn.jankratochvil.net> User-Agent: Mutt/1.4.2.2i Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-03/txt/msg00026.txt.bz2 > * Just some text like "command-line" would be hiding a DWARF-contained > information from the user - each macro command-line macro is still > associated with a compilation unit (CU). In this context I find the Base > Source (see below) file name as the appropriate identifier of this CU. Sounds like a good argument in favor of keeping the name of the base file in that macro data, indeed. As a result, I also agree with you that introducing a special file for command-line macro definitions is excessive. Regarding whether or not we should print line "0" or "command-line", in the absence of further comments from other contributors, I suggest we follow your approach of keeping this as is - but with some extra documentation just to make it clear what the zero means. The documentation patch can be proposed separately from this patch, since it would document the existing behavior. > Partially I think this workaround may be more appropriate as > distribution/vendor specific one so it may be dropped if you think so. I don't think we should change which compiler is being used, particulary if we can't determine whether the compiler has the problem or not... Usually, what we do is add setup_xfail's. Perhaps other maintainers with more experience will have a different opinion? Now, back to your original patch. I don't like the code semi-duplication, but I don't see any better way of doing this either, so... | + /* Flag is in use by the second pass and determines if GDB is still before | + first DW_MACINFO_start_file. If true GDB is still reading the definitions | + from command line. First DW_MACINFO_start_file will need to be ignored as | + it was already executed to create CURRENT_FILE for the main source holding | + also the command line definitions. On first met DW_MACINFO_start_file | + this flag is reset to normally execute all the remaining | + DW_MACINFO_start_file macinfos. */ | + int at_commandline; I suggest placing this comment a little later, where we actually start using it (just at the start of the second pass). We can leave this flag here, of course. | + /* Start the first pass to find ahead the main source file name. GDB has to | + create CURRENT_FILE where to place the macros given to the compiler | + from the command line. Such command line macros are present before first | + DW_MACINFO_start_file but still those macros are associated to the | + compilation unit. The compilation unit GDB identifies by its main source | + file name. */ I suggest a different wording. That's just a suggestion, trying to follow the style that I am used to seeing in the GDB sources, not gospel. Use it if you think it helps: /* First pass: Find the name of the base filename. This filename is needed in order to process all macros whose definition (or undefinition) comes from the command line. These macros are defined before the first DW_MACINFO_start_file entry, and yet still need to be associated to base file. To determine the base file name, we scan the macro definitions until we reach the first DW_MACINFO_start_file entry. We then initialize CURRENT_FILE accordingly so that any macro definition found before the first DW_MACINFO_start_file can still be associated to the base file. */ | + /* Here is the second pass to read in the macros starting from the ones | + defined at the command line. */ Another suggestion, that refers to the suggestion I was making about the comment for variable at_command_line.... /* Second pass: Process all entries. Use the AT_COMMAND_LINE flag to determine whether we are still processing command-line macro definitions/undefinitions. This flag is unset when we reach the first DW_MACINFO_start_file entry. */ | + do | + { | + /* Do we at least have room for a macinfo type byte? */ | + if (mac_ptr >= mac_end) | + { | + /* Complaint is in the first pass above. */ | + break; | + } Unfortunately, I'm not sure about the fact that the complaint has already been logged during the first pass. The first pass stops the scanning as soon as we have found our base file. I suggest you move the complaint back here, and then replace the complaint in the first pass by a comment explaining that a complaint will be logged during the second pass. | + if (at_commandline != (line == 0)) This is a little too smart for my aging brain :-). I'd rather have a clearer expression: if (line == 0 && !at_commandline) | + if (at_commandline != (line == 0)) Same here. | +# Workaround ccache making lineno non-zero for command-line definitions. | +if {[find_gcc] == "gcc" && [file executable "/usr/bin/gcc"]} { | + set result [catch "exec which gcc" output] | + if {$result == 0 && [string first "/ccache/" $output] >= -1} { | + lappend options "compiler=/usr/bin/gcc" | + } | +} For now, my vote is to hold off on this change, and let the user change his path if ccache is not to be used. | gdb_expect { | - -re "Defined at \[^\r\n\]*(${filepat}):${decimal}\[\r\n\]" { | + -re "Defined at \[^\r\n\]*(${filepat}):(${decimal})\[\r\n\]" { | # `location' and `definition' should be empty when we see | # this message. | if {[llength $location] == 0 && [llength $definition] == 0} { | set location $expect_out(1,string) | + # Definitions from gcc command-line get suffixed by the lineno. | + if {$expect_out(2,string) == "0" } { | + set location "$location:$expect_out(2,string)" | + } Can you update the function documentation, please? Something like: If a macro was defined at the command-line level, ":0" will be appended at the end of the filename where this macro is defined. You may dump the comment that you added here, if you'd like. -- Joel