[rfc] Retrieve libraries from remote target

[rfc] Retrieve libraries from remote target

[Ulrich Weigand]

Hello,

one issue that comes up regularly regarding remote debugging is the
requirement to have (exact) copies of the shared libraries used on
the remote system available on the host for use with GDB.

The following patch attempts to address this by enabling GDB to fetch
the original images of those libraries from the remote side (via the new
remote protocol file access routines).

The user interface is simple: if the path specified as "sysroot" starts
with the string "remote:", the sysroot location is assumed to be on the
remote target.  The rest of the sysroot string (which may be empty) is the
directory prefix used on the remote file system.

The implementations consists of a number of separate changes:

- A new function remote_bfd_open (with the same semantics as bfd_openr)
  opens a BFD that refers to a file on the remote system.

- The solib_open routine is replaced by a "solib_bfd_open" routine that
  returns not a file handle, but a BFD (same as symfile_bfd_open).

- solib_bfd_open and symfile_bfd_open are extended to handle paths
  starting with "remote:" by using remote_bfd_open instead of bfd_openr.

- Likewise, some other routines that (re-)open BFDs (e.g. separate
  debug files) are updated to handle remote BFDs.

I'd appreciate comments on that approach: does this make sense? what
do you think about the user interface?  am I missing other places that
open BFDs?

Bye,
Ulrich


ChangeLog:

	* remote.h (remote_filename_p, remote_bfd_open): Add prototypes.
	* remote.c (remote_bfd_iovec_open, remote_bfd_iovec_close,
	remote_bfd_iovec_pread, remote_bfd_iovec_stat, remote_filename_p,
	remote_bfd_open): New functions.

	* solist.h (solib_open): Remove prototype.
	(solib_bfd_open): Add prototype.
	* solib.c: Include "remote.h".
	(solib_open): Remove, replace by ...
	(solib_bfd_open): ... this new function.  Handle remote BFDs.
	(solib_map_sections): Replace solib_open by solib_bfd_open.
	* solib-frv.c: Include "exceptions.h".
	(enable_break2): Replace solib_open by solib_bfd_open.
	* solib-svr4.c: Include "exceptions.h".
	(enable_break): Replace solib_open by solib_bfd_open.

	* symfile.c: Include "remote.h".
	(build_id_verify): Handle remote BFDs.
	(separate_debug_file_exists): Use BFD to access file.  Handle
	remote BFDs.
	(symfile_bfd_open): Handle remote BFDs.
	(reread_symbols): Handle remote BFDs.

	* Makefile.in (solib.o): Update dependencies.
	(solib-frv.o, solib-svr4.o, symfile.o): Likewise.


