[RFA/commit] Improve gdb_realpath for Windows hosts

[RFA/commit] Improve gdb_realpath for Windows hosts

[Joel Brobecker]

Hello,

I noticed that inserting a breakpoint on Windows hosts
often does not work when the linespec involves a filename
with its full path. For instance:

    (gdb) b c:/some/double/slashes/dir/foo.c:4
    No source file named c:/some/double/slashes/dir/foo.c:4.
    (gdb) b c:\some\double\slashes\dir\foo.c:4
    No source file named c:\some\double\slashes\dir\foo.c:4.

The problem in this case comes from the fact that the compiler
produced some debugging info where the filename had double
backslaces (weird), as in `c:\\some\\double\\slashes\\dir'.

On Unix platforms, this wouldn't be a problem, because gdb_realpath
takes care of normalizing the path.  But on windows, the implementation
is just an xstrdup.

This fixes the problem by enhancing gdb_realpath on Windows hosts.
The code is inspired from libiberty's lrealpath. I decided to
continue duplicating part of lrealpath rather than replacing
everything by a call to lrealpath because I think that we actually
want something slightly different: after normalizing the path,
lrealpath then lowercases it. This is not idea of GDB, because
it prevents us from displaying filenames using the correct casing.

gdb/ChangeLog:

        * utils.c (gdb_realpath): Add better support for Windows hosts.

Tested on x86-windows. Any objection to this? There is a performance
cost, but this is just on par with what we do on Unix hosts, so
I assume it's OK.

Thanks,
-- 
Joel

---
 gdb/utils.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 16405d1..d55e6f1 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3341,6 +3341,25 @@ gdb_realpath (const char *filename)
   }
 #endif
 
+  /* The MS Windows method.  If we don't have realpath, we assume we
+     don't have symlinks and just canonicalize to a Windows absolute
+     path.  GetFullPath converts ../ and ./ in relative paths to
+     absolute paths, filling in current drive if one is not given
+     or using the current directory of a specified drive (eg, "E:foo").
+     It also converts all forward slashes to back slashes.  */
+  /* The file system is case-insensitive but case-preserving.
+     So we do not lowercase the path.  Otherwise, we might not
+     be able to display the original casing in a given path.  */
+#if defined (_WIN32)
+  {
+    char buf[MAX_PATH];
+    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
+
+    if (len > 0 && len < MAX_PATH)
+      return xstrdup (buf);
+  }
+#endif
+
   /* This system is a lost cause, just dup the buffer.  */
   return xstrdup (filename);
 }
-- 
1.7.1

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[Eli Zaretskii]

> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Joel Brobecker <brobecker@adacore.com>
> Date: Thu, 22 Dec 2011 21:14:44 +0400
> 
> gdb/ChangeLog:
> 
>         * utils.c (gdb_realpath): Add better support for Windows hosts.
> 
> Tested on x86-windows. Any objection to this?

Looks good to me.  Although there's a subtle semantic difference
between `realpath' on Posix platforms and GetFullPathName: the former
requires the file to exist, while the latter does not.  But I reckon
we handle this somewhere...

