diff mbox

[1/5] Introduce build_debug_file_name

Message ID 1434447768-17328-2-git-send-email-gbenson@redhat.com
State New, archived
Headers

Commit Message

Gary Benson June 16, 2015, 9:42 a.m. UTC 
  This commit introduces a new function build_debug_file_name which
concatenates a series of filename components into a filename.
find_separate_debug_file is updated to use build_debug_file_name.
A later commit in this series will extend build_debug_file_name
to correctly handle "target:" prefixes, so it is convenient to
have filename building pulled out into one function.  For now the
only functional change here is that the original code sometimes
generated filenames with repeated directory separators while the
new code does not.

gdb/ChangeLog:

	* gdb/symfile.c (build_debug_file_name): New function.
	(find_separate_debug_file): Use the above to build filenames.
---
 gdb/ChangeLog |    5 ++
 gdb/symfile.c |  117 +++++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 90 insertions(+), 32 deletions(-)

Comments

Eli Zaretskii June 16, 2015, 2:46 p.m. UTC | #1 
> From: Gary Benson <gbenson@redhat.com>
> Cc: Cédric Buissart <cedric.buissart@gmail.com>
> Date: Tue, 16 Jun 2015 10:42:44 +0100
> 
> +/* Build the filename of a separate debug file from an arbitrary
> +   number of components.  Returns an xmalloc'd string that must
> +   be be freed by the caller.  The final argument of this function
> +   must be NULL to mark the end the argument list.  */
> +
> +static char *
> +build_debug_file_name (const char *first, ...)
> +{
> +  va_list ap;
> +  const char *arg, *last;
> +  VEC (char_ptr) *args = NULL;
> +  struct cleanup *back_to = make_cleanup_free_char_ptr_vec (args);
> +  int bufsiz = 0;
> +  char *buf, *tmp;
> +  int i;
> +
> +  va_start (ap, first);
> +  for (arg = first; arg; arg = va_arg (ap, const char *))
> +    last = arg;
> +  va_end (ap);
> +
> +  va_start (ap, first);
> +  for (arg = first; arg; arg = va_arg (ap, const char *))
> +    {
> +      if (arg == last)
> +	tmp = xstrdup (arg);
> +      else
> +	{
> +	  int len;
> +
> +	  /* Strip leading separators from subdirectories.  */
> +	  if (arg != first)
> +	    {
> +	      while (*arg != '\0' && IS_DIR_SEPARATOR (*arg))
> +		arg++;
> +	    }
> +
> +	  /* Strip trailing separators.  */
> +	  len = strlen (arg);
> +
> +	  while (len > 0 && IS_DIR_SEPARATOR (arg[len - 1]))
> +	    len--;

Was this logic tested with Windows-style "d:/foo" file names?  E.g.,
what will happen if the first component is "d:/"?

Thanks.
Gary Benson June 17, 2015, 9:47 a.m. UTC | #2 
Eli Zaretskii wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: Cédric Buissart <cedric.buissart@gmail.com>
> > Date: Tue, 16 Jun 2015 10:42:44 +0100
> > 
> > +/* Build the filename of a separate debug file from an arbitrary
> > +   number of components.  Returns an xmalloc'd string that must
> > +   be be freed by the caller.  The final argument of this function
> > +   must be NULL to mark the end the argument list.  */
> > +
> > +static char *
> > +build_debug_file_name (const char *first, ...)
> > +{
> > +  va_list ap;
> > +  const char *arg, *last;
> > +  VEC (char_ptr) *args = NULL;
> > +  struct cleanup *back_to = make_cleanup_free_char_ptr_vec (args);
> > +  int bufsiz = 0;
> > +  char *buf, *tmp;
> > +  int i;
> > +
> > +  va_start (ap, first);
> > +  for (arg = first; arg; arg = va_arg (ap, const char *))
> > +    last = arg;
> > +  va_end (ap);
> > +
> > +  va_start (ap, first);
> > +  for (arg = first; arg; arg = va_arg (ap, const char *))
> > +    {
> > +      if (arg == last)
> > +	tmp = xstrdup (arg);
> > +      else
> > +	{
> > +	  int len;
> > +
> > +	  /* Strip leading separators from subdirectories.  */
> > +	  if (arg != first)
> > +	    {
> > +	      while (*arg != '\0' && IS_DIR_SEPARATOR (*arg))
> > +		arg++;
> > +	    }
> > +
> > +	  /* Strip trailing separators.  */
> > +	  len = strlen (arg);
> > +
> > +	  while (len > 0 && IS_DIR_SEPARATOR (arg[len - 1]))
> > +	    len--;
> 
> Was this logic tested with Windows-style "d:/foo" file names?  E.g.,
> what will happen if the first component is "d:/"?

I don't have access to any Windows machine so I haven't been able to
test this, but I don't think the new code would regress compared to
what it replaces (which doesn't seem to have been written with Windows
in mind at all, e.g. it uses "/" rather than SLASH_STRING and has no
particular handling for drive letters).

For the case you mention nothing would be stripped (the "d" in that
path is !IS_DIR_SEPARATOR) so the filename components would be
concatenated verbatim, just as with the original code.  The resulting
filename may not make sense, but it's not a regression.

I don't believe this series should be blocked unless it breaks
something that actually worked before.

Thanks,
Gary
Eli Zaretskii June 17, 2015, 4:41 p.m. UTC | #3 
> Date: Wed, 17 Jun 2015 10:47:34 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: gdb-patches@sourceware.org, cedric.buissart@gmail.com
> 
> For the case you mention nothing would be stripped (the "d" in that
> path is !IS_DIR_SEPARATOR) so the filename components would be
> concatenated verbatim, just as with the original code.  The resulting
> filename may not make sense, but it's not a regression.

But won't we produce "d://foo/bar" as result?

> I don't believe this series should be blocked unless it breaks
> something that actually worked before.

If a fix is very simple, why not make it?
Gary Benson June 18, 2015, 10:55 a.m. UTC | #4 
Eli Zaretskii wrote:
> > Date: Wed, 17 Jun 2015 10:47:34 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: gdb-patches@sourceware.org, cedric.buissart@gmail.com
> > 
> > For the case you mention nothing would be stripped (the "d" in
> > that path is !IS_DIR_SEPARATOR) so the filename components would
> > be concatenated verbatim, just as with the original code.  The
> > resulting filename may not make sense, but it's not a regression.
> 
> But won't we produce "d://foo/bar" as result?

No.  build_debug_file_name strips both leading and trailing slashes.
The only ways to get a double slash from build_debug_file_name is to
pass one in in the middle of a string.

  build_debug_file_name ("d:", "foo", "bar")          -> "d:/foo/bar"
  build_debug_file_name ("d:/", "/foo", "bar")        -> "d:/foo/bar"
  build_debug_file_name ("d:///", "///foo///", "bar") -> "d:/foo/bar"

but

  build_debug_file_name("d://foo", "bar") -> "d://foo/bar"
  
> > I don't believe this series should be blocked unless it breaks
> > something that actually worked before.
> 
> If a fix is very simple, why not make it?

I don't understand, what are you asking me to fix?

Thanks,
Gary
Eli Zaretskii June 18, 2015, 12:11 p.m. UTC | #5 
> Date: Thu, 18 Jun 2015 11:55:28 +0100
> From: Gary Benson <gbenson@redhat.com>
> Cc: gdb-patches@sourceware.org, cedric.buissart@gmail.com
> 
> > But won't we produce "d://foo/bar" as result?
> 
> No.  build_debug_file_name strips both leading and trailing slashes.
> The only ways to get a double slash from build_debug_file_name is to
> pass one in in the middle of a string.
> 
>   build_debug_file_name ("d:", "foo", "bar")          -> "d:/foo/bar"
>   build_debug_file_name ("d:/", "/foo", "bar")        -> "d:/foo/bar"
>   build_debug_file_name ("d:///", "///foo///", "bar") -> "d:/foo/bar"
> 
> but
> 
>   build_debug_file_name("d://foo", "bar") -> "d://foo/bar"
>   
> > > I don't believe this series should be blocked unless it breaks
> > > something that actually worked before.
> > 
> > If a fix is very simple, why not make it?
> 
> I don't understand, what are you asking me to fix?

Nothing, given the above.

Thanks.
Gary Benson June 19, 2015, 8:32 a.m. UTC | #6 
Eli Zaretskii wrote:
> > Date: Thu, 18 Jun 2015 11:55:28 +0100
> > From: Gary Benson <gbenson@redhat.com>
> > Cc: gdb-patches@sourceware.org, cedric.buissart@gmail.com
> > 
> > > But won't we produce "d://foo/bar" as result?
> > 
> > No.  build_debug_file_name strips both leading and trailing slashes.
> > The only ways to get a double slash from build_debug_file_name is to
> > pass one in in the middle of a string.
> > 
> >   build_debug_file_name ("d:", "foo", "bar")          -> "d:/foo/bar"
> >   build_debug_file_name ("d:/", "/foo", "bar")        -> "d:/foo/bar"
> >   build_debug_file_name ("d:///", "///foo///", "bar") -> "d:/foo/bar"
> > 
> > but
> > 
> >   build_debug_file_name("d://foo", "bar") -> "d://foo/bar"
> >   
> > > > I don't believe this series should be blocked unless it breaks
> > > > something that actually worked before.
> > > 
> > > If a fix is very simple, why not make it?
> > 
> > I don't understand, what are you asking me to fix?
> 
> Nothing, given the above.
> 
> Thanks.

Cool, thanks Eli.

Cheers,
Gary
Pedro Alves July 1, 2015, 11:05 a.m. UTC | #7 
On 06/16/2015 10:42 AM, Gary Benson wrote:
> This commit introduces a new function build_debug_file_name which
> concatenates a series of filename components into a filename.
> find_separate_debug_file is updated to use build_debug_file_name.
> A later commit in this series will extend build_debug_file_name
> to correctly handle "target:" prefixes, so it is convenient to
> have filename building pulled out into one function.  For now the
> only functional change here is that the original code sometimes
> generated filenames with repeated directory separators while the
> new code does not.

I'd drop the "debug" from the function's name.  Sounds like a
candidate for reuse elsewhere to me.

> 
> gdb/ChangeLog:
> 
> 	* gdb/symfile.c (build_debug_file_name): New function.
> 	(find_separate_debug_file): Use the above to build filenames.
> ---
>  gdb/ChangeLog |    5 ++
>  gdb/symfile.c |  117 +++++++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 90 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 0c35ffa..799133a 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1431,6 +1431,79 @@ separate_debug_file_exists (const char *name, unsigned long crc,
>    return 1;
>  }
>  
> +/* Build the filename of a separate debug file from an arbitrary
> +   number of components.  Returns an xmalloc'd string that must
> +   be be freed by the caller.  The final argument of this function
> +   must be NULL to mark the end the argument list.  */

double "be be".

> +
> +static char *
> +build_debug_file_name (const char *first, ...)
> +{
> +  va_list ap;
> +  const char *arg, *last;
> +  VEC (char_ptr) *args = NULL;
> +  struct cleanup *back_to = make_cleanup_free_char_ptr_vec (args);
> +  int bufsiz = 0;
> +  char *buf, *tmp;
> +  int i;
> +
> +  va_start (ap, first);
> +  for (arg = first; arg; arg = va_arg (ap, const char *))

arg != NULL in predicate.

> +    last = arg;
> +  va_end (ap);
> +
> +  va_start (ap, first);
> +  for (arg = first; arg; arg = va_arg (ap, const char *))

likewise.

> +    {
> +      if (arg == last)
> +	tmp = xstrdup (arg);
> +      else
> +	{
> +	  int len;
> +
> +	  /* Strip leading separators from subdirectories.  */
> +	  if (arg != first)
> +	    {
> +	      while (*arg != '\0' && IS_DIR_SEPARATOR (*arg))
> +		arg++;
> +	    }
> +
> +	  /* Strip trailing separators.  */
> +	  len = strlen (arg);
> +
> +	  while (len > 0 && IS_DIR_SEPARATOR (arg[len - 1]))
> +	    len--;
> +
> +	  if (len > 0)
> +	    {
> +	      tmp = xmalloc (len + strlen (SLASH_STRING) + 1);
> +	      memcpy (tmp, arg, len);
> +	      strcpy (tmp + len, SLASH_STRING);
> +	    }
> +	  else
> +	    tmp = NULL;
> +	}
> +
> +      if (tmp != NULL)
> +	{
> +	  VEC_safe_push (char_ptr, args, tmp);
> +	  bufsiz += strlen (tmp);

Why build the temporary VEC instead of just incrementally
building the final buf?

I think you could simplify this much if you did that,
and plus use reconcat.

> +	}
> +    }
> +  va_end (ap);
> +
> +  bufsiz += 1;  /* Terminator.  */
> +
> +  buf = xmalloc (bufsiz);
> +  buf[0] = '\0';
> +  for (i = 0; VEC_iterate (char_ptr, args, i, tmp); i++)
> +    strcat (buf, tmp);
> +  gdb_assert (bufsiz == strlen (buf) + 1);


Thanks,
Pedro Alves
Gary Benson July 2, 2015, 11:18 a.m. UTC | #8 
Pedro Alves wrote:
> On 06/16/2015 10:42 AM, Gary Benson wrote:
> > This commit introduces a new function build_debug_file_name which
> > concatenates a series of filename components into a filename.
> > find_separate_debug_file is updated to use build_debug_file_name.
> > A later commit in this series will extend build_debug_file_name to
> > correctly handle "target:" prefixes, so it is convenient to have
> > filename building pulled out into one function.  For now the only
> > functional change here is that the original code sometimes
> > generated filenames with repeated directory separators while the
> > new code does not.
> 
> I'd drop the "debug" from the function's name.  Sounds like a
> candidate for reuse elsewhere to me.

Should I put it somewhere else, maybe common-utils.c?

Cheers,
Gary
Pedro Alves July 2, 2015, 11:38 a.m. UTC | #9 
On 07/02/2015 12:18 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 06/16/2015 10:42 AM, Gary Benson wrote:
>>> This commit introduces a new function build_debug_file_name which
>>> concatenates a series of filename components into a filename.
>>> find_separate_debug_file is updated to use build_debug_file_name.
>>> A later commit in this series will extend build_debug_file_name to
>>> correctly handle "target:" prefixes, so it is convenient to have
>>> filename building pulled out into one function.  For now the only
>>> functional change here is that the original code sometimes
>>> generated filenames with repeated directory separators while the
>>> new code does not.
>>
>> I'd drop the "debug" from the function's name.  Sounds like a
>> candidate for reuse elsewhere to me.
> 
> Should I put it somewhere else, maybe common-utils.c?

I dislike common-utils.c for "kitchen-sink" reasons, though.

There's common/filestuff.c, but that looks more for low/OS level
file things.

There's substitute_path_component (and gdb_realpath / gdb_abspath)
in utils.c.  Maybe put it next to substitute_path_component, which
seems to be in the same "family" of function, and then (at some point)
we would move all file name/path manipulation routines to its own file.

WDYT?

Thanks,
Pedro Alves
Gary Benson July 2, 2015, 1:53 p.m. UTC | #10 
Pedro Alves wrote:
> On 07/02/2015 12:18 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > On 06/16/2015 10:42 AM, Gary Benson wrote:
> > > > This commit introduces a new function build_debug_file_name
> > > > which concatenates a series of filename components into a
> > > > filename.  find_separate_debug_file is updated to use
> > > > build_debug_file_name.  A later commit in this series will
> > > > extend build_debug_file_name to correctly handle "target:"
> > > > prefixes, so it is convenient to have filename building pulled
> > > > out into one function.  For now the only functional change
> > > > here is that the original code sometimes generated filenames
> > > > with repeated directory separators while the new code does
> > > > not.
> > >
> > > I'd drop the "debug" from the function's name.  Sounds like a
> > > candidate for reuse elsewhere to me.
> > 
> > Should I put it somewhere else, maybe common-utils.c?
> 
> I dislike common-utils.c for "kitchen-sink" reasons, though.
> 
> There's common/filestuff.c, but that looks more for low/OS level
> file things.

Yeah, that was my first thought.

> There's substitute_path_component (and gdb_realpath / gdb_abspath)
> in utils.c.  Maybe put it next to substitute_path_component, which
> seems to be in the same "family" of function, and then (at some
> point) we would move all file name/path manipulation routines to its
> own file.
> 
> WDYT?

Suits me :)

Cheers,
Gary
Jan Kratochvil July 25, 2015, 4:19 p.m. UTC | #11 
On Tue, 16 Jun 2015 11:42:44 +0200, Gary Benson wrote:
> +/* Build the filename of a separate debug file from an arbitrary
> +   number of components.  Returns an xmalloc'd string that must
> +   be be freed by the caller.  The final argument of this function
> +   must be NULL to mark the end the argument list.  */

I would describe also:
  Initial slash is left in place.  Additionally a slash is appended at the
  end.


Jan
diff mbox

Patch

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 0c35ffa..799133a 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1431,6 +1431,79 @@  separate_debug_file_exists (const char *name, unsigned long crc,
   return 1;
 }
 
+/* Build the filename of a separate debug file from an arbitrary
+   number of components.  Returns an xmalloc'd string that must
+   be be freed by the caller.  The final argument of this function
+   must be NULL to mark the end the argument list.  */
+
+static char *
+build_debug_file_name (const char *first, ...)
+{
+  va_list ap;
+  const char *arg, *last;
+  VEC (char_ptr) *args = NULL;
+  struct cleanup *back_to = make_cleanup_free_char_ptr_vec (args);
+  int bufsiz = 0;
+  char *buf, *tmp;
+  int i;
+
+  va_start (ap, first);
+  for (arg = first; arg; arg = va_arg (ap, const char *))
+    last = arg;
+  va_end (ap);
+
+  va_start (ap, first);
+  for (arg = first; arg; arg = va_arg (ap, const char *))
+    {
+      if (arg == last)
+	tmp = xstrdup (arg);
+      else
+	{
+	  int len;
+
+	  /* Strip leading separators from subdirectories.  */
+	  if (arg != first)
+	    {
+	      while (*arg != '\0' && IS_DIR_SEPARATOR (*arg))
+		arg++;
+	    }
+
+	  /* Strip trailing separators.  */
+	  len = strlen (arg);
+
+	  while (len > 0 && IS_DIR_SEPARATOR (arg[len - 1]))
+	    len--;
+
+	  if (len > 0)
+	    {
+	      tmp = xmalloc (len + strlen (SLASH_STRING) + 1);
+	      memcpy (tmp, arg, len);
+	      strcpy (tmp + len, SLASH_STRING);
+	    }
+	  else
+	    tmp = NULL;
+	}
+
+      if (tmp != NULL)
+	{
+	  VEC_safe_push (char_ptr, args, tmp);
+	  bufsiz += strlen (tmp);
+	}
+    }
+  va_end (ap);
+
+  bufsiz += 1;  /* Terminator.  */
+
+  buf = xmalloc (bufsiz);
+  buf[0] = '\0';
+  for (i = 0; VEC_iterate (char_ptr, args, i, tmp); i++)
+    strcat (buf, tmp);
+  gdb_assert (bufsiz == strlen (buf) + 1);
+
+  do_cleanups (back_to);
+  return buf;
+}
+
 char *debug_file_directory = NULL;
 static void
 show_debug_file_directory (struct ui_file *file, int from_tty,
@@ -1461,38 +1534,22 @@  find_separate_debug_file (const char *dir,
 {
   char *debugdir;
   char *debugfile;
-  int i;
   VEC (char_ptr) *debugdir_vec;
   struct cleanup *back_to;
   int ix;
 
-  /* Set I to max (strlen (canon_dir), strlen (dir)).  */
-  i = strlen (dir);
-  if (canon_dir != NULL && strlen (canon_dir) > i)
-    i = strlen (canon_dir);
-
-  debugfile = xmalloc (strlen (debug_file_directory) + 1
-		       + i
-		       + strlen (DEBUG_SUBDIRECTORY)
-		       + strlen ("/")
-		       + strlen (debuglink)
-		       + 1);
-
   /* First try in the same directory as the original file.  */
-  strcpy (debugfile, dir);
-  strcat (debugfile, debuglink);
-
+  debugfile = build_debug_file_name (dir, debuglink, NULL);
   if (separate_debug_file_exists (debugfile, crc32, objfile))
     return debugfile;
+  xfree (debugfile);
 
   /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
-  strcpy (debugfile, dir);
-  strcat (debugfile, DEBUG_SUBDIRECTORY);
-  strcat (debugfile, "/");
-  strcat (debugfile, debuglink);
-
+  debugfile = build_debug_file_name (dir, DEBUG_SUBDIRECTORY,
+				     debuglink, NULL);
   if (separate_debug_file_exists (debugfile, crc32, objfile))
     return debugfile;
+  xfree (debugfile);
 
   /* Then try in the global debugfile directories.
 
@@ -1504,16 +1561,14 @@  find_separate_debug_file (const char *dir,
 
   for (ix = 0; VEC_iterate (char_ptr, debugdir_vec, ix, debugdir); ++ix)
     {
-      strcpy (debugfile, debugdir);
-      strcat (debugfile, "/");
-      strcat (debugfile, dir);
-      strcat (debugfile, debuglink);
-
+      debugfile = build_debug_file_name (debugdir, dir, debuglink,
+					 NULL);
       if (separate_debug_file_exists (debugfile, crc32, objfile))
 	{
 	  do_cleanups (back_to);
 	  return debugfile;
 	}
+      xfree (debugfile);
 
       /* If the file is in the sysroot, try using its base path in the
 	 global debugfile directory.  */
@@ -1522,21 +1577,19 @@  find_separate_debug_file (const char *dir,
 			    strlen (gdb_sysroot)) == 0
 	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
 	{
-	  strcpy (debugfile, debugdir);
-	  strcat (debugfile, canon_dir + strlen (gdb_sysroot));
-	  strcat (debugfile, "/");
-	  strcat (debugfile, debuglink);
-
+	  debugfile = build_debug_file_name (debugdir, canon_dir
+					     + strlen (gdb_sysroot),
+					     debuglink, NULL);
 	  if (separate_debug_file_exists (debugfile, crc32, objfile))
 	    {
 	      do_cleanups (back_to);
 	      return debugfile;
 	    }
+	  xfree (debugfile);
 	}
     }
 
   do_cleanups (back_to);
-  xfree (debugfile);
   return NULL;
 }