Re: [patch] Correct `gcc -D' macros get ignored (PR 9873)

[Joel Brobecker <brobecker at adacore dot com>]

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