* Re: [PATCH] Don't set dir separator if path has drive spec
2013-06-04 0:32 [PATCH] Don't set dir separator if path has drive spec Yao Qi
@ 2013-06-04 0:48 ` Doug Evans
2013-06-04 1:33 ` Yao Qi
2013-09-27 12:58 ` [PATCH] Fix regular /path/to/directory sysroots and target reported dll paths with drive specs. (was: Re: [PATCH] Don't set dir separator if path has drive spec) Pedro Alves
1 sibling, 1 reply; 6+ messages in thread
From: Doug Evans @ 2013-06-04 0:48 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On Mon, Jun 3, 2013 at 5:33 PM, Yao Qi <yao@codesourcery.com> wrote:
> Hi,
> When I debug a remote Windows program on Linux host, set sysroot to
> "remote:", find GDB can't load these dlls from the remote. If the dll
> name is "C:/Windows/system32/ntdll.dll" and GDB add "remote:" prefix
> to the path. The dll name becomes "remote:/C:/Windows/system32/ntdll.dll"
> and GDBserver is confused by the name. The first slash shouldn't be
> added. In solib.c:solib_find,
>
> need_dir_separator = !IS_DIR_SEPARATOR (in_pathname[0]);
>
> it controls whether to insert dir separator after sysroot. It works
> for UNIX-like file path, but it doesn't work for the DOS-like absolute
> path. This patch is to fix this problem. Is it OK?
>
> gdb:
>
> 2013-06-04 Yao Qi <yao@codesourcery.com>
>
> * solib.c (solib_find): Don't need dir separator if path has
> drive spec.
> ---
> gdb/solib.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/gdb/solib.c b/gdb/solib.c
> index a3479c5..d0392b4 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -230,7 +230,8 @@ solib_find (char *in_pathname, int *fd)
> {
> int need_dir_separator;
>
> - need_dir_separator = !IS_DIR_SEPARATOR (in_pathname[0]);
> + need_dir_separator = (!IS_DIR_SEPARATOR (in_pathname[0])
> + && !HAS_TARGET_DRIVE_SPEC (fskind, in_pathname));
>
> /* Cat the prefixed pathname together. */
> temp_pathname = concat (sysroot,
Hi. Looks right to me.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] Fix regular /path/to/directory sysroots and target reported dll paths with drive specs. (was: Re: [PATCH] Don't set dir separator if path has drive spec)
2013-06-04 0:32 [PATCH] Don't set dir separator if path has drive spec Yao Qi
2013-06-04 0:48 ` Doug Evans
@ 2013-09-27 12:58 ` Pedro Alves
2013-09-27 14:18 ` [PATCH] Fix regular /path/to/directory sysroots and target reported dll paths with drive specs Yao Qi
1 sibling, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2013-09-27 12:58 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 06/04/2013 01:33 AM, Yao Qi wrote:
> Hi,
> When I debug a remote Windows program on Linux host, set sysroot to
> "remote:", find GDB can't load these dlls from the remote. If the dll
> name is "C:/Windows/system32/ntdll.dll" and GDB add "remote:" prefix
> to the path. The dll name becomes "remote:/C:/Windows/system32/ntdll.dll"
> and GDBserver is confused by the name. The first slash shouldn't be
> added. In solib.c:solib_find,
>
> need_dir_separator = !IS_DIR_SEPARATOR (in_pathname[0]);
>
> it controls whether to insert dir separator after sysroot. It works
> for UNIX-like file path, but it doesn't work for the DOS-like absolute
> path. This patch is to fix this problem. Is it OK?
>
> gdb:
>
> 2013-06-04 Yao Qi <yao@codesourcery.com>
>
> * solib.c (solib_find): Don't need dir separator if path has
> drive spec.
This actually broke sysroots that are _not_ "remote:".
Comments on the patch below?
------------
Subject: [PATCH] Fix regular /path/to/directory sysroots and target reported
dll paths with drive specs.
I tried debugging a remote Windows program on Linux host, and pointed the
sysroot to "/some/path/" rather than "remote:", and I found GDB couldn't
find the dlls in the sysroot. If the dll name is
"C:/Windows/system32/ntdll.dll", I end up with the sysroot+in_pathname
concatenated this way:
(top-gdb) p temp_pathname
$1 = 0x228b690 "/some/pathC:/Windows/system32/ntdll.dll"
^^
That is, a directory separator is missing. This is a regression.
The problem is that solib_find decides that since the target path has
a drive spec, a separator is not necessary, which is clearly wrong in
this case. That check was added in
<https://sourceware.org/ml/gdb-patches/2013-06/msg00028.html>, to
handle the case of sysroot being "remote:". This patch fixes that
original issue in a different way. Instead of checking whether the
path has a drive spec, check whether the sysroot is "remote:". The
patch adds a table that helps visualize the cases that need a
separator. I also confirmed the original issue is still handled as
expected. That is, that "set sysroot remote:" still does the right
thing.
remote_filename_p returns true if the filename is prefixed with
"remote:". In this case, we need to check whether the filename is
exactly "remote:". I thought of different ways or either changing
remote_filename_p or adding another convenience function to remote.c
to avoid exposing the "remote:" prefix out of remote.c. But all
attempts turned out adding lot of over needless complication. So the
patch just exposes the prefix behind a new macro, which allows using a
straighforward strcmp.
gdb/
2013-09-27 Pedro Alves <palves@redhat.com>
* remote.h (REMOTE_SYSROOT_PREFIX): New define.
(remote_filename_p): Add comment.
* remote.c (remote_filename_p): Adjust to use
REMOTE_SYSROOT_PREFIX.
* solib.c (solib_find): When deciding whether we need to add a
directory separator, check whether the sysroot is "remote:"
instead of checking whether the patch has a drive spec. Add
comments.
---
gdb/remote.c | 4 +++-
gdb/remote.h | 7 +++++++
gdb/solib.c | 21 +++++++++++++++++++--
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 0fa1e2b..a9ef297 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10036,7 +10036,9 @@ remote_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb)
int
remote_filename_p (const char *filename)
{
- return strncmp (filename, "remote:", 7) == 0;
+ return strncmp (filename,
+ REMOTE_SYSROOT_PREFIX,
+ sizeof (REMOTE_SYSROOT_PREFIX) - 1) == 0;
}
bfd *
diff --git a/gdb/remote.h b/gdb/remote.h
index c036db7..b6ea547 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -57,6 +57,13 @@ void remote_file_delete (const char *remote_file, int from_tty);
bfd *remote_bfd_open (const char *remote_file, const char *target);
+/* If a path starts with this sequence, GDB will retrieve the target
+ libraries from the remote system. */
+
+#define REMOTE_SYSROOT_PREFIX "remote:"
+
+/* True if FILENAME starts with REMOTE_SYSROOT_PREFIX. */
+
int remote_filename_p (const char *filename);
extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
diff --git a/gdb/solib.c b/gdb/solib.c
index 5456d6f..5516b3e 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -230,8 +230,25 @@ solib_find (char *in_pathname, int *fd)
{
int need_dir_separator;
- need_dir_separator = (!IS_DIR_SEPARATOR (in_pathname[0])
- && !HAS_TARGET_DRIVE_SPEC (fskind, in_pathname));
+ /* Concatenate the sysroot and the target reported filename. We
+ may need to glue them with a directory separator. Cases to
+ consider:
+
+ | sysroot | separator | in_pathname |
+ |-----------------+-----------+----------------|
+ | /some/dir | / | c:/foo/bar.dll |
+ | /some/dir | | /foo/bar.dll |
+ | remote: | | c:/foo/bar.dll |
+ | remote: | | /foo/bar.dll |
+ | remote:some/dir | / | c:/foo/bar.dll |
+ | remote:some/dir | | /foo/bar.dll |
+
+ IOW, we don't need to add a separator if IN_PATHNAME already
+ has one, or when the the sysroot is exactly "remote:".
+ There's no need to check for drive spec explicitly, as we only
+ get here if IN_PATHNAME is considered an absolute path. */
+ need_dir_separator = !(IS_DIR_SEPARATOR (in_pathname[0])
+ || strcmp (REMOTE_SYSROOT_PREFIX, sysroot) == 0);
/* Cat the prefixed pathname together. */
temp_pathname = concat (sysroot,
--
1.7.11.7
^ permalink raw reply [flat|nested] 6+ messages in thread