From patchwork Wed Mar 22 19:26:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 19704 Received: (qmail 69498 invoked by alias); 22 Mar 2017 19:26:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 69484 invoked by uid 89); 22 Mar 2017 19:26:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f176.google.com Received: from mail-wr0-f176.google.com (HELO mail-wr0-f176.google.com) (209.85.128.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Mar 2017 19:26:44 +0000 Received: by mail-wr0-f176.google.com with SMTP id l37so134925228wrc.1 for ; Wed, 22 Mar 2017 12:26:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=bkxBo/iTvkU2qaHACEIziZ3Me6NcwX/KLkus85XJTs4=; b=Xq6C8vTicd+k4xy0ju0i/sgUnpjUsDEQw8/sxavY95+skTW4ZyFOr7a1uKWGqE2jCE V9Bl3LYp0X2bVrnG5dzXB2lRx6/X1lDYdeBAizOrTajZjCvLlVD32zRZc2rlKN3x1OqJ emL77Quz2/mJX7Wbk3Nffe2QzKM+HY2rpppL12IIat3kZDFO0wR4JFGw3P3drdv4p1u/ 3XksZy3Q+/gTV5tuqHhhjRzosiq4mba1H2g8qtOU/aw6wlzkmBuMosPULrd9a8uQP6Yx IDvOTtnPGm/soxfEPNAh5DRUKQWgHruRv+26/wRjxwkDvCI9HS+HgZ6lwB46pHwS6jMy 6oCg== X-Gm-Message-State: AFeK/H1Mr+4hHlqiRvOsYJb9xbL1KkHG6/3CrYBxOQjcKQq232/vE/TKvOtiRKgfMd8GjFJp X-Received: by 10.223.164.150 with SMTP id g22mr40917074wrb.92.1490210803111; Wed, 22 Mar 2017 12:26:43 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id o22sm2929449wro.13.2017.03.22.12.26.41 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 22 Mar 2017 12:26:42 -0700 (PDT) Subject: [PATCH v2] gdb: Make ldirname return a std::string To: Philipp Rudo References: <1490198646-10469-1-git-send-email-palves@redhat.com> <20170322190135.2d634571@ThinkPad> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <639eca0b-2b06-4b27-1bdb-52c2f432d25e@redhat.com> Date: Wed, 22 Mar 2017 19:26:40 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170322190135.2d634571@ThinkPad> 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. > 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. I shouldn't point res.comp_dir to the storage if ldirname returned an empty string though. Fixed now too. From 98a4e4d213eb85b264c50bcae7664f5fb3cd29d7 Mon Sep 17 00:00:00 2001 From: Pedro Alves 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 , 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 * 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 .: 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 = ""; + if (res.name == NULL) + res.name = ""; + + 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;