diff -urNp gdb-orig/gdb/Makefile.in gdb-head/gdb/Makefile.in
--- gdb-orig/gdb/Makefile.in	2008-05-08 13:47:36.000000000 +0200
+++ gdb-head/gdb/Makefile.in	2008-05-08 22:03:11.083795755 +0200
@@ -2764,10 +2764,11 @@ solib.o: solib.c $(defs_h) $(gdb_string_
 	$(objfiles_h) $(exceptions_h) $(gdbcore_h) $(command_h) $(target_h) \
 	$(frame_h) $(gdb_regex_h) $(inferior_h) $(environ_h) $(language_h) \
 	$(gdbcmd_h) $(completer_h) $(filenames_h) $(exec_h) $(solist_h) \
-	$(observer_h) $(readline_h)
+	$(observer_h) $(readline_h) $(remote_h)
 solib-frv.o: solib-frv.c $(defs_h) $(gdb_string_h) $(inferior_h) \
 	$(gdbcore_h) $(solist_h) $(frv_tdep_h) $(objfiles_h) $(symtab_h) \
-	$(language_h) $(command_h) $(gdbcmd_h) $(elf_frv_h) $(solib_h)
+	$(language_h) $(command_h) $(gdbcmd_h) $(elf_frv_h) $(solib_h) \
+	$(exceptions_h)
 solib-irix.o: solib-irix.c $(defs_h) $(symtab_h) $(bfd_h) $(symfile_h) \
 	$(objfiles_h) $(gdbcore_h) $(target_h) $(inferior_h) $(solist_h) \
 	$(solib_h) $(solib_irix_h)
@@ -2787,7 +2788,7 @@ solib-svr4.o: solib-svr4.c $(defs_h) $(e
 	$(elf_mips_h) $(symtab_h) $(bfd_h) $(symfile_h) $(objfiles_h) \
 	$(gdbcore_h) $(target_h) $(inferior_h) $(gdb_assert_h) \
 	$(solist_h) $(solib_h) $(solib_svr4_h) $(bfd_target_h) $(elf_bfd_h) \
-	$(exec_h) $(auxv_h)
+	$(exec_h) $(auxv_h) $(exceptions_h)
 solib-target.o: solib-target.c $(defs_h) $(objfiles_h) $(solist_h) \
 	$(symtab_h) $(symfile_h) $(target_h) $(vec_h) $(xml_support_h) \
 	$(solib_target_h) $(gdb_string_h)
@@ -2900,7 +2901,7 @@ symfile.o: symfile.c $(defs_h) $(bfdlink
 	$(gdb_stabs_h) $(gdb_obstack_h) $(completer_h) $(bcache_h) \
 	$(hashtab_h) $(readline_h) $(gdb_assert_h) $(block_h) \
 	$(gdb_string_h) $(gdb_stat_h) $(observer_h) $(exec_h) \
-	$(parser_defs_h) $(varobj_h) $(elf_bfd_h) $(solib_h)
+	$(parser_defs_h) $(varobj_h) $(elf_bfd_h) $(solib_h) $(remote_h)
 symfile-mem.o: symfile-mem.c $(defs_h) $(symtab_h) $(gdbcore_h) \
 	$(objfiles_h) $(exceptions_h) $(gdbcmd_h) $(target_h) $(value_h) \
 	$(symfile_h) $(observer_h) $(auxv_h) $(elf_common_h)
diff -urNp gdb-orig/gdb/remote.c gdb-head/gdb/remote.c
--- gdb-orig/gdb/remote.c	2008-05-08 21:48:26.045546000 +0200
+++ gdb-head/gdb/remote.c	2008-05-08 22:03:11.153785707 +0200
@@ -6978,6 +6978,100 @@ remote_hostio_close_cleanup (void *opaqu
   remote_hostio_close (fd, &remote_errno);
 }
 
+
+static void *
+remote_bfd_iovec_open (struct bfd *abfd, void *open_closure)
+{
+  const char *filename = bfd_get_filename (abfd);
+  int fd, remote_errno;
+  int *stream;
+
+  gdb_assert (remote_filename_p (filename));
+
+  fd = remote_hostio_open (filename + 7, FILEIO_O_RDONLY, 0, &remote_errno);
+  if (fd == -1)
+    {
+      errno = remote_fileio_errno_to_host (remote_errno);
+      bfd_set_error (bfd_error_system_call);
+      return NULL;
+    }
+
+  stream = xmalloc (sizeof (int));
+  *stream = fd;
+  return stream;
+}
+
+static int
+remote_bfd_iovec_close (struct bfd *abfd, void *stream)
+{
+  int fd = *(int *)stream;
+  int remote_errno;
+
+  xfree (stream);
+
+  if (remote_hostio_close (fd, &remote_errno))
+    {
+      errno = remote_fileio_errno_to_host (remote_errno);
+      bfd_set_error (bfd_error_system_call);
+      return 0;
+    }
+
+  return 1;
+}
+
+static file_ptr
+remote_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf,
+			file_ptr nbytes, file_ptr offset)
+{
+  int fd = *(int *)stream;
+  int remote_errno;
+  file_ptr pos, bytes;
+
+  pos = 0;
+  while (nbytes > pos)
+    {
+      bytes = remote_hostio_pread (fd, (char *)buf + pos, nbytes - pos,
+				   offset + pos, &remote_errno);
+      if (bytes == 0)
+        /* Success, but no bytes, means end-of-file.  */
+        break;
+      if (bytes == -1)
+	{
+	  errno = remote_fileio_errno_to_host (remote_errno);
+	  bfd_set_error (bfd_error_system_call);
+	  return -1;
+	}
+
+      pos += bytes;
+    }
+
+  return pos;
+}
+
+static int
+remote_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb)
+{
+  /* FIXME: We should probably implement remote_hostio_stat.  */
+  sb->st_size = INT_MAX;
+  return 0;
+}
+
+int
+remote_filename_p (const char *filename)
+{
+  return strncmp (filename, "remote:", 7) == 0;
+}
+
+bfd *
+remote_bfd_open (const char *remote_file, const char *target)
+{
+  return bfd_openr_iovec (remote_file, target,
+			  remote_bfd_iovec_open, NULL,
+			  remote_bfd_iovec_pread,
+			  remote_bfd_iovec_close,
+			  remote_bfd_iovec_stat);
+}
+
 void
 remote_file_put (const char *local_file, const char *remote_file, int from_tty)
 {
diff -urNp gdb-orig/gdb/remote.h gdb-head/gdb/remote.h
--- gdb-orig/gdb/remote.h	2008-01-01 23:53:12.000000000 +0100
+++ gdb-head/gdb/remote.h	2008-05-08 22:03:11.189913273 +0200
@@ -73,4 +73,8 @@ void remote_file_get (const char *remote
 		      int from_tty);
 void remote_file_delete (const char *remote_file, int from_tty);
 
+bfd *remote_bfd_open (const char *remote_file, const char *target);
+
+int remote_filename_p (const char *filename);
+
 #endif
diff -urNp gdb-orig/gdb/solib.c gdb-head/gdb/solib.c
--- gdb-orig/gdb/solib.c	2008-01-07 16:19:58.000000000 +0100
+++ gdb-head/gdb/solib.c	2008-05-08 22:03:11.196912269 +0200
@@ -44,6 +44,7 @@
 #include "solist.h"
 #include "observer.h"
 #include "readline/readline.h"
+#include "remote.h"
 
 /* Architecture-specific operations.  */
 
@@ -106,11 +107,11 @@ The search path for loading non-absolute
 
    GLOBAL FUNCTION
 
-   solib_open -- Find a shared library file and open it.
+   solib_bfd_open -- Find a shared library file and open BFD for it.
 
    SYNOPSIS
 
-   int solib_open (char *in_patname, char **found_pathname);
+   struct bfd *solib_bfd_open (char *in_pathname);
 
    DESCRIPTION
 
@@ -137,16 +138,17 @@ The search path for loading non-absolute
 
    RETURNS
 
-   file handle for opened solib, or -1 for failure.  */
+   BFD file handle for opened solib; throws error on failure.  */
 
-int
-solib_open (char *in_pathname, char **found_pathname)
+bfd *
+solib_bfd_open (char *in_pathname)
 {
   struct target_so_ops *ops = solib_ops (current_gdbarch);
   int found_file = -1;
   char *temp_pathname = NULL;
   char *p = in_pathname;
   int gdb_sysroot_is_empty;
+  bfd *abfd;
 
   gdb_sysroot_is_empty = (gdb_sysroot == NULL || *gdb_sysroot == 0);
 
@@ -168,6 +170,29 @@ solib_open (char *in_pathname, char **fo
       strcat (temp_pathname, in_pathname);
     }
 
+  /* Handle remote files.  */
+  if (remote_filename_p (temp_pathname))
+    {
+      temp_pathname = xstrdup (temp_pathname);
+      abfd = remote_bfd_open (temp_pathname, gnutarget);
+      if (!abfd)
+	{
+	  make_cleanup (xfree, temp_pathname);
+	  error (_("Could not open `%s' as an executable file: %s"),
+		 temp_pathname, bfd_errmsg (bfd_get_error ()));
+	}
+
+      if (!bfd_check_format (abfd, bfd_object))
+	{
+	  bfd_close (abfd);
+	  make_cleanup (xfree, temp_pathname);
+	  error (_("\"%s\": not in executable format: %s."),
+		 temp_pathname, bfd_errmsg (bfd_get_error ()));
+	}
+
+      return abfd;
+    }
+
   /* Now see if we can open it.  */
   found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0);
 
@@ -228,16 +253,30 @@ solib_open (char *in_pathname, char **fo
 			OPF_TRY_CWD_FIRST, in_pathname, O_RDONLY | O_BINARY, 0,
 			&temp_pathname);
 
-  /* Done.  If not found, tough luck.  Return found_file and 
-     (optionally) found_pathname.  */
-  if (temp_pathname)
+  /* Done.  If still not found, error.  */
+  if (found_file < 0)
+    perror_with_name (in_pathname);
+
+  /* Leave temp_pathname allocated.  abfd->name will point to it.  */
+  abfd = bfd_fopen (temp_pathname, gnutarget, FOPEN_RB, found_file);
+  if (!abfd)
     {
-      if (found_pathname != NULL)
-	*found_pathname = temp_pathname;
-      else
-	xfree (temp_pathname);
+      close (found_file);
+      make_cleanup (xfree, temp_pathname);
+      error (_("Could not open `%s' as an executable file: %s"),
+	     temp_pathname, bfd_errmsg (bfd_get_error ()));
+    }
+
+  if (!bfd_check_format (abfd, bfd_object))
+    {
+      bfd_close (abfd);
+      make_cleanup (xfree, temp_pathname);
+      error (_("\"%s\": not in executable format: %s."),
+	     temp_pathname, bfd_errmsg (bfd_get_error ()));
     }
-  return found_file;
+
+  bfd_set_cacheable (abfd, 1);
+  return abfd;
 }
 
 
@@ -273,46 +312,25 @@ solib_map_sections (void *arg)
 {
   struct so_list *so = (struct so_list *) arg;	/* catch_errors bogon */
   char *filename;
-  char *scratch_pathname;
-  int scratch_chan;
   struct section_table *p;
   struct cleanup *old_chain;
   bfd *abfd;
 
   filename = tilde_expand (so->so_name);
-
   old_chain = make_cleanup (xfree, filename);
-  scratch_chan = solib_open (filename, &scratch_pathname);
-
-  if (scratch_chan < 0)
-    {
-      perror_with_name (filename);
-    }
-
-  /* Leave scratch_pathname allocated.  abfd->name will point to it.  */
-  abfd = bfd_fopen (scratch_pathname, gnutarget, FOPEN_RB, scratch_chan);
-  if (!abfd)
-    {
-      close (scratch_chan);
-      error (_("Could not open `%s' as an executable file: %s"),
-	     scratch_pathname, bfd_errmsg (bfd_get_error ()));
-    }
+  abfd = solib_bfd_open (filename);
+  do_cleanups (old_chain);
 
   /* Leave bfd open, core_xfer_memory and "info files" need it.  */
   so->abfd = abfd;
-  bfd_set_cacheable (abfd, 1);
 
   /* copy full path name into so_name, so that later symbol_file_add
      can find it */
-  if (strlen (scratch_pathname) >= SO_NAME_MAX_PATH_SIZE)
-    error (_("Full path name length of shared library exceeds SO_NAME_MAX_PATH_SIZE in so_list structure."));
-  strcpy (so->so_name, scratch_pathname);
+  if (strlen (bfd_get_filename (abfd)) >= SO_NAME_MAX_PATH_SIZE)
+    error (_("Full path name length of shared library exceeds \
+SO_NAME_MAX_PATH_SIZE in so_list structure."));
+  strcpy (so->so_name, bfd_get_filename (abfd));
 
-  if (!bfd_check_format (abfd, bfd_object))
-    {
-      error (_("\"%s\": not in executable format: %s."),
-	     scratch_pathname, bfd_errmsg (bfd_get_error ()));
-    }
   if (build_section_table (abfd, &so->sections, &so->sections_end))
     {
       error (_("Can't find the file sections in `%s': %s"),
@@ -339,9 +357,6 @@ solib_map_sections (void *arg)
 	}
     }
 
-  /* Free the file names, close the file now.  */
-  do_cleanups (old_chain);
-
   return (1);
 }
 
diff -urNp gdb-orig/gdb/solib-frv.c gdb-head/gdb/solib-frv.c
--- gdb-orig/gdb/solib-frv.c	2008-01-01 23:53:13.000000000 +0100
+++ gdb-head/gdb/solib-frv.c	2008-05-08 22:03:11.203911264 +0200
@@ -30,6 +30,7 @@
 #include "command.h"
 #include "gdbcmd.h"
 #include "elf/frv.h"
+#include "exceptions.h"
 
 /* Flag which indicates whether internal debug messages should be printed.  */
 static int solib_frv_debug;
@@ -645,12 +646,11 @@ enable_break2 (void)
       unsigned int interp_sect_size;
       gdb_byte *buf;
       bfd *tmp_bfd = NULL;
-      int tmp_fd = -1;
-      char *tmp_pathname = NULL;
       int status;
       CORE_ADDR addr, interp_loadmap_addr;
       gdb_byte addr_buf[FRV_PTR_SIZE];
       struct int_elf32_fdpic_loadmap *ldm;
+      volatile struct gdb_exception ex;
 
       /* Read the contents of the .interp section into a local buffer;
          the contents specify the dynamic linker this program uses.  */
@@ -668,25 +668,16 @@ enable_break2 (void)
          be trivial on GNU/Linux).  Therefore, we have to try an alternate
          mechanism to find the dynamic linker's base address.  */
 
