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

Simon Marchi
2019-04-27T16:16:27Z

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