This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] gdb: Make ldirname return a std::string


On Wed, 22 Mar 2017 19:26:40 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 03/22/2017 06:01 PM, Philipp Rudo wrote:
> 
> >> -  if (*comp_dir == NULL
> >> -      && producer_is_gcc_lt_4_3 (cu) && *name != NULL
> >> -      && IS_ABSOLUTE_PATH (*name))
> >> +  if (res.comp_dir == NULL
> >> +      && producer_is_gcc_lt_4_3 (cu) && res.name != NULL
> >> +      && IS_ABSOLUTE_PATH (res.name))
> >>      {
> >> -      char *d = ldirname (*name);
> >> -
> >> -      *comp_dir = d;
> >> -      if (d != NULL)
> >> -	make_cleanup (xfree, d);
> >> +      res.comp_dir_storage = ldirname (res.name);
> >> +      res.comp_dir = res.comp_dir_storage.c_str ();
> >>      }
> >> -  if (*comp_dir != NULL)
> >> +  if (res.comp_dir != NULL)  
> > 
> > Is this check valid?  Doesn't std::string.c_str () always return a
> > pointer != NULL?  
> 
> Yes, std::string.c_str () always return a non-null pointer, but
> if the condition in the if above:
> 
>   if (res.comp_dir == NULL
>       && producer_is_gcc_lt_4_3 (cu) && res.name != NULL
>       && IS_ABSOLUTE_PATH (res.name))
> 
> is false, and further above, this:
> 
>   res.comp_dir = dwarf2_string_attr (die, DW_AT_comp_dir, cu);
> 
> ... had left res.comp_dir NULL too, then we get here with
> res.comp_dir still NULL.

You are right.
 
> > I would check for res.comp_dir_storage.empty ().  
> 
> That won't work.  The "comp_dir_storage" field is there
> for the case when we needed to call ldirname.
> That's what replaces the make_cleanup call.  I.e., before, the memory
> for the result of that ldirname call was released whenever
> something called do_cleanups afterwards.  While after this
> patch, that memory is owned by file_and_directory, via the
> comp_dir_storage field.  But that storage is _not_ used if the
> 
>   res.comp_dir = dwarf2_string_attr (die, DW_AT_comp_dir, cu);
> 
> line above managed to find a comp_dir attribute.  In that
> case, res.comp_dir points to some memory owned by the obstack
> that owns DIE/CU, and the storage field is left empty.  I see I
> forgot to add some comments.  Fixed in this update below.

Here too.
 
> I shouldn't point res.comp_dir to the storage if ldirname returned an
> empty string though.  Fixed now too.

V2 looks good to me.

Philipp
 