-      tmp_fd  = solib_open (buf, &tmp_pathname);
-      if (tmp_fd >= 0)
-	tmp_bfd = bfd_fopen (tmp_pathname, gnutarget, FOPEN_RB, tmp_fd);
-
+      TRY_CATCH (ex, RETURN_MASK_ALL)
+        {
+          tmp_bfd = solib_bfd_open (buf);
+        }
       if (tmp_bfd == NULL)
 	{
 	  enable_break_failure_warning ();
 	  return 0;
 	}
 
-      /* Make sure the dynamic linker is really a useful object.  */
-      if (!bfd_check_format (tmp_bfd, bfd_object))
-	{
-	  warning (_("Unable to grok dynamic linker %s as an object file"), buf);
-	  enable_break_failure_warning ();
-	  bfd_close (tmp_bfd);
-	  return 0;
-	}
-
       status = frv_fdpic_loadmap_addresses (current_gdbarch,
                                             &interp_loadmap_addr, 0);
       if (status < 0)
diff -urNp gdb-orig/gdb/solib-svr4.c gdb-head/gdb/solib-svr4.c
--- gdb-orig/gdb/solib-svr4.c	2008-03-07 20:31:38.000000000 +0100
+++ gdb-head/gdb/solib-svr4.c	2008-05-08 22:03:11.211910116 +0200
@@ -42,6 +42,7 @@
 #include "elf-bfd.h"
 #include "exec.h"
 #include "auxv.h"
