This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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;