Did you test this with UNC file names?

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         * utils.c (gdb_realpath): Add better support for Windows hosts.
> > 
> > Tested on x86-windows. Any objection to this?
> 
> Looks good to me.  Although there's a subtle semantic difference
> between `realpath' on Posix platforms and GetFullPathName: the former
> requires the file to exist, while the latter does not.  But I reckon
> we handle this somewhere...

Thanks for taking a look!

I was not aware of its behavior when the file does not exist.
It's strange, because MSN says:

    This function does not verify that the resulting path and file
    name are valid, or that they see an existing file on the associated
    volume.

It's ambiguous, and you're probably right. But it explains
my surprise.

That being said, it's treated as a normalization error, and
we fallback to xstrdup in that case, hoping that the user then
uses the a filename that matches according to filename_cmp
(it deals with casing and directory separators, but not the
case of consecutive directory separators). In other words,
if the file does not exist, we're back to where we were before
this patch.

> Did you test this with UNC file names?

I did not test with UNC file names, no. Mostly because it never
entered my mind. I'm not a Windows person I'm afraid, never used it
unless asked to fix a bug that reproduces on this system only. And
I don't know if this is something I could do with our machines at
work.

That being said, MSDN does confirm that it should work (I think!):

    Share and volume names are valid input for lpFileName. For example,
    the following list identities the returned path and file names if
    test-2 is a remote computer [...]:

        # If you specify "\\test-2\q$\lh" the path returned is
          "\\test-2\q$\lh"

        # If you specify "\\?\UNC\test-2\q$\lh" the path returned is
          "\\?\UNC\test-2\q$\lh"

-- 
Joel

Checked in: [RFA/commit] Improve gdb_realpath for Windows hosts

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * utils.c (gdb_realpath): Add better support for Windows hosts.

Checked in (HEAD only).

-- 
Joel

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[asmwarrior]

On 2011-12-23 1:14, Joel Brobecker wrote:
> Hello,
>
> I noticed that inserting a breakpoint on Windows hosts
> often does not work when the linespec involves a filename
> with its full path. For instance:
>
>      (gdb) b c:/some/double/slashes/dir/foo.c:4
>      No source file named c:/some/double/slashes/dir/foo.c:4.
>      (gdb) b c:\some\double\slashes\dir\foo.c:4
>      No source file named c:\some\double\slashes\dir\foo.c:4.
>
> The problem in this case comes from the fact that the compiler
> produced some debugging info where the filename had double
> backslaces (weird), as in `c:\\some\\double\\slashes\\dir'.
>
> On Unix platforms, this wouldn't be a problem, because gdb_realpath
> takes care of normalizing the path.  But on windows, the implementation
> is just an xstrdup.
>
> This fixes the problem by enhancing gdb_realpath on Windows hosts.
> The code is inspired from libiberty's lrealpath. I decided to
> continue duplicating part of lrealpath rather than replacing
> everything by a call to lrealpath because I think that we actually
> want something slightly different: after normalizing the path,
> lrealpath then lowercases it. This is not idea of GDB, because
> it prevents us from displaying filenames using the correct casing.
>
> gdb/ChangeLog:
>
>          * utils.c (gdb_realpath): Add better support for Windows hosts.
>
> Tested on x86-windows. Any objection to this? There is a performance
> cost, but this is just on par with what we do on Unix hosts, so
> I assume it's OK.
>
> Thanks,

Someone said that "MAX_PATH" is not enough about several months ago when we were discussing some breakpoint issue
see:
http://sourceware.org/ml/gdb/2011-06/msg00101.html

asmwarrior

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[Joel Brobecker]

> Someone said that "MAX_PATH" is not enough about several months ago
> when we were discussing some breakpoint issue see:
> http://sourceware.org/ml/gdb/2011-06/msg00101.html

My understanding is:
  - The way this function is called, its output is limited to MAX_PATH;
  - If the canonialized path does in fact exceed MAX_PATH, then
    the canonicalization fails, and we fallback on the old way
    (strdup).

So I do not see the problem with the current approach. There might
be a better implementation, allowing longer paths, but this is
already better than before. And if there is a better implementation
indeed, it does need to work on all supported versions of Windows
(I think we need to support at least as back as XP, possibly older).

-- 
Joel

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[asmwarrior]

On 2011-12-27 20:40, Joel Brobecker wrote:
>> Someone said that "MAX_PATH" is not enough about several months ago
>> >  when we were discussing some breakpoint issue see:
>> >  http://sourceware.org/ml/gdb/2011-06/msg00101.html
> My understanding is:
>    - The way this function is called, its output is limited to MAX_PATH;
>    - If the canonialized path does in fact exceed MAX_PATH, then
>      the canonicalization fails, and we fallback on the old way
>      (strdup).
>
> So I do not see the problem with the current approach. There might
> be a better implementation, allowing longer paths, but this is
> already better than before. And if there is a better implementation
> indeed, it does need to work on all supported versions of Windows
> (I think we need to support at least as back as XP, possibly older).
>
> -- Joel
Yeah, I agree with you. In fact, I use such patch locally in my XP for months without any problems.:-)