> From 98a4e4d213eb85b264c50bcae7664f5fb3cd29d7 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 22 Mar 2017 15:21:29 +0000
> Subject: [PATCH v2] gdb: Make ldirname return a std::string
> 
> Eliminates several uses of cleanups.
> 
> (And if applied before
> <https://sourceware.org/ml/gdb-patches/2017-03/msg00394.html>, fixes
> the leak in python.c:do_start_initialization too.)
> 
> Tested on x86_64 Fedora 23 with Python 2 and 3.
> 
> gdb/ChangeLog
> 2017-03-22  Pedro Alves  <palves@redhat.com>
> 
> 	* dwarf2read.c (struct file_and_directory): New.
> 	(dwarf2_get_dwz_file): Adjust to use std::string.
> 	(dw2_get_file_names_reader): Adjust to use file_and_directory.
> 	(find_file_and_directory): Adjust to return a
> file_and_directory object.
> 	(read_file_scope): Adjust to use file_and_directory.  Remove
> 	make_cleanup/do_cleanups calls.
> 	(open_and_init_dwp_file): Adjust to use std::string.  Remove
> 	make_cleanup/do_cleanups calls.
> 	* python/python.c (do_start_initialization): Adjust to
> ldirname returning a std::string.
> 	* utils.c (ldirname): Now returns a std::string.
> 	* utils.h (ldirname): Change return type to std::string.
> 	* xml-syscall.c (xml_init_syscalls_info): Adjust to ldirname
> 	returning a std::string.
> 	* xml-tdesc.c (file_read_description_xml): Likewise.
> ---
>  gdb/dwarf2read.c    | 117
> +++++++++++++++++++++++++++-------------------------
> gdb/python/python.c |   2 +- gdb/utils.c         |  10 ++---
>  gdb/utils.h         |   2 +-
>  gdb/xml-syscall.c   |   8 +---
>  gdb/xml-tdesc.c     |   8 +---
>  6 files changed, 71 insertions(+), 76 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index b3ea52b..36b6ead 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1885,9 +1885,27 @@ static void queue_comp_unit (struct
> dwarf2_per_cu_data *per_cu,
> 
>  static void process_queue (void);
> 
> -static void find_file_and_directory (struct die_info *die,
> -				     struct dwarf2_cu *cu,
> -				     const char **name, const char
> **comp_dir); +/* The return type of find_file_and_directory.  Note,
> the enclosed
> +   string pointers are only valid while this object is valid.  */
> +
> +struct file_and_directory
> +{
> +  /* The filename.  This is never NULL.  */
> +  const char *name;
> +
> +  /* The compilation directory.  NULL if not known.  If we needed to
> +     compute a new string, this points to COMP_DIR_STORAGE,
> otherwise,
> +     points directly to the DW_AT_comp_dir string attribute owned by
> +     the obstack that owns the DIE.  */
> +  const char *comp_dir;
> +
> +  /* If we needed to build a new string for comp_dir, this is what
> +     owns the storage.  */
> +  std::string comp_dir_storage;
> +};
> +
> +static file_and_directory find_file_and_directory (struct die_info
> *die,
> +						   struct dwarf2_cu
> *cu);
> 
>  static char *file_full_name (int file, struct line_header *lh,
>  			     const char *comp_dir);
> @@ -2552,18 +2570,15 @@ dwarf2_get_dwz_file (void)
>    buildid_len = (size_t) buildid_len_arg;
> 
>    filename = (const char *) data;
> +
> +  std::string abs_storage;
>    if (!IS_ABSOLUTE_PATH (filename))
>      {
>        char *abs = gdb_realpath (objfile_name
> (dwarf2_per_objfile->objfile));
> -      char *rel;
> 
>        make_cleanup (xfree, abs);
> -      abs = ldirname (abs);
> -      make_cleanup (xfree, abs);
> -
> -      rel = concat (abs, SLASH_STRING, filename, (char *) NULL);
> -      make_cleanup (xfree, rel);
> -      filename = rel;
> +      abs_storage = ldirname (abs) + SLASH_STRING + filename;
> +      filename = abs_storage.c_str ();
>      }
> 
>    /* First try the file name given in the section.  If that doesn't
> @@ -3332,7 +3347,6 @@ dw2_get_file_names_reader (const struct
> die_reader_specs *reader, struct line_header *lh;
>    struct attribute *attr;
>    int i;
> -  const char *name, *comp_dir;
>    void **slot;
>    struct quick_file_names *qfn;
>    unsigned int line_offset;
> @@ -3385,13 +3399,13 @@ dw2_get_file_names_reader (const struct
> die_reader_specs *reader, gdb_assert (slot != NULL);
>    *slot = qfn;
> 
> -  find_file_and_directory (comp_unit_die, cu, &name, &comp_dir);
> +  file_and_directory fnd = find_file_and_directory (comp_unit_die,
> cu);
> 
>    qfn->num_file_names = lh->num_file_names;
>    qfn->file_names =
>      XOBNEWVEC (&objfile->objfile_obstack, const char *,
> lh->num_file_names); for (i = 0; i < lh->num_file_names; ++i)
> -    qfn->file_names[i] = file_full_name (i + 1, lh, comp_dir);
> +    qfn->file_names[i] = file_full_name (i + 1, lh, fnd.comp_dir);
>    qfn->real_names = NULL;
> 
>    free_line_header (lh);
> @@ -9122,37 +9136,38 @@ producer_is_gcc_lt_4_3 (struct dwarf2_cu *cu)
>    return cu->producer_is_gcc_lt_4_3;
>  }
> 
> -static void
> -find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu,
> -			 const char **name, const char **comp_dir)
> +static file_and_directory
> +find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu)
>  {
> +  file_and_directory res;
> +
>    /* Find the filename.  Do not use dwarf2_name here, since the
> filename is not a source language identifier.  */
> -  *name = dwarf2_string_attr (die, DW_AT_name, cu);
> -  *comp_dir = dwarf2_string_attr (die, DW_AT_comp_dir, cu);
> +  res.name = dwarf2_string_attr (die, DW_AT_name, cu);
> +  res.comp_dir = dwarf2_string_attr (die, DW_AT_comp_dir, cu);
> 
> -  if (*comp_dir == NULL
> -      && producer_is_gcc_lt_4_3 (cu) && *name != NULL
> -      && IS_ABSOLUTE_PATH (*name))
> +  if (res.comp_dir == NULL
> +      && producer_is_gcc_lt_4_3 (cu) && res.name != NULL
> +      && IS_ABSOLUTE_PATH (res.name))
>      {
> -      char *d = ldirname (*name);
> -
> -      *comp_dir = d;
> -      if (d != NULL)
> -	make_cleanup (xfree, d);
> +      res.comp_dir_storage = ldirname (res.name);
> +      if (!res.comp_dir_storage.empty ())
> +	res.comp_dir = res.comp_dir_storage.c_str ();
>      }
> -  if (*comp_dir != NULL)
> +  if (res.comp_dir != NULL)
>      {
>        /* Irix 6.2 native cc prepends <machine>.: to the compilation
>  	 directory, get rid of it.  */
> -      const char *cp = strchr (*comp_dir, ':');
> +      const char *cp = strchr (res.comp_dir, ':');
> 
> -      if (cp && cp != *comp_dir && cp[-1] == '.' && cp[1] == '/')
> -	*comp_dir = cp + 1;
> +      if (cp && cp != res.comp_dir && cp[-1] == '.' && cp[1] == '/')
> +	res.comp_dir = cp + 1;
>      }
> 
> -  if (*name == NULL)
> -    *name = "<unknown>";
> +  if (res.name == NULL)
> +    res.name = "<unknown>";
> +
> +  return res;
>  }
> 
>  /* Handle DW_AT_stmt_list for a compilation unit.
> @@ -9262,12 +9277,9 @@ read_file_scope (struct die_info *die, struct
> dwarf2_cu *cu) {
>    struct objfile *objfile = dwarf2_per_objfile->objfile;
>    struct gdbarch *gdbarch = get_objfile_arch (objfile);
> -  struct cleanup *back_to = make_cleanup (null_cleanup, 0);
>    CORE_ADDR lowpc = ((CORE_ADDR) -1);
>    CORE_ADDR highpc = ((CORE_ADDR) 0);
>    struct attribute *attr;
> -  const char *name = NULL;
> -  const char *comp_dir = NULL;
>    struct die_info *child_die;
>    CORE_ADDR baseaddr;
> 
> @@ -9281,7 +9293,7 @@ read_file_scope (struct die_info *die, struct
> dwarf2_cu *cu) lowpc = highpc;
>    lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
> 
> -  find_file_and_directory (die, cu, &name, &comp_dir);
> +  file_and_directory fnd = find_file_and_directory (die, cu);
> 
>    prepare_one_comp_unit (cu, die, cu->language);
> 
> @@ -9295,12 +9307,12 @@ read_file_scope (struct die_info *die, struct
> dwarf2_cu *cu) if (cu->producer && strstr (cu->producer, "GNU Go
> ") != NULL) set_cu_language (DW_LANG_Go, cu);
> 
> -  dwarf2_start_symtab (cu, name, comp_dir, lowpc);
> +  dwarf2_start_symtab (cu, fnd.name, fnd.comp_dir, lowpc);
> 
>    /* Decode line number information if present.  We do this before
>       processing child DIEs, so that the line header table is
> available for DW_AT_decl_file.  */
> -  handle_DW_AT_stmt_list (die, cu, comp_dir, lowpc);
> +  handle_DW_AT_stmt_list (die, cu, fnd.comp_dir, lowpc);
> 
>    /* Process all dies in compilation unit.  */
>    if (die->child != NULL)
> @@ -9338,8 +9350,6 @@ read_file_scope (struct die_info *die, struct
> dwarf2_cu *cu) dwarf_decode_macros (cu, macro_offset, 0);
>  	}
>      }
> -
> -  do_cleanups (back_to);
>  }
> 
>  /* TU version of handle_DW_AT_stmt_list for read_type_unit_scope.
> @@ -10892,49 +10902,44 @@ open_and_init_dwp_file (void)
>  {
>    struct objfile *objfile = dwarf2_per_objfile->objfile;
>    struct dwp_file *dwp_file;
> -  char *dwp_name;
> -  struct cleanup *cleanups = make_cleanup (null_cleanup, 0);
> 
>    /* Try to find first .dwp for the binary file before any symbolic
> links resolving.  */
> 
>    /* If the objfile is a debug file, find the name of the real binary
>       file and get the name of dwp file from there.  */
> +  std::string dwp_name;
>    if (objfile->separate_debug_objfile_backlink != NULL)
>      {
>        struct objfile *backlink =
> objfile->separate_debug_objfile_backlink; const char
> *backlink_basename = lbasename (backlink->original_name);
> -      char *debug_dirname = ldirname (objfile->original_name);
> 
> -      make_cleanup (xfree, debug_dirname);
> -      dwp_name = xstrprintf ("%s%s%s.dwp", debug_dirname,
> -			     SLASH_STRING, backlink_basename);
> +      dwp_name = ldirname (objfile->original_name) + SLASH_STRING +
> backlink_basename; }
>    else
> -    dwp_name = xstrprintf ("%s.dwp", objfile->original_name);
> -  make_cleanup (xfree, dwp_name);
> +    dwp_name = objfile->original_name;
> +
> +  dwp_name += ".dwp";
> 
> -  gdb_bfd_ref_ptr dbfd (open_dwp_file (dwp_name));
> +  gdb_bfd_ref_ptr dbfd (open_dwp_file (dwp_name.c_str ()));
>    if (dbfd == NULL
>        && strcmp (objfile->original_name, objfile_name (objfile)) !=
> 0) {
>        /* Try to find .dwp for the binary file after gdb_realpath
> resolving.  */
> -      dwp_name = xstrprintf ("%s.dwp", objfile_name (objfile));
> -      make_cleanup (xfree, dwp_name);
> -      dbfd = open_dwp_file (dwp_name);
> +      dwp_name = objfile_name (objfile);
> +      dwp_name += ".dwp";
> +      dbfd = open_dwp_file (dwp_name.c_str ());
>      }
> 
>    if (dbfd == NULL)
>      {
>        if (dwarf_read_debug)
> -	fprintf_unfiltered (gdb_stdlog, "DWP file not found: %s\n",
> dwp_name);
> -      do_cleanups (cleanups);
> +	fprintf_unfiltered (gdb_stdlog, "DWP file not found: %s\n",
> dwp_name.c_str ()); return NULL;
>      }
>    dwp_file = OBSTACK_ZALLOC (&objfile->objfile_obstack, struct
> dwp_file); dwp_file->name = bfd_get_filename (dbfd.get ());
>    dwp_file->dbfd = dbfd.release ();
> -  do_cleanups (cleanups);
> 
>    /* +1: section 0 is unused */
>    dwp_file->num_sections = bfd_count_sections (dwp_file->dbfd) + 1;
> @@ -10958,7 +10963,7 @@ open_and_init_dwp_file (void)
>        error (_("Dwarf Error: DWP file CU version %s doesn't match"
>  	       " TU version %s [in DWP file %s]"),
>  	     pulongest (dwp_file->cus->version),
> -	     pulongest (dwp_file->tus->version), dwp_name);
> +	     pulongest (dwp_file->tus->version), dwp_name.c_str ());
>      }
>    dwp_file->version = dwp_file->cus->version;
> 
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 73fb3d0..a7aff53 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1550,7 +1550,7 @@ do_start_initialization ()
>       /foo/bin/python
>       /foo/lib/pythonX.Y/...
>       This must be done before calling Py_Initialize.  */
> -  progname = concat (ldirname (python_libdir), SLASH_STRING, "bin",
> +  progname = concat (ldirname (python_libdir).c_str (),
> SLASH_STRING, "bin", SLASH_STRING, "python", (char *) NULL);
>  #ifdef IS_PY3K
>    oldloc = xstrdup (setlocale (LC_ALL, NULL));
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 27021a1..39798cc 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -2943,20 +2943,19 @@ dummy_obstack_deallocate (void *object, void
> *data) /* Simple, portable version of dirname that does not modify its
>     argument.  */
> 
> -char *
> +std::string
>  ldirname (const char *filename)
>  {
> +  std::string dirname;
>    const char *base = lbasename (filename);
> -  char *dirname;
> 
>    while (base > filename && IS_DIR_SEPARATOR (base[-1]))
>      --base;
> 
>    if (base == filename)
> -    return NULL;
> +    return dirname;
> 
> -  dirname = (char *) xmalloc (base - filename + 2);
> -  memcpy (dirname, filename, base - filename);
> +  dirname = std::string (filename, base - filename);
> 
>    /* On DOS based file systems, convert "d:foo" to "d:.", so that we
>       create "d:./bar" later instead of the (different) "d:/bar".  */
> @@ -2964,7 +2963,6 @@ ldirname (const char *filename)
>        && !IS_DIR_SEPARATOR (filename[0]))
>      dirname[base++ - filename] = '.';
> 
> -  dirname[base - filename] = '\0';
>    return dirname;
>  }
> 
> diff --git a/gdb/utils.h b/gdb/utils.h
> index f138702..fb75f2e 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -135,7 +135,7 @@ extern int gdb_filename_fnmatch (const char
> *pattern, const char *string, extern void substitute_path_component
> (char **stringp, const char *from, const char *to);
> 
> -char *ldirname (const char *filename);
> +std::string ldirname (const char *filename);
> 
>  extern int count_path_elements (const char *path);
> 
> diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
> index 1e42b8d..a436418 100644
> --- a/gdb/xml-syscall.c
> +++ b/gdb/xml-syscall.c
> @@ -363,7 +363,6 @@ static struct syscalls_info *
>  xml_init_syscalls_info (const char *filename)
>  {
>    char *full_file;
> -  char *dirname;
>    struct syscalls_info *syscalls_info;
>    struct cleanup *back_to;
> 
> @@ -373,12 +372,9 @@ xml_init_syscalls_info (const char *filename)
> 
>    back_to = make_cleanup (xfree, full_file);
> 
> -  dirname = ldirname (filename);
> -  if (dirname != NULL)
> -    make_cleanup (xfree, dirname);
> -
>    syscalls_info = syscall_parse_xml (full_file,
> -				     xml_fetch_content_from_file,
> dirname);
> +				     xml_fetch_content_from_file,
> +				     (void *) ldirname
> (filename).c_str ()); do_cleanups (back_to);
> 
>    return syscalls_info;
> diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
> index 1677659..effe652 100644
> --- a/gdb/xml-tdesc.c
> +++ b/gdb/xml-tdesc.c
> @@ -694,7 +694,6 @@ file_read_description_xml (const char *filename)
>    struct target_desc *tdesc;
>    char *tdesc_str;
>    struct cleanup *back_to;
> -  char *dirname;
> 
>    tdesc_str = xml_fetch_content_from_file (filename, NULL);
>    if (tdesc_str == NULL)
> @@ -705,11 +704,8 @@ file_read_description_xml (const char *filename)
> 
>    back_to = make_cleanup (xfree, tdesc_str);
> 
> -  dirname = ldirname (filename);
> -  if (dirname != NULL)
> -    make_cleanup (xfree, dirname);
> -
> -  tdesc = tdesc_parse_xml (tdesc_str, xml_fetch_content_from_file,
> dirname);
> +  tdesc = tdesc_parse_xml (tdesc_str, xml_fetch_content_from_file,
> +			   (void *) ldirname (filename).c_str ());
>    do_cleanups (back_to);
> 
>    return tdesc;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]