+#include "exceptions.h"
 
 static struct link_map_offsets *svr4_fetch_link_map_offsets (void);
 static int svr4_have_link_map_offsets (void);
@@ -1084,8 +1085,7 @@ enable_break (void)
       struct so_list *so;
       bfd *tmp_bfd = NULL;
       struct target_ops *tmp_bfd_target;
-      int tmp_fd = -1;
-      char *tmp_pathname = NULL;
+      volatile struct gdb_exception ex;
 
       /* Read the contents of the .interp section into a local buffer;
          the contents specify the dynamic linker this program uses.  */
@@ -1104,21 +1104,13 @@ enable_break (void)
          be trivial on GNU/Linux).  Therefore, we have to try an alternate
          mechanism to find the dynamic linker's base address.  */
 
-      tmp_fd = solib_open (buf, &tmp_pathname);
-      if (tmp_fd >= 0)
-	tmp_bfd = bfd_fopen (tmp_pathname, gnutarget, FOPEN_RB, tmp_fd);
-
+      TRY_CATCH (ex, RETURN_MASK_ALL)
+        {
+	  tmp_bfd = solib_bfd_open (buf);
+	}
       if (tmp_bfd == NULL)
 	goto bkpt_at_symbol;
 
-      /* Make sure the dynamic linker's really a useful object.  */
-      if (!bfd_check_format (tmp_bfd, bfd_object))
-	{
-	  warning (_("Unable to grok dynamic linker %s as an object file"), buf);
-	  bfd_close (tmp_bfd);
-	  goto bkpt_at_symbol;
-	}
-
       /* Now convert the TMP_BFD into a target.  That way target, as
          well as BFD operations can be used.  Note that closing the
          target will also close the underlying bfd.  */
