Message ID | 835zr68kiw.fsf@gnu.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 25531 invoked by alias); 22 Apr 2019 09:19:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 25298 invoked by uid 89); 22 Apr 2019 09:19:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-15.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.1 spammy=aware, U*simark, symfile.c, dir_notarget X-HELO: eggs.gnu.org Received: from eggs.gnu.org (HELO eggs.gnu.org) (209.51.188.92) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 22 Apr 2019 09:19:32 +0000 Received: from fencepost.gnu.org ([2001:470:142:3::e]:37744) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <eliz@gnu.org>) id 1hIV6o-0007uK-Js; Mon, 22 Apr 2019 05:19:30 -0400 Received: from [176.228.60.248] (port=4402 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from <eliz@gnu.org>) id 1hIV6o-0002oZ-2U; Mon, 22 Apr 2019 05:19:30 -0400 Date: Mon, 22 Apr 2019 12:19:19 +0300 Message-Id: <835zr68kiw.fsf@gnu.org> From: Eli Zaretskii <eliz@gnu.org> To: Simon Marchi <simark@simark.ca> CC: gdb-patches@sourceware.org In-reply-to: <2697b965-e108-5e7c-75d3-9baa7493141c@simark.ca> (message from Simon Marchi on Sun, 21 Apr 2019 08:55:07 -0400) Subject: Re: Fix lookup of separate debug file on MS-Windows References: <83ef5ze84y.fsf@gnu.org> <03da9895-5136-1da6-8c37-c4be0d06b608@gmail.com> <8336mfdumf.fsf@gnu.org> <831s1va7h8.fsf@gnu.org> <2697b965-e108-5e7c-75d3-9baa7493141c@simark.ca> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-IsSubscribed: yes |
Commit Message
Eli Zaretskii
April 22, 2019, 9:19 a.m. UTC
> 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.
Comments
Ping! OK to commit the below to both branches? > Date: Mon, 22 Apr 2019 12:19:19 +0300 > From: Eli Zaretskii <eliz@gnu.org> > CC: gdb-patches@sourceware.org > > > 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)) > + { > + drive = std::string (1, dir_notarget[0]); > + dir_notarget = STRIP_DRIVE_SPEC (dir_notarget); > + } > + else > + drive = ""; > + > for (const gdb::unique_xmalloc_ptr<char> &debugdir : debugdir_vec) > { > debugfile = target_prefix ? "target:" : ""; > debugfile += debugdir.get (); > debugfile += "/"; > + debugfile += drive; > debugfile += dir_notarget; > debugfile += debuglink; > >
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
> Cc: gdb-patches@sourceware.org > From: Simon Marchi <simark@simark.ca> > Date: Sat, 27 Apr 2019 12:16:23 -0400 > > 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. When debugging remotely, is the debug info on a Windows or on a GNU/Linux filesystem? If the latter, the above will work. I always thought that in remote debugging, GDB itself runs on the local host, i.e. on GNU/Linux in this case, and the part that runs on the remote is gdbserver. Isn't that correct? > 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... But if we do that, how do we distinguish between the use case you describe above and a use case where the we debug locally and the file names just happen to include colons? Are we willing to restrict file names on GNU/Linux to support the remote debugging on Windows? Thanks for the other comments, I will implement them when we agree about the above issue.
On 2019-04-27 12:39, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Simon Marchi <simark@simark.ca> >> Date: Sat, 27 Apr 2019 12:16:23 -0400 >> >> 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. > > When debugging remotely, is the debug info on a Windows or on a > GNU/Linux filesystem? If the latter, the above will work. I always > thought that in remote debugging, GDB itself runs on the local host, > i.e. on GNU/Linux in this case, and the part that runs on the remote > is gdbserver. Isn't that correct? The latter part is correct, gdbserver would run on the Windows machine, while GDB would run on the local GNU/Linux machine. But the debug info can come from either places, depending on the setup. If the executable read by GDB comes from the "target:" filesystem, we will search for the separate debug files on the target: filesystem (hence all this complexity with target_prefix in this function). If you are debugging remotely, the target: filesystem represents the remote filesystem, which GDB can read through requests serviced by gdbserver. If you have a local copy of the remote filesystem (a sysroot) and made "set sysroot" point to it, then GDB will read from there. If you are debugging locally on Windows, you want the path to end up like: target:D:/my/debug/E/the/directory/program.debug If you are debugging remotely your Windows from another Windows, using "set sysroot target:" (the default), you also want the path to end up like target:D:/my/debug/E/the/directory/program.debug If you are debugging remotely from a Windows machine to another Windows machine, but installed a sysroot of the debugged machine in F:/sysroot, then I guess you'll want the path to end up like F:/sysroot/D/my/debug/E/the/directory/program.debug So maybe we would want to apply the same treatment to the sysroot drive letter? If you are debugging remotely your Windows from GNU/Linux, using "set sysroot target:", we would want the path to end up like target:D:/my/debug/E/the/directory/program.debug Since the target filesystem wouldn't support a "E:" directory. If you are debugging remotely your Windows from GNU/Linux, but installed a sysroot of the debugged machine in /sysroot, then either of these could work: /sysroot/D:/my/debug/E:/the/directory/program.debug /sysroot/D:/my/debug/E/the/directory/program.debug /sysroot/D/my/debug/E:/the/directory/program.debug /sysroot/D/my/debug/E/the/directory/program.debug This leads me to say that for simplicity, when we convert a Windows drive letter into a directory component, we should always convert it to the letter without the colon. This way, whatever platform GDB itself runs on, the searched path will be the same and predictable. Also, if you were to copy D:/my/debug over to your GNU/Linux machine, you wouldn't have to rename the "E" directory to "E:". ? I would suggest the first one, If you are debugging remotely from GNU/Linux and installed a sysroot of your Windows machine in /sysroot, then I suppose you would want the path to end up like: /sysroot/D:/my/debug/E/the/directory/program.debug ? Or /sysroot/D/my/debug/E/the/directory/program.debug ? In this last example, the file is lookup up on the GNU/Linux filesystem, so the path technically could include the E:. However, since the So if you are using a local sysroot of your In the first case since you are reading from the GNU/Linux filesystem, so colons would be supported. But in the latter case, they wouldn't. One use case to consider is: you start debugging your Windows machine from your GNU/Linux machine, we do strip the colon, such that the separate debug info is searched at: target:D:/my/debug/E/the/directory/program.debug and it is found. However, it is quite slow, because everytime you start debugging, GDB needs to download this information. So you make a sysroot of your Windows machine on your GNU/Linux machine, at /sysroot, such that you have locally: /sysroot/E:/the/directory/program.exe /sysroot/D:/my/debug/E/the/directory/program.debug Then, you point GDB to the executable in your local sysroot, and it should At this point, the local filesystem > >> 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... > > But if we do that, how do we distinguish between the use case you > describe above and a use case where the we debug locally and the file > names just happen to include colons? Are we willing to restrict file > names on GNU/Linux to support the remote debugging on Windows? > > Thanks for the other comments, I will implement them when we agree > about the above issue. On 2019-04-27 12:39, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org >> From: Simon Marchi <simark@simark.ca> >> Date: Sat, 27 Apr 2019 12:16:23 -0400 >> >> 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. > > When debugging remotely, is the debug info on a Windows or on a > GNU/Linux filesystem? If the latter, the above will work. I always > thought that in remote debugging, GDB itself runs on the local host, > i.e. on GNU/Linux in this case, and the part that runs on the remote > is gdbserver. Isn't that correct? The latter part is correct, gdbserver would run on the Windows machine, while GDB would run on the local GNU/Linux machine. But the debug info can come from either places, depending on the setup. If the executable read by GDB comes from the "target:" filesystem, we will search for the separate debug files on the target: filesystem (hence all this complexity with target_prefix in this function). If you are debugging remotely, the target: filesystem represents the remote filesystem, which GDB can read through requests serviced by gdbserver. If you have a local copy of the remote filesystem (a sysroot) and made "set sysroot" point to it, then GDB will read from there. I tried to think about different scenarios, it brought more questions than answers, but here it is. If you are debugging locally on Windows, you want the path to end up like: target:D:/my/debug/E/the/directory/program.debug If you are debugging remotely your Windows from another Windows, using "set sysroot target:" (the default), you also want the path to end up like target:D:/my/debug/E/the/directory/program.debug If you are debugging remotely from a Windows machine to another Windows machine, but installed a sysroot of the debugged machine in F:/sysroot, then I guess you'll want the path to end up like F:/sysroot/D/my/debug/E/the/directory/program.debug We couldn't have a "D:" directory, so maybe we would want to apply the same treatment to the sysroot drive letter? If you are debugging remotely your Windows from GNU/Linux, using "set sysroot target:", we would want the path to end up like target:D:/my/debug/E/the/directory/program.debug ... since the target filesystem wouldn't support a "E:" directory. If you are debugging remotely your Windows from GNU/Linux, but installed a sysroot of the debugged machine in /sysroot, then either of these could work: /sysroot/D:/my/debug/E:/the/directory/program.debug /sysroot/D:/my/debug/E/the/directory/program.debug /sysroot/D/my/debug/E:/the/directory/program.debug /sysroot/D/my/debug/E/the/directory/program.debug This leads me to say that for simplicity, when we convert a Windows drive letter into a directory component, we should always convert it to the letter without the colon. This way, whatever platform GDB itself runs on, the searched path will be the same and predictable. Also, if you were to copy D:/my/debug/E/the/directory/program.debug over to your sysroot on the GNU/Linux machine, you wouldn't have to rename the "E" directory to "E:" for it to be found. Now, the question remains: how do we know for sure a path is a Windows path, in which we need to strip the colon? I don't know. Given the complexity of supporting all these scenarios (if we want to support them, we at least want to test them, which is a lot of work) and the fact that they are all theoretical at this point, I would be fine to go with what you had in your patch (using HAS_DRIVE_SPEC). If somebody ever needs to remotely debug a Windows machine from a Linux machine and have GDB find separate debug file in the remote debug file directory... then they can add the missing pieces :). Simon
> Date: Sat, 27 Apr 2019 14:56:11 -0400 > From: Simon Marchi <simark@simark.ca> > Cc: gdb-patches@sourceware.org > > Given the complexity of supporting all these scenarios (if we want to > support them, we at least want to test them, which is a lot of work) and > the fact that they are all theoretical at this point, I would be fine to > go with what you had in your patch (using HAS_DRIVE_SPEC). If somebody > ever needs to remotely debug a Windows machine from a Linux machine and > have GDB find separate debug file in the remote debug file directory... > then they can add the missing pieces :). OK, thanks. I will wait for a day or two for any other comments, and if nothing comes up, will push with a FIXME comment regarding the remote debugging use cases.
> Date: Sat, 27 Apr 2019 22:04:37 +0300 > From: Eli Zaretskii <eliz@gnu.org> > CC: gdb-patches@sourceware.org > > OK, thanks. I will wait for a day or two for any other comments, and > if nothing comes up, will push with a FIXME comment regarding the > remote debugging use cases. Done.
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)) + { + drive = std::string (1, dir_notarget[0]); + dir_notarget = STRIP_DRIVE_SPEC (dir_notarget); + } + else + drive = ""; + for (const gdb::unique_xmalloc_ptr<char> &debugdir : debugdir_vec) { debugfile = target_prefix ? "target:" : ""; debugfile += debugdir.get (); debugfile += "/"; + debugfile += drive; debugfile += dir_notarget; debugfile += debuglink;