Re: Fix lookup of separate debug file on MS-Windows

[Simon Marchi <simark@simark.ca>]

On 2019-04-22 5:19 a.m., Eli Zaretskii wrote:
>> From: Simon Marchi <simark@simark.ca>
>> Date: Sun, 21 Apr 2019 08:55:07 -0400
>>
>> On 2019-04-21 8:05 a.m., Eli Zaretskii wrote:
>>> Ping!
>>>
>>> Would people please voice their opinions regarding preservation of the
>>> drive letter (and removing the colon) vs just dropping the drive
>>> letter altogether?  I'd like to fix this issue for the upcoming
>>> release of GDB 8.3.
>>
>> I first read your patch (which initiated this thread), and my first reaction was
>> also to be surprised that we would lose the drive letter.  The risk of clash
>> is low, but why take that risk at all when it's easy enough to keep the drive letter
>> and just strip the colon?
> 
> OK, so how about the patch below?
> 
>> I don't know the complete history of this, so if you know about distributions that
>> place debug files in a path without the drive letter (what your patch implements),
>> then I would be fine if GDB looked in both places, starting with the path including
>> the drive letter.
> 
> I'm not aware of any Windows distributions that put debug info in the
> directory suggested by my previous patch.  So I think we can safely
> use just the /X/ subdirectory method.
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index a3a5f3e..533a52c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -19917,7 +19917,11 @@
>  the directory of the executable file, then in a subdirectory of that
>  directory named @file{.debug}, and finally under each one of the global debug
>  directories, in a subdirectory whose name is identical to the leading
> -directories of the executable's absolute file name.
> +directories of the executable's absolute file name.  (On MS-Windows,
> +the drive letter of the executable's leading directories is converted
> +to a one-letter subdirectory, i.e.@: @file{d:/usr/bin/} is converted
> +to @file{/d/usr/bin/}, because Windows filesystems disallow colons in
> +file names.)
>  
>  @item
>  For the ``build ID'' method, @value{GDBN} looks in the
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 5736666..5749c61 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1443,11 +1443,24 @@ find_separate_debug_file (const char *dir,
>      = dirnames_to_char_ptr_vec (debug_file_directory);
>    gdb::unique_xmalloc_ptr<char> canon_sysroot = gdb_realpath (gdb_sysroot);
>  
> + /* MS-Windows/MS-DOS don't allow colons in file names; we must
> +    convert the drive letter into a one-letter directory, so that the
> +    file name resulting from splicing below will be valid.  */
> +  std::string drive;
> +  if (HAS_DRIVE_SPEC (dir_notarget))

I think we should consider the case where we build GDB on GNU/Linux, to remotely
debug a Windows program.  When building on GNU/Linux, HAS_DRIVE_SPEC always return false,
since it's defined as (see include/filenames.h):

  #define HAS_DRIVE_SPEC(f) (0)

Let's suppose DEBUGDIR is "D:/my/debug", DIR is "target:E:/the/directory/" and DEBUGLINK
is "program.debug".  On GNU/Linux, we would build the path

  target:D:/my/debug/E:/the/directory/program.debug

And I suppose that the "E:" would result in the debug file not being found.

So I think we should be using HAS_DRIVE_SPEC_1, which allows us to do the same check.  We
just need to pass to the macro whether the target filesystem id DOS-based.  The only problem
is, how do we know whether the target filesystem is DOS-based?  We wouldn't want
HAS_DRIVE_SPEC_1 to do this stripping erroneously when debugging natively on GNU/Linux...

If we can't find a practical solution to this, then I would say it's better to keep using
HAS_DRIVE_SPEC, and it will be a known deficiency for the moment that it is wrong when debugging
remotely from GNU/Linux to Windows.  It wouldn't be a regression, as it is already wrong today).
But you will have fixed it for the native Windows case, which is a net gain.

> +    {
> +      drive = std::string (1, dir_notarget[0]);

This should be equivalent to:

    drive = dir_notarget[0];

> +      dir_notarget = STRIP_DRIVE_SPEC (dir_notarget);
> +    }
> +  else
> +    drive = "";

The else clause is unnecessary, as drive is already the empty string;

So you could reduce it to:

  std::string drive;
  if (HAS_DRIVE_SPEC (dir_notarget))
    {
      drive = dir_notarget[0];
      dir_notarget = STRIP_DRIVE_SPEC(dir_notarget);
    }

Simon