@@ -1212,7 +1204,6 @@ enable_break (void)
       /* For whatever reason we couldn't set a breakpoint in the dynamic
          linker.  Warn and drop into the old code.  */
     bkpt_at_symbol:
-      xfree (tmp_pathname);
       warning (_("Unable to find dynamic linker breakpoint function.\n"
                "GDB will be unable to debug shared library initializers\n"
                "and track explicitly loaded dynamic code."));
diff -urNp gdb-orig/gdb/solist.h gdb-head/gdb/solist.h
--- gdb-orig/gdb/solist.h	2008-01-07 16:19:58.000000000 +0100
+++ gdb-head/gdb/solist.h	2008-05-08 22:03:11.254903944 +0200
@@ -128,7 +128,7 @@ void free_so (struct so_list *so);
 struct so_list *master_so_list (void);
 
 /* Find solib binary file and open it.  */
-extern int solib_open (char *in_pathname, char **found_pathname);
+extern bfd *solib_bfd_open (char *in_pathname);
 
 /* FIXME: gdbarch needs to control this variable */
 extern struct target_so_ops *current_target_so_ops;
diff -urNp gdb-orig/gdb/symfile.c gdb-head/gdb/symfile.c
--- gdb-orig/gdb/symfile.c	2008-05-08 13:47:38.000000000 +0200
+++ gdb-head/gdb/symfile.c	2008-05-08 22:03:11.267902078 +0200
@@ -53,6 +53,7 @@
 include "varobj.h"
 #include "elf-bfd.h"
 #include "solib.h"
