[RFC] Apply compilation dir to source_path lookup

  On 9/13/19 6:45 PM, Andrew Burgess wrote:
> * Eli Zaretskii <eliz@gnu.org> [2019-09-13 10:28:52 +0300]:
> 
>>> Date: Fri, 13 Sep 2019 09:36:42 +0300
>>> From: Eli Zaretskii <eliz@gnu.org>
>>> CC: mgulick@mathworks.com, gdb-patches@sourceware.org
>>>
>>>>                         @value{GDBN} will also append the compilation
>>>> +directory to the filename and check this against all other entries in
>>>> +the source path.
>>>
>>> I think "append" here is a mistake.  Should it be "prepend"?  And
>>> anyway, doesn't this simply repeat what was described in the text
>>> above?
>>
>> Btw, do the "prepend" and "append", as implemented, take care to DTRT
>> with Windows drive letters at the beginning of absolute file names?  A
>> literal prepending or appending will do the wrong thing there.

You beat me to a response, but here's what I was going to say:

The only way this would be a problem is if both the compilation
directory and the source file contained a drive letter.  I had assumed
that if the debug information contained a compilation directory, then
the file path would be relative to that.  GCC at least seems to behave
this way.

[mgulick@mgulick-deb9-64:~/test/src] ...
$ gcc -g -o test.o -fdebug-prefix-map=$HOME= -c test.c
[mgulick@mgulick-deb9-64:~/test/src] ...
$ dwarfdump test.o
...
                    DW_AT_name                  test.c
                    DW_AT_comp_dir              /test/src
...

[mgulick@mgulick-deb9-64:~/test/src] ...
$ gcc -g -o test.o -fdebug-prefix-map=$HOME= -c `pwd`/test.c
[mgulick@mgulick-deb9-64:~/test/src] ...
$ dwarfdump test.o
...
                    DW_AT_name                  /test/src/test.c
...

In this case there is no DW_AT_comp_dir present.

If you are concerned about this (possibly some crazy compiler emitting
strange dwarf), the following change should suffice:


> 
> Gah!
> 
> Looking at the implementation of 'openp' (in source.c) I see this
> code:
> 
>   /* For dos paths, d:/foo -> /foo, and d:foo -> foo.  */
>   if (HAS_DRIVE_SPEC (string))
>     string = STRIP_DRIVE_SPEC (string);
> 
> which just seems to throw out the drive spec, and I can't find any
> code that adds it back in.  Is that going to do the right thing?
> 
> I did consider only creating the COMP_DIR/FILENAME combination if
> FILENAME was not absolute, but I worried that this might be
> unnecessarily restrictive, but now I'm tempted to say that would solve
> this problem, and we should just wait until someone comes up with an
> example where that is not good enough, before we figure out how to
> allow it...

This also seems fine to me.  If FILENAME is absolute, there shouldn't be
a compilation directory.

> 
> Thanks,
> Andrew
> 

Thanks,
Mike
  

> From: Mike Gulick <mgulick@mathworks.com>
> CC: Mike Gulick <mgulick@mathworks.com>, "gdb-patches@sourceware.org"
> 	<gdb-patches@sourceware.org>
> Date: Fri, 13 Sep 2019 22:52:19 +0000
> 
> The only way this would be a problem is if both the compilation
> directory and the source file contained a drive letter.

Are you sure this is the only problematic use case?

Does GDB even try to prepend any directories to a source file whose
name is absolute and includes a drive letter?  If it does, is that
TRT?

> I had assumed that if the debug information contained a compilation
> directory, then the file path would be relative to that.  GCC at
> least seems to behave this way.
> 
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ gcc -g -o test.o -fdebug-prefix-map=$HOME= -c test.c
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ dwarfdump test.o
> ...
>                     DW_AT_name                  test.c
>                     DW_AT_comp_dir              /test/src
> ...
> 
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ gcc -g -o test.o -fdebug-prefix-map=$HOME= -c `pwd`/test.c
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ dwarfdump test.o
> ...
>                     DW_AT_name                  /test/src/test.c
> ...
> 
> In this case there is no DW_AT_comp_dir present.

That's not exactly what I see on Windows (but note that I used -g3):

D:\usr\eli\data>gcc -g3 -c -o test.o d:/usr/eli/data/test.c
D:\usr\eli\data>objdump --dwarf test.o
...
    <3a>   DW_AT_name        : d:/usr/eli/data/test.c
    <51>   DW_AT_comp_dir    : D:\usr\eli\data

D:\usr\eli\data>gcc -g3 -c -o test.o -fdebug-prefix-map=D:\usr= d:/usr/eli/data/test.c
D:\usr\eli\data>objdump --dwarf test.o
...
    <3a>   DW_AT_name        : /eli/data/test.c
    <4b>   DW_AT_comp_dir    : \eli\data