asmwarrior
ollydbg from codeblocks' forum

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[asmwarrior]

Can you explain that why you avoid converting to lowercase?

>   /* The file system is case-insensitive but case-preserving.
>      So we do not lowercase the path.  Otherwise, we might not
>      be able to display the original casing in a given path.  */

I just thought that lowercase is another canonization of path, so you can have a uniform path when you set breakpoints(file specification)

The original code in iberty library is like:

>   /* The MS Windows method.  If we don't have realpath, we assume we
>      don't have symlinks and just canonicalize to a Windows absolute
>      path.  GetFullPath converts ../ and ./ in relative paths to
>      absolute paths, filling in current drive if one is not given
>      or using the current directory of a specified drive (eg, "E:foo").
>      It also converts all forward slashes to back slashes.  */
> #if defined (_WIN32)
>   {
>     char buf[MAX_PATH];
>     char* basename;
>     DWORD len = GetFullPathName (filename, MAX_PATH, buf, &basename);
>     if (len == 0 || len > MAX_PATH - 1)
>       return xstrdup (filename);
>     else
>       {
>     /* The file system is case-preserving but case-insensitive,
>        Canonicalize to lowercase, using the codepage associated
>        with the process locale.  */
>         CharLowerBuff (buf, len);
>         return xstrdup (buf);
>       }
>   }
> #endif

Thanks.

asmwarrior
ollydbg from codeblocks' forum

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[Joel Brobecker]

> Can you explain that why you avoid converting to lowercase?
> 
> >  /* The file system is case-insensitive but case-preserving.
> >     So we do not lowercase the path.  Otherwise, we might not
> >     be able to display the original casing in a given path.  */
> 
> I just thought that lowercase is another canonization of path, so you
> can have a uniform path when you set breakpoints(file specification)

Two reasons, mostly:
  . It is unnecessary;
  . It changes the filename casing when displaying the name of
    the file where the breakpoint has been inserted.

Several Windows users at AdaCore often complain that GNU tools do not
properly preserve the filename casing, so I did not think it was
proper for us to do so. Even as a Unix user, I do feel that it is
important to preserve the casing as well.

I should say that it does not affect filename matching, which is
performed through another specialized function.

> The original code in iberty library is like:

Honestly, I do not understand why they do it. There is no correlation,
IMO, between the fact that the FS is case-insensitive and the fact
that you'd want to change the filename casing (again, doing something
extra which is unnecessary, since it does not make a difference to
the filesystem).

-- 
Joel

Re: [RFA/commit] Improve gdb_realpath for Windows hosts

[asmwarrior]

On 2011-12-27 22:12, Joel Brobecker wrote:
>> Can you explain that why you avoid converting to lowercase?
>>
>>>   /* The file system is case-insensitive but case-preserving.
>>>      So we do not lowercase the path.  Otherwise, we might not
>>>      be able to display the original casing in a given path.  */
>> I just thought that lowercase is another canonization of path, so you
>> can have a uniform path when you set breakpoints(file specification)
> Two reasons, mostly:
>    . It is unnecessary;
>    . It changes the filename casing when displaying the name of
>      the file where the breakpoint has been inserted.
>
> Several Windows users at AdaCore often complain that GNU tools do not
> properly preserve the filename casing, so I did not think it was
> proper for us to do so. Even as a Unix user, I do feel that it is
> important to preserve the casing as well.
>
> I should say that it does not affect filename matching, which is
> performed through another specialized function.
>
>> The original code in iberty library is like:
> Honestly, I do not understand why they do it. There is no correlation,
> IMO, between the fact that the FS is case-insensitive and the fact
> that you'd want to change the filename casing (again, doing something
> extra which is unnecessary, since it does not make a difference to
> the filesystem).
>
OK, I understand now. Thank you Joel for your explanation and your time.

asmwarrior
ollydbg from codeblocks' forum