+#include "remote.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -1255,7 +1256,10 @@ build_id_verify (const char *filename, s
   int retval = 0;
 
   /* We expect to be silent on the non-existing files.  */
-  abfd = bfd_openr (filename, gnutarget);
+  if (remote_filename_p (filename))
+    abfd = remote_bfd_open (filename, gnutarget);
+  else
+    abfd = bfd_openr (filename, gnutarget);
   if (abfd == NULL)
     return 0;
 
@@ -1346,18 +1350,22 @@ static int
 separate_debug_file_exists (const char *name, unsigned long crc)
 {
   unsigned long file_crc = 0;
-  int fd;
+  bfd *abfd;
   gdb_byte buffer[8*1024];
   int count;
 
-  fd = open (name, O_RDONLY | O_BINARY);
-  if (fd < 0)
+  if (remote_filename_p (name))
+    abfd = remote_bfd_open (name, gnutarget);
+  else
+    abfd = bfd_openr (name, gnutarget);
+
+  if (!abfd)
     return 0;
 
-  while ((count = read (fd, buffer, sizeof (buffer))) > 0)
+  while ((count = bfd_bread (buffer, sizeof (buffer), abfd)) > 0)
     file_crc = gnu_debuglink_crc32 (file_crc, buffer, count);
 
-  close (fd);
+  bfd_close (abfd);
 
   return crc == file_crc;
 }
@@ -1603,6 +1611,23 @@ symfile_bfd_open (char *name)
   int desc;
   char *absolute_name;
 
+  if (remote_filename_p (name))
+    {
+      sym_bfd = remote_bfd_open (name, gnutarget);
+      if (!sym_bfd)
+	error (_("\"%s\": can't open to read symbols: %s."), name,
+	       bfd_errmsg (bfd_get_error ()));
+
+      if (!bfd_check_format (sym_bfd, bfd_object))
+	{
+	  bfd_close (sym_bfd);
+	  error (_("\"%s\": can't read symbols: %s."), name,
+		 bfd_errmsg (bfd_get_error ()));
+	}
+
+      return sym_bfd;
+    }
+
   name = tilde_expand (name);	/* Returns 1st new malloc'd copy.  */
 
   /* Look down path for it, allocate 2nd new malloc'd copy.  */
@@ -2346,7 +2371,10 @@ reread_symbols (void)
 	      if (!bfd_close (objfile->obfd))
 		error (_("Can't close BFD for %s: %s"), objfile->name,
 		       bfd_errmsg (bfd_get_error ()));