> If you are concerned about this (possibly some crazy compiler emitting
> strange dwarf), the following change should suffice:

I'm not familiar with the code enough to be sure, but how do we
convince ourselves that removing one of the several drive letters and
doing nothing else is TRT in this case?  Maybe we should prepend that
drive letter to the complete file name we produce?

Thanks.
  

Mike,

Thanks for your feedback.  I incorporated your suggested changes and
pushed this to master.

Thanks,
Andrew



* Mike Gulick <mgulick@mathworks.com> [2019-09-13 22:52:19 +0000]:

> On 9/13/19 6:45 PM, Andrew Burgess wrote:
> > * Eli Zaretskii <eliz@gnu.org> [2019-09-13 10:28:52 +0300]:
> > 
> >>> Date: Fri, 13 Sep 2019 09:36:42 +0300
> >>> From: Eli Zaretskii <eliz@gnu.org>
> >>> CC: mgulick@mathworks.com, gdb-patches@sourceware.org
> >>>
> >>>>                         @value{GDBN} will also append the compilation
> >>>> +directory to the filename and check this against all other entries in
> >>>> +the source path.
> >>>
> >>> I think "append" here is a mistake.  Should it be "prepend"?  And
> >>> anyway, doesn't this simply repeat what was described in the text
> >>> above?
> >>
> >> Btw, do the "prepend" and "append", as implemented, take care to DTRT
> >> with Windows drive letters at the beginning of absolute file names?  A
> >> literal prepending or appending will do the wrong thing there.
> 
> You beat me to a response, but here's what I was going to say:
> 
> The only way this would be a problem is if both the compilation
> directory and the source file contained a drive letter.  I had assumed
> that if the debug information contained a compilation directory, then
> the file path would be relative to that.  GCC at least seems to behave
> this way.
> 
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ gcc -g -o test.o -fdebug-prefix-map=$HOME= -c test.c
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ dwarfdump test.o
> ...
>                     DW_AT_name                  test.c
>                     DW_AT_comp_dir              /test/src
> ...
> 
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ gcc -g -o test.o -fdebug-prefix-map=$HOME= -c `pwd`/test.c
> [mgulick@mgulick-deb9-64:~/test/src] ...
> $ dwarfdump test.o
> ...
>                     DW_AT_name                  /test/src/test.c
> ...
> 
> In this case there is no DW_AT_comp_dir present.
> 
> If you are concerned about this (possibly some crazy compiler emitting
> strange dwarf), the following change should suffice:
> 
> diff --git a/gdb/source.c b/gdb/source.c
> index 1635563b20..3fd05a06f2 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1049,8 +1049,12 @@ find_and_open_source (const char *filename,
>         cdir_filename.pop_back ();
>        /* Add our own directory separator.  */
>        cdir_filename.append (SLASH_STRING);
> -      /* Append filename, without any leading directory separators.  */
> +      /* Append filename, without any leading directory separators or drive
> +        names.  */
>        const char * filename_start = filename;
> +      /* For dos paths, d:/foo -> /foo, and d:foo -> foo.  */
> +      if (HAS_DRIVE_SPEC (filename_start))
> +       filename_start = STRIP_DRIVE_SPEC (filename_start);
>        while (IS_DIR_SEPARATOR (filename_start[0]))
>         filename_start++;
>        cdir_filename.append (filename_start);
> 
> > 
> > Gah!
> > 
> > Looking at the implementation of 'openp' (in source.c) I see this
> > code:
> > 
> >   /* For dos paths, d:/foo -> /foo, and d:foo -> foo.  */
> >   if (HAS_DRIVE_SPEC (string))
> >     string = STRIP_DRIVE_SPEC (string);
> > 
> > which just seems to throw out the drive spec, and I can't find any
> > code that adds it back in.  Is that going to do the right thing?
> > 
> > I did consider only creating the COMP_DIR/FILENAME combination if
> > FILENAME was not absolute, but I worried that this might be
> > unnecessarily restrictive, but now I'm tempted to say that would solve
> > this problem, and we should just wait until someone comes up with an
> > example where that is not good enough, before we figure out how to
> > allow it...
> 
> This also seems fine to me.  If FILENAME is absolute, there shouldn't be
> a compilation directory.
> 
> > 
> > Thanks,
> > Andrew
> > 
> 
> Thanks,
> Mike
>
  

On 9/17/19 4:22 PM, Andrew Burgess wrote:
> Mike,
> 
> Thanks for your feedback.  I incorporated your suggested changes and
> pushed this to master.
> 
> Thanks,
> Andrew
> 

Looks great.  Thanks so much for your help Andrew.

-Mike