From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 39173 invoked by alias); 23 Mar 2017 10:04:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 39153 invoked by uid 89); 23 Mar 2017 10:04:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 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_LOW autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Mar 2017 10:04:49 +0000 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v2NA47GL084084 for ; Thu, 23 Mar 2017 06:04:49 -0400 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0a-001b2d01.pphosted.com with ESMTP id 29bv4jkvha-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 23 Mar 2017 06:04:48 -0400 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 23 Mar 2017 10:04:45 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (9.149.109.198) by e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 23 Mar 2017 10:04:43 -0000 Received: from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com [9.149.105.232]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v2NA4h7X37027854; Thu, 23 Mar 2017 10:04:43 GMT Received: from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9D7B952041; Thu, 23 Mar 2017 09:03:05 +0000 (GMT) Received: from ThinkPad (unknown [9.152.212.148]) by d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTP id 8671052049; Thu, 23 Mar 2017 09:03:05 +0000 (GMT) Date: Thu, 23 Mar 2017 10:04:00 -0000 From: Philipp Rudo To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdb: Make ldirname return a std::string In-Reply-To: <639eca0b-2b06-4b27-1bdb-52c2f432d25e@redhat.com> References: <1490198646-10469-1-git-send-email-palves@redhat.com> <20170322190135.2d634571@ThinkPad> <639eca0b-2b06-4b27-1bdb-52c2f432d25e@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17032310-0016-0000-0000-00000461F748 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17032310-0017-0000-0000-000026EE5FCB Message-Id: <20170323110442.4632e8d8@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-23_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703230093 X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00411.txt.bz2 On Wed, 22 Mar 2017 19:26:40 +0000 Pedro Alves 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 > 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;