-	      objfile->obfd = bfd_openr (obfd_filename, gnutarget);
+	      if (remote_filename_p (obfd_filename))
+		objfile->obfd = remote_bfd_open (obfd_filename, gnutarget);
+	      else
+		objfile->obfd = bfd_openr (obfd_filename, gnutarget);
 	      if (objfile->obfd == NULL)
 		error (_("Can't open %s to read symbols."), objfile->name);
 	      /* bfd_openr sets cacheable to true, which is what we want.  */
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Retrieve libraries from remote target

[Daniel Jacobowitz]

On Thu, May 08, 2008 at 10:21:56PM +0200, Ulrich Weigand wrote:
> The user interface is simple: if the path specified as "sysroot" starts
> with the string "remote:", the sysroot location is assumed to be on the
> remote target.  The rest of the sysroot string (which may be empty) is the
> directory prefix used on the remote file system.

Very interesting.  What's the performance like on this?

One interesting thing is that this will probably make "set
trust-readonly-sections" slower than reading target memory.
Another is that the list of open files on the remote target is tied to
the remote connection; if we disconnect from the remote target with a
BFD still open, what happens?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Retrieve libraries from remote target

[Eli Zaretskii]

> Date: Thu, 8 May 2008 22:21:56 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> The user interface is simple: if the path specified as "sysroot" starts
> with the string "remote:", the sysroot location is assumed to be on the
> remote target.

What if my _local_ "sysroot" happens to begin with the literal string
"remote:"?  How can I tell GDB this is not a remote location?

> +	  error (_("Could not open `%s' as an executable file: %s"),
> +		 temp_pathname, bfd_errmsg (bfd_get_error ()));

In other error messages, you used "..", not `..', to quote file names.
I like `..' better, and I think we use `..' in more places than we use
"..".  I think consistency is important in user messages.

> +  if (strlen (bfd_get_filename (abfd)) >= SO_NAME_MAX_PATH_SIZE)
> +    error (_("Full path name length of shared library exceeds \
> +SO_NAME_MAX_PATH_SIZE in so_list structure."));

Is this really a useful message, especially since we don't show the
actual value of SO_NAME_MAX_PATH_SIZE and the actual length of the
file name?  I say either show the two numbers explicitly, or just tell
it's too long.  (Yes, I know you only made a minor modification of an
existing message.)

Also, GNU coding standards frown on using "path name", they want us to
use "file name".  If you do that, I think you can lose the "Full"
part.

Finally, this needs documentation before it is committed.

Thanks!

Re: [rfc] Retrieve libraries from remote target

[Ulrich Weigand]

Eli Zaretskii wrote:
> > Date: Thu, 8 May 2008 22:21:56 +0200 (CEST)
> > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > 
> > The user interface is simple: if the path specified as "sysroot" starts
> > with the string "remote:", the sysroot location is assumed to be on the
> > remote target.
> 
> What if my _local_ "sysroot" happens to begin with the literal string
> "remote:"?  How can I tell GDB this is not a remote location?

You cannot.  Is this limitation a real problem?

> > +	  error (_("Could not open `%s' as an executable file: %s"),
> > +		 temp_pathname, bfd_errmsg (bfd_get_error ()));
> 
> In other error messages, you used "..", not `..', to quote file names.
> I like `..' better, and I think we use `..' in more places than we use
> "..".  I think consistency is important in user messages.

All messages in my patch were just copies from one place to another;
but I agree that they should be consistent.  I'll change everything
to use `..'.

> > +  if (strlen (bfd_get_filename (abfd)) >= SO_NAME_MAX_PATH_SIZE)
> > +    error (_("Full path name length of shared library exceeds \
> > +SO_NAME_MAX_PATH_SIZE in so_list structure."));
> 
> Is this really a useful message, especially since we don't show the
> actual value of SO_NAME_MAX_PATH_SIZE and the actual length of the
> file name?  I say either show the two numbers explicitly, or just tell
> it's too long.  (Yes, I know you only made a minor modification of an
> existing message.)

I think just "Shared library file name is too long." or something
similar would be enough; the user cannot really do anything in that
situation anyway ...

> Also, GNU coding standards frown on using "path name", they want us to
> use "file name".  If you do that, I think you can lose the "Full"
> part.

OK.

> Finally, this needs documentation before it is committed.

Sure, I'll add that.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Retrieve libraries from remote target

[Eli Zaretskii]

> Date: Fri, 9 May 2008 21:28:46 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> Cc: gdb-patches@sourceware.org
> 
> > > The user interface is simple: if the path specified as "sysroot" starts
> > > with the string "remote:", the sysroot location is assumed to be on the
> > > remote target.
> > 
> > What if my _local_ "sysroot" happens to begin with the literal string
> > "remote:"?  How can I tell GDB this is not a remote location?
> 
> You cannot.  Is this limitation a real problem?

If it can happen in real usage, then I think it's a real problem.

Re: [rfc] Retrieve libraries from remote target

[Ulrich Weigand]

Eli Zaretskii wrote:
> > Date: Fri, 9 May 2008 21:28:46 +0200 (CEST)
> > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > Cc: gdb-patches@sourceware.org
> > 
> > > > The user interface is simple: if the path specified as "sysroot" starts
> > > > with the string "remote:", the sysroot location is assumed to be on the
> > > > remote target.
> > > 
> > > What if my _local_ "sysroot" happens to begin with the literal string
> > > "remote:"?  How can I tell GDB this is not a remote location?
> > 
> > You cannot.  Is this limitation a real problem?
> 
> If it can happen in real usage, then I think it's a real problem.

Well, in real usage the sysroot tends to be specified as absolute
path, so that problem cannot occur.  If you do want to specify the
sysroot as relative path, there typically are multiple ways to 
identify the same directory, e.g. on Unix you could always use
"./remote:" instead of "remote:" ...  (I am not 100% certain that
something like that is possible on any host OS supported by GDB.)

But I'm certainly open to alternative interfaces.  Do you have
suggestions for a different approach?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Retrieve libraries from remote target

[Eli Zaretskii]

> Date: Sat, 10 May 2008 00:09:53 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> Cc: gdb-patches@sourceware.org
> 
> Eli Zaretskii wrote:
> > > Date: Fri, 9 May 2008 21:28:46 +0200 (CEST)
> > > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > > Cc: gdb-patches@sourceware.org
> > > 
> > > > > The user interface is simple: if the path specified as "sysroot" starts
> > > > > with the string "remote:", the sysroot location is assumed to be on the
> > > > > remote target.
> > > > 
> > > > What if my _local_ "sysroot" happens to begin with the literal string
> > > > "remote:"?  How can I tell GDB this is not a remote location?
> > > 
> > > You cannot.  Is this limitation a real problem?
> > 
> > If it can happen in real usage, then I think it's a real problem.
> 
> Well, in real usage the sysroot tends to be specified as absolute
> path, so that problem cannot occur.  If you do want to specify the
> sysroot as relative path, there typically are multiple ways to 
> identify the same directory, e.g. on Unix you could always use
> "./remote:" instead of "remote:" ...  (I am not 100% certain that
> something like that is possible on any host OS supported by GDB.)
> 
> But I'm certainly open to alternative interfaces.  Do you have
> suggestions for a different approach?

If "./remote:" will work, I'm fine with that; we just need to mention
that in the manual.  Can we verify that "./remote:" will work with
sysroot?

Failing that, some kind of escape-protecting the colon will do.

Re: [rfc] Retrieve libraries from remote target

[Daniel Jacobowitz]

On Sat, May 10, 2008 at 12:11:43PM +0300, Eli Zaretskii wrote:
> If "./remote:" will work, I'm fine with that; we just need to mention
> that in the manual.  Can we verify that "./remote:" will work with
> sysroot?

I have used ./ on sysroot paths in the past so I'm confident this
will work.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Retrieve libraries from remote target

[Eli Zaretskii]

> Date: Sun, 11 May 2008 17:25:37 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Ulrich Weigand <uweigand@de.ibm.com>, gdb-patches@sourceware.org
> 
> On Sat, May 10, 2008 at 12:11:43PM +0300, Eli Zaretskii wrote:
> > If "./remote:" will work, I'm fine with that; we just need to mention
> > that in the manual.  Can we verify that "./remote:" will work with
> > sysroot?
> 
> I have used ./ on sysroot paths in the past so I'm confident this
> will work.

Thanks.

Ulrich, we are okay as long as ./ is mentioned in the manual, perhaps
in a @footnote or something.