* [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions @ 2018-04-25 19:40 Simon Marchi 2018-04-25 19:40 ` [PATCH 2/2] Use XOBNEW when possible Simon Marchi 2018-04-25 22:36 ` [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Tom Tromey 0 siblings, 2 replies; 7+ messages in thread From: Simon Marchi @ 2018-04-25 19:40 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Since we use obstacks with objects that are not default constructible, we sometimes need to manually call the constructor by hand using placement new: foo *f = obstack_alloc (obstack, sizeof (foo)); f = new (f) foo; This patch introduces a utility to make this pattern simpler: foo *f = obstack_new<foo> (); To help catch places where we would forget to call new when allocating such an object on an obstack, this patch also poisons some other methods of allocating an instance of a type on an obstack: - OBSTACK_ZALLOC/OBSTACK_CALLOC - XOBNEW/XOBNEW - GDBARCH_OBSTACK_ZALLOC/GDBARCH_OBSTACK_CALLOC Unfortunately, there's no way to catch wrong usages of obstack_alloc. By pulling on that string though, it tripped on allocating struct template_symbol using OBSTACK_ZALLOC. The criterion currently used to know whether it's safe to "malloc" an instance of a struct is whether it is a POD. Because it inherits from struct symbol, template_symbol is not a POD. This criterion is a bit too strict however, it should still safe to allocate memory for a template_symbol and memset it to 0. We didn't use is_trivially_constructible as the criterion in the first place only because it is not available in gcc < 5. So here I considered two alternatives: 1. Relax that criterion to use std::is_trivially_constructible and add a bit more glue code to make it work with gcc < 5 2. Continue pulling on the string and change how the symbol structures are allocated and initialized I managed to do both, but I decided to go with #1 to keep this patch simpler and more focused. When building with a compiler that does not have is_trivially_constructible, the check will just not be enforced. gdb/ChangeLog: * common/traits.h (HAVE_IS_TRIVIALLY_COPYABLE): Define if compiler supports std::is_trivially_constructible. * common/poison.h: Include obstack.h. (IsMallocable): Define to is_trivially_constructible if the compiler supports it, define to true_type otherwise. (xobnew): New. (XOBNEW): Redefine. (xobnewvec): New. (XOBNEWVEC): Redefine. * gdb_obstack.h (obstack_zalloc): New. (OBSTACK_ZALLOC): Redefine. (obstack_calloc): New. (OBSTACK_CALLOC): Redefine. (obstack_new): New. * dwarf2read.c (dwarf2_read_index): Use obstack_new. * gdbarch.sh: Include gdb_obstack in gdbarch.h. (gdbarch_obstack): New declaration in gdbarch.h, definition in gdbarch.c. (GDBARCH_OBSTACK_CALLOC, GDBARCH_OBSTACK_ZALLOC): Use obstack_calloc/obstack_zalloc. (gdbarch_obstack_zalloc): Remove. * target-descriptions.c (tdesc_data_init): Use obstack_new. --- gdb/common/poison.h | 31 ++++++++++++++++++++++++++++++- gdb/common/traits.h | 8 ++++++++ gdb/dwarf2read.c | 3 +-- gdb/gdb_obstack.h | 36 ++++++++++++++++++++++++++++++++---- gdb/gdbarch.c | 9 ++------- gdb/gdbarch.h | 10 +++++++--- gdb/gdbarch.sh | 21 +++++++++++---------- gdb/target-descriptions.c | 7 +------ 8 files changed, 92 insertions(+), 33 deletions(-) diff --git a/gdb/common/poison.h b/gdb/common/poison.h index c98d2b3..ddab2c1 100644 --- a/gdb/common/poison.h +++ b/gdb/common/poison.h @@ -21,6 +21,7 @@ #define COMMON_POISON_H #include "traits.h" +#include "obstack.h" /* Poison memset of non-POD types. The idea is catching invalid initialization of non-POD structs that is easy to be introduced as @@ -88,7 +89,11 @@ void *memmove (D *dest, const S *src, size_t n) = delete; objects that require new/delete. */ template<typename T> -using IsMallocable = std::is_pod<T>; +#if HAVE_IS_TRIVIALLY_CONSTRUCTIBLE +using IsMallocable = std::is_trivially_constructible<T>; +#else +using IsMallocable = std::true_type; +#endif template<typename T> using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>; @@ -216,4 +221,28 @@ non-POD data type."); #undef XRESIZEVAR #define XRESIZEVAR(T, P, S) xresizevar<T> (P, S) +template<typename T> +static T * +xobnew (obstack *ob) +{ + static_assert (IsMallocable<T>::value, "Trying to use XOBNEW with a \ +non-POD data type."); + return XOBNEW (ob, T); +} + +#undef XOBNEW +#define XOBNEW(O, T) xobnew<T> (O) + +template<typename T> +static T * +xobnewvec (obstack *ob, size_t n) +{ + static_assert (IsMallocable<T>::value, "Trying to use XOBNEWVEC with a \ +non-POD data type."); + return XOBNEWVEC (ob, T, n); +} + +#undef XOBNEWVEC +#define XOBNEWVEC(O, T, N) xobnewvec<T> (O, N) + #endif /* COMMON_POISON_H */ diff --git a/gdb/common/traits.h b/gdb/common/traits.h index d9e6839..070ef15 100644 --- a/gdb/common/traits.h +++ b/gdb/common/traits.h @@ -33,6 +33,14 @@ # define HAVE_IS_TRIVIALLY_COPYABLE 1 #endif +/* HAVE_IS_TRIVIALLY_CONSTRUCTIBLE is defined as 1 iff + std::is_trivially_constructible is available. GCC only implemented it + in GCC 5. */ +#if (__has_feature(is_trivially_constructible) \ + || (defined __GNUC__ && __GNUC__ >= 5)) +# define HAVE_IS_TRIVIALLY_COPYABLE 1 +#endif + namespace gdb { /* Pre C++14-safe (CWG 1558) version of C++17's std::void_t. See diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 4207e4c..80185a9 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -3601,8 +3601,7 @@ dwarf2_read_index (struct dwarf2_per_objfile *dwarf2_per_objfile) create_addrmap_from_index (dwarf2_per_objfile, &local_map); - map = XOBNEW (&objfile->objfile_obstack, struct mapped_index); - map = new (map) mapped_index (); + map = obstack_new<mapped_index> (&objfile->objfile_obstack); *map = local_map; dwarf2_per_objfile->index_table = map; diff --git a/gdb/gdb_obstack.h b/gdb/gdb_obstack.h index 1011008..29cad93 100644 --- a/gdb/gdb_obstack.h +++ b/gdb/gdb_obstack.h @@ -24,12 +24,40 @@ /* Utility macros - wrap obstack alloc into something more robust. */ -#define OBSTACK_ZALLOC(OBSTACK,TYPE) \ - ((TYPE *) memset (obstack_alloc ((OBSTACK), sizeof (TYPE)), 0, sizeof (TYPE))) +template <typename T> +static inline T* +obstack_zalloc (struct obstack *ob) +{ + static_assert (IsMallocable<T>::value, "Trying to use OBSTACK_ZALLOC with a \ +non-POD data type. Use obstack_new instead."); + return ((T *) memset (obstack_alloc (ob, sizeof (T)), 0, sizeof (T))); +} + +#define OBSTACK_ZALLOC(OBSTACK,TYPE) obstack_zalloc<TYPE> ((OBSTACK)) + +template <typename T> +static inline T * +obstack_calloc (struct obstack *ob, size_t number) +{ + static_assert (IsMallocable<T>::value, "Trying to use OBSTACK_CALLOC with a \ +non-POD data type. Use obstack_new instead."); + return ((T *) memset (obstack_alloc (ob, number * sizeof (T)), 0, + number * sizeof (T))); +} #define OBSTACK_CALLOC(OBSTACK,NUMBER,TYPE) \ - ((TYPE *) memset (obstack_alloc ((OBSTACK), (NUMBER) * sizeof (TYPE)), \ - 0, (NUMBER) * sizeof (TYPE))) + obstack_calloc<TYPE> ((OBSTACK), (NUMBER)) + +/* Allocate an object on OB and call its constructor. */ + +template <typename T, typename... Args> +static inline T* +obstack_new (struct obstack *ob, Args&&... args) +{ + T* object = (T *) obstack_alloc (ob, sizeof (T)); + object = new (object) T (std::forward<Args> (args)...); + return object; +} /* Unless explicitly specified, GDB obstacks always use xmalloc() and xfree(). */ diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c index 1359c2f..4b4ae0b 100644 --- a/gdb/gdbarch.c +++ b/gdb/gdbarch.c @@ -471,15 +471,10 @@ gdbarch_alloc (const struct gdbarch_info *info, } -/* Allocate extra space using the per-architecture obstack. */ -void * -gdbarch_obstack_zalloc (struct gdbarch *arch, long size) +obstack *gdbarch_obstack (gdbarch *arch) { - void *data = obstack_alloc (arch->obstack, size); - - memset (data, 0, size); - return data; + return arch->obstack; } /* See gdbarch.h. */ diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 0084f19..d42e69c 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -38,6 +38,7 @@ #include <vector> #include "frame.h" #include "dis-asm.h" +#include "gdb_obstack.h" struct floatformat; struct ui_file; @@ -1705,14 +1706,17 @@ extern struct gdbarch *gdbarch_alloc (const struct gdbarch_info *info, struct gd extern void gdbarch_free (struct gdbarch *); +/* Get the obstack owned by ARCH. */ + +extern obstack *gdbarch_obstack (gdbarch *arch); /* Helper function. Allocate memory from the ``struct gdbarch'' obstack. The memory is freed when the corresponding architecture is also freed. */ -extern void *gdbarch_obstack_zalloc (struct gdbarch *gdbarch, long size); -#define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), (NR) * sizeof (TYPE))) -#define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), sizeof (TYPE))) +#define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) obstack_calloc<TYPE> (gdbarch_obstack ((GDBARCH)), (NR)) + +#define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) obstack_zalloc<TYPE> (gdbarch_obstack((GDBARCH))) /* Duplicate STRING, returning an equivalent string that's allocated on the obstack associated with GDBARCH. The string is freed when the corresponding diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh index 4fc54cb..ed407cb 100755 --- a/gdb/gdbarch.sh +++ b/gdb/gdbarch.sh @@ -1261,6 +1261,7 @@ cat <<EOF #include <vector> #include "frame.h" #include "dis-asm.h" +#include "gdb_obstack.h" struct floatformat; struct ui_file; @@ -1532,14 +1533,19 @@ extern struct gdbarch *gdbarch_alloc (const struct gdbarch_info *info, struct gd extern void gdbarch_free (struct gdbarch *); +/* Get the obstack owned by ARCH. */ + +extern obstack *gdbarch_obstack (gdbarch *arch); /* Helper function. Allocate memory from the \`\`struct gdbarch'' obstack. The memory is freed when the corresponding architecture is also freed. */ -extern void *gdbarch_obstack_zalloc (struct gdbarch *gdbarch, long size); -#define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), (NR) * sizeof (TYPE))) -#define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), sizeof (TYPE))) +#define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) \ + obstack_calloc<TYPE> (gdbarch_obstack ((GDBARCH)), (NR)) + +#define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) \ + obstack_zalloc<TYPE> (gdbarch_obstack((GDBARCH))) /* Duplicate STRING, returning an equivalent string that's allocated on the obstack associated with GDBARCH. The string is freed when the corresponding @@ -1849,15 +1855,10 @@ EOF printf "\n" printf "\n" cat <<EOF -/* Allocate extra space using the per-architecture obstack. */ -void * -gdbarch_obstack_zalloc (struct gdbarch *arch, long size) +obstack *gdbarch_obstack (gdbarch *arch) { - void *data = obstack_alloc (arch->obstack, size); - - memset (data, 0, size); - return data; + return arch->obstack; } /* See gdbarch.h. */ diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 36ea4b1..61fb344 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -723,12 +723,7 @@ tdesc_find_type (struct gdbarch *gdbarch, const char *id) static void * tdesc_data_init (struct obstack *obstack) { - struct tdesc_arch_data *data; - - data = OBSTACK_ZALLOC (obstack, struct tdesc_arch_data); - new (data) tdesc_arch_data (); - - return data; + return obstack_new<tdesc_arch_data> (obstack); } /* Similar, but for the temporary copy used during architecture -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] Use XOBNEW when possible 2018-04-25 19:40 [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Simon Marchi @ 2018-04-25 19:40 ` Simon Marchi 2018-04-25 22:27 ` Tom Tromey 2018-04-25 22:36 ` [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Tom Tromey 1 sibling, 1 reply; 7+ messages in thread From: Simon Marchi @ 2018-04-25 19:40 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Since XOBNEW is now poisoned to prevent using it with non-trivially-constructible objects, it is worth using it over plain obstack_alloc. This patch changes the locations I could find where we can do that change easily. gdb/ChangeLog: * ada-lang.c (cache_symbol): Use XOBNEW/XOBNEWVEC. * dwarf2-frame.c (dwarf2_build_frame_info): Likewise. * hppa-tdep.c (hppa_init_objfile_priv_data): Likewise. * mdebugread.c (mdebug_build_psymtabs): Likewise. (add_pending): Likewise. (parse_symbol): Likewise. (parse_partial_symbols): Likewise. (psymtab_to_symtab_1): Likewise. (new_psymtab): Likewise. (elfmdebug_build_psymtabs): Likewise. * minsyms.c (terminate_minimal_symbol_table): Likewise. * objfiles.c (get_objfile_bfd_data): Likewise. (objfile_register_static_link): Likewise. * psymtab.c (allocate_psymtab): Likewise. * stabsread.c (read_member_functions): Likewise. * xcoffread.c (xcoff_end_psymtab): Likewise. --- gdb/ada-lang.c | 3 +-- gdb/dwarf2-frame.c | 3 +-- gdb/hppa-tdep.c | 6 ++---- gdb/mdebugread.c | 47 ++++++++++++++++------------------------------- gdb/minsyms.c | 6 ++---- gdb/objfiles.c | 9 ++------- gdb/psymtab.c | 4 +--- gdb/stabsread.c | 5 ++--- gdb/xcoffread.c | 3 +-- 9 files changed, 28 insertions(+), 58 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index de20c43..8145480 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -4744,8 +4744,7 @@ cache_symbol (const char *name, domain_enum domain, struct symbol *sym, return; h = msymbol_hash (name) % HASH_SIZE; - e = (struct cache_entry *) obstack_alloc (&sym_cache->cache_space, - sizeof (*e)); + e = XOBNEW (&sym_cache->cache_space, cache_entry); e->next = sym_cache->root[h]; sym_cache->root[h] = e; e->name = copy diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c index 753ce1f..1a444fd 100644 --- a/gdb/dwarf2-frame.c +++ b/gdb/dwarf2-frame.c @@ -2205,8 +2205,7 @@ dwarf2_build_frame_info (struct objfile *objfile) fde_table.entries = NULL; /* Build a minimal decoding of the DWARF2 compilation unit. */ - unit = (struct comp_unit *) obstack_alloc (&objfile->objfile_obstack, - sizeof (struct comp_unit)); + unit = XOBNEW (&objfile->objfile_obstack, comp_unit); unit->abfd = objfile->obfd; unit->objfile = objfile; unit->dbase = 0; diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c index 84dbd66..c739410 100644 --- a/gdb/hppa-tdep.c +++ b/gdb/hppa-tdep.c @@ -205,11 +205,9 @@ hppa_symbol_address(const char *sym) static struct hppa_objfile_private * hppa_init_objfile_priv_data (struct objfile *objfile) { - struct hppa_objfile_private *priv; + hppa_objfile_private *priv + = XOBNEW (&objfile->objfile_obstack, hppa_objfile_private); - priv = (struct hppa_objfile_private *) - obstack_alloc (&objfile->objfile_obstack, - sizeof (struct hppa_objfile_private)); set_objfile_data (objfile, hppa_objfile_priv_data, priv); memset (priv, 0, sizeof (*priv)); diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c index c0bce55..2741512 100644 --- a/gdb/mdebugread.c +++ b/gdb/mdebugread.c @@ -356,9 +356,8 @@ mdebug_build_psymtabs (minimal_symbol_reader &reader, char *fdr_end; FDR *fdr_ptr; - info->fdr = (FDR *) obstack_alloc (&objfile->objfile_obstack, - (info->symbolic_header.ifdMax - * sizeof (FDR))); + info->fdr = (FDR *) XOBNEWVEC (&objfile->objfile_obstack, FDR, + info->symbolic_header.ifdMax); fdr_src = (char *) info->external_fdr; fdr_end = (fdr_src + info->symbolic_header.ifdMax * swap->external_fdr_size); @@ -508,9 +507,7 @@ add_pending (FDR *fh, char *sh, struct type *t) /* Make sure we do not make duplicates. */ if (!p) { - p = ((struct mdebug_pending *) - obstack_alloc (&mdebugread_objfile->objfile_obstack, - sizeof (struct mdebug_pending))); + p = XOBNEW (&mdebugread_objfile->objfile_obstack, mdebug_pending); p->s = sh; p->t = t; p->next = pending_list[f_idx]; @@ -1174,9 +1171,8 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend, SYMBOL_DOMAIN (s) = LABEL_DOMAIN; SYMBOL_ACLASS_INDEX (s) = LOC_CONST; SYMBOL_TYPE (s) = objfile_type (mdebugread_objfile)->builtin_void; - e = ((struct mdebug_extra_func_info *) - obstack_alloc (&mdebugread_objfile->objfile_obstack, - sizeof (struct mdebug_extra_func_info))); + e = XOBNEW (&mdebugread_objfile->objfile_obstack, + mdebug_extra_func_info); memset (e, 0, sizeof (struct mdebug_extra_func_info)); SYMBOL_VALUE_BYTES (s) = (gdb_byte *) e; e->numargs = top_stack->numargs; @@ -2372,8 +2368,7 @@ parse_partial_symbols (minimal_symbol_reader &reader, && (bfd_get_section_flags (cur_bfd, text_sect) & SEC_RELOC)) relocatable = 1; - extern_tab = (EXTR *) obstack_alloc (&objfile->objfile_obstack, - sizeof (EXTR) * hdr->iextMax); + extern_tab = XOBNEWVEC (&objfile->objfile_obstack, EXTR, hdr->iextMax); includes_allocated = 30; includes_used = 0; @@ -2415,10 +2410,8 @@ parse_partial_symbols (minimal_symbol_reader &reader, } /* Allocate the global pending list. */ - pending_list = - ((struct mdebug_pending **) - obstack_alloc (&objfile->objfile_obstack, - hdr->ifdMax * sizeof (struct mdebug_pending *))); + pending_list = XOBNEWVEC (&objfile->objfile_obstack, mdebug_pending *, + hdr->ifdMax); memset (pending_list, 0, hdr->ifdMax * sizeof (struct mdebug_pending *)); @@ -2659,8 +2652,7 @@ parse_partial_symbols (minimal_symbol_reader &reader, textlow, objfile->global_psymbols, objfile->static_psymbols); - pst->read_symtab_private = obstack_alloc (&objfile->objfile_obstack, - sizeof (struct symloc)); + pst->read_symtab_private = XOBNEW (&objfile->objfile_obstack, symloc); memset (pst->read_symtab_private, 0, sizeof (struct symloc)); save_pst = pst; @@ -3773,11 +3765,8 @@ parse_partial_symbols (minimal_symbol_reader &reader, /* Skip the first file indirect entry as it is a self dependency for source files or a reverse .h -> .c dependency for header files. */ pst->number_of_dependencies = 0; - pst->dependencies = - ((struct partial_symtab **) - obstack_alloc (&objfile->objfile_obstack, - ((fh->crfd - 1) - * sizeof (struct partial_symtab *)))); + pst->dependencies = XOBNEWVEC (&objfile->objfile_obstack, + partial_symtab *, (fh->crfd - 1)); for (s_idx = 1; s_idx < fh->crfd; s_idx++) { RFDT rh; @@ -4064,10 +4053,9 @@ psymtab_to_symtab_1 (struct objfile *objfile, { /* Make up special symbol to contain procedure specific info. */ - struct mdebug_extra_func_info *e = - ((struct mdebug_extra_func_info *) - obstack_alloc (&mdebugread_objfile->objfile_obstack, - sizeof (struct mdebug_extra_func_info))); + mdebug_extra_func_info *e + = XOBNEW (&mdebugread_objfile->objfile_obstack, + mdebug_extra_func_info); struct symbol *s = new_symbol (MDEBUG_EFI_SYMBOL_NAME); memset (e, 0, sizeof (struct mdebug_extra_func_info)); @@ -4750,8 +4738,7 @@ new_psymtab (const char *name, struct objfile *objfile) /* Keep a backpointer to the file's symbols. */ - psymtab->read_symtab_private = obstack_alloc (&objfile->objfile_obstack, - sizeof (struct symloc)); + psymtab->read_symtab_private = XOBNEW (&objfile->objfile_obstack, symloc); memset (psymtab->read_symtab_private, 0, sizeof (struct symloc)); CUR_BFD (psymtab) = cur_bfd; DEBUG_SWAP (psymtab) = debug_swap; @@ -4877,9 +4864,7 @@ elfmdebug_build_psymtabs (struct objfile *objfile, minimal_symbol_reader reader (objfile); - info = ((struct ecoff_debug_info *) - obstack_alloc (&objfile->objfile_obstack, - sizeof (struct ecoff_debug_info))); + info = XOBNEW (&objfile->objfile_obstack, ecoff_debug_info); if (!(*swap->read_debug_info) (abfd, sec, info)) error (_("Error reading ECOFF debugging information: %s"), diff --git a/gdb/minsyms.c b/gdb/minsyms.c index 9d23c4f..db3a912 100644 --- a/gdb/minsyms.c +++ b/gdb/minsyms.c @@ -1427,10 +1427,8 @@ void terminate_minimal_symbol_table (struct objfile *objfile) { if (! objfile->per_bfd->msymbols) - objfile->per_bfd->msymbols - = ((struct minimal_symbol *) - obstack_alloc (&objfile->per_bfd->storage_obstack, - sizeof (struct minimal_symbol))); + objfile->per_bfd->msymbols = XOBNEW (&objfile->per_bfd->storage_obstack, + minimal_symbol); { struct minimal_symbol *m diff --git a/gdb/objfiles.c b/gdb/objfiles.c index 98e81c4..2ec358a 100644 --- a/gdb/objfiles.c +++ b/gdb/objfiles.c @@ -147,11 +147,7 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd) set_bfd_data (abfd, objfiles_bfd_data, storage); } else - { - storage = (objfile_per_bfd_storage *) - obstack_alloc (&objfile->objfile_obstack, - sizeof (objfile_per_bfd_storage)); - } + storage = XOBNEW (&objfile->objfile_obstack, objfile_per_bfd_storage); /* objfile_per_bfd_storage is not trivially constructible, must call the ctor manually. */ @@ -269,8 +265,7 @@ objfile_register_static_link (struct objfile *objfile, slot = htab_find_slot (objfile->static_links, &lookup_entry, INSERT); gdb_assert (*slot == NULL); - entry = (struct static_link_htab_entry *) obstack_alloc - (&objfile->objfile_obstack, sizeof (*entry)); + entry = XOBNEW (&objfile->objfile_obstack, static_link_htab_entry); entry->block = block; entry->static_link = static_link; *slot = (void *) entry; diff --git a/gdb/psymtab.c b/gdb/psymtab.c index ac0ee0a..fa59ee2 100644 --- a/gdb/psymtab.c +++ b/gdb/psymtab.c @@ -1805,9 +1805,7 @@ allocate_psymtab (const char *filename, struct objfile *objfile) objfile->free_psymtabs = psymtab->next; } else - psymtab = (struct partial_symtab *) - obstack_alloc (&objfile->objfile_obstack, - sizeof (struct partial_symtab)); + psymtab = XOBNEW (&objfile->objfile_obstack, partial_symtab); memset (psymtab, 0, sizeof (struct partial_symtab)); psymtab->filename diff --git a/gdb/stabsread.c b/gdb/stabsread.c index 0017f18..2149014 100644 --- a/gdb/stabsread.c +++ b/gdb/stabsread.c @@ -2731,9 +2731,8 @@ read_member_functions (struct field_info *fip, const char **pp, xfree (main_fn_name); } - new_fnlist->fn_fieldlist.fn_fields = (struct fn_field *) - obstack_alloc (&objfile->objfile_obstack, - sizeof (struct fn_field) * length); + new_fnlist->fn_fieldlist.fn_fields + = XOBNEWVEC (&objfile->objfile_obstack, fn_field, length); memset (new_fnlist->fn_fieldlist.fn_fields, 0, sizeof (struct fn_field) * length); for (i = length; (i--, sublist); sublist = sublist->next) diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c index 8c707aa..4f9b315 100644 --- a/gdb/xcoffread.c +++ b/gdb/xcoffread.c @@ -2097,8 +2097,7 @@ xcoff_end_psymtab (struct objfile *objfile, struct partial_symtab *pst, struct partial_symtab *subpst = allocate_psymtab (include_list[i], objfile); - subpst->read_symtab_private = obstack_alloc (&objfile->objfile_obstack, - sizeof (struct symloc)); + subpst->read_symtab_private = XOBNEW (&objfile->objfile_obstack, symloc); ((struct symloc *) subpst->read_symtab_private)->first_symnum = 0; ((struct symloc *) subpst->read_symtab_private)->numsyms = 0; subpst->textlow = 0; -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Use XOBNEW when possible 2018-04-25 19:40 ` [PATCH 2/2] Use XOBNEW when possible Simon Marchi @ 2018-04-25 22:27 ` Tom Tromey 2018-04-26 2:59 ` Simon Marchi 0 siblings, 1 reply; 7+ messages in thread From: Tom Tromey @ 2018-04-25 22:27 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches >>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes: Simon> Since XOBNEW is now poisoned to prevent using it with Simon> non-trivially-constructible objects, it is worth using it over plain Simon> obstack_alloc. This patch changes the locations I could find where we Simon> can do that change easily. Thanks. Simon> - struct hppa_objfile_private *priv; Simon> + hppa_objfile_private *priv Simon> + = XOBNEW (&objfile->objfile_obstack, hppa_objfile_private); Simon> - priv = (struct hppa_objfile_private *) Simon> - obstack_alloc (&objfile->objfile_obstack, Simon> - sizeof (struct hppa_objfile_private)); Simon> set_objfile_data (objfile, hppa_objfile_priv_data, priv); Simon> memset (priv, 0, sizeof (*priv)); Maybe ones that memset should be using OBSTACK_ZALLOC. Simon> + e = XOBNEW (&mdebugread_objfile->objfile_obstack, Simon> + mdebug_extra_func_info); Simon> memset (e, 0, sizeof (struct mdebug_extra_func_info)); There seem to be a few. Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Use XOBNEW when possible 2018-04-25 22:27 ` Tom Tromey @ 2018-04-26 2:59 ` Simon Marchi 0 siblings, 0 replies; 7+ messages in thread From: Simon Marchi @ 2018-04-26 2:59 UTC (permalink / raw) To: Tom Tromey; +Cc: Simon Marchi, gdb-patches On 2018-04-25 18:27, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes: > > Simon> Since XOBNEW is now poisoned to prevent using it with > Simon> non-trivially-constructible objects, it is worth using it over > plain > Simon> obstack_alloc. This patch changes the locations I could find > where we > Simon> can do that change easily. > > Thanks. > > Simon> - struct hppa_objfile_private *priv; > Simon> + hppa_objfile_private *priv > Simon> + = XOBNEW (&objfile->objfile_obstack, hppa_objfile_private); > > Simon> - priv = (struct hppa_objfile_private *) > Simon> - obstack_alloc (&objfile->objfile_obstack, > Simon> - sizeof (struct hppa_objfile_private)); > Simon> set_objfile_data (objfile, hppa_objfile_priv_data, priv); > Simon> memset (priv, 0, sizeof (*priv)); > > Maybe ones that memset should be using OBSTACK_ZALLOC. Right, I'll do that. Thanks, Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions 2018-04-25 19:40 [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Simon Marchi 2018-04-25 19:40 ` [PATCH 2/2] Use XOBNEW when possible Simon Marchi @ 2018-04-25 22:36 ` Tom Tromey 2018-04-26 2:58 ` Simon Marchi 1 sibling, 1 reply; 7+ messages in thread From: Tom Tromey @ 2018-04-25 22:36 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches >>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes: Simon> This patch introduces a utility to make this pattern simpler: Simon> foo *f = obstack_new<foo> (); What about just having those types that can use this inherit from allocate_on_obstack? Then you can write: foo *f = new (obstack) foo (); It would be nice if we could have a global operator new that does this, but I don't think it is possible to have one that is limited to is_trivially_constructible classes. Maybe is_trivially_destructible is also needed somehow since an obstack isn't going to run those destructors. Not sure how to manage this either, right now allocate_on_obstack just assumes you know what you're up to. Simon> To help catch places where we would forget to call new when allocating Simon> such an object on an obstack, this patch also poisons some other methods Simon> of allocating an instance of a type on an obstack: This is excellent. Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions 2018-04-25 22:36 ` [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Tom Tromey @ 2018-04-26 2:58 ` Simon Marchi 2018-04-26 17:42 ` Tom Tromey 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi @ 2018-04-26 2:58 UTC (permalink / raw) To: Tom Tromey; +Cc: Simon Marchi, gdb-patches Hi Tom, Thanks for taking a look. On 2018-04-25 18:36, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes: > > Simon> This patch introduces a utility to make this pattern simpler: > Simon> foo *f = obstack_new<foo> (); > > What about just having those types that can use this inherit from > allocate_on_obstack? Then you can write: > > foo *f = new (obstack) foo (); I thought about this. The downside I see is that it forces new to allocate on an obstack. What if you want to use the standard new for the same type in another context? Maybe this doesn't happen in practice though. > It would be nice if we could have a global operator new that does this, > but I don't think it is possible to have one that is limited to > is_trivially_constructible classes. That goes beyond my C++ knowledge. > Maybe is_trivially_destructible is also needed somehow since an obstack > isn't going to run those destructors. Not sure how to manage this > either, right now allocate_on_obstack just assumes you know what you're > up to. I remember you having some back and forth with Yao about this. Here's my take on it. It is possible to allocate objects that need to be destroyed on obstacks, the important thing is to call the destructor manually at some point before the obstack is freed. It's done for mapped_index right now, for example. You just can't allocate such an object on an obstack and forget about it, assuming it will be freed by magic (it's just like when you "new" something, you have to keep track of it to destroy it at some point). When doing this, you obviously lose one nice feature of the obstack, not having to care about deallocation. So if that was the main reason an obstack is used, you might as well reconsider using an obstack at all [1]. But if the obstack is used for the more efficient memory allocation, then it would still make sense to use an obstack and track the objects separately so that they can be destroyed properly. What we need is (not sure this is realistic): everywhere we allocate an object on an obstack and don't keep track of it for further destruction, have a static_assert there that the type should be trivially destructible. If you happen to make said type non trivially destructible, you'll get an error and realize "oh right, the objects created here are never destroyed". Or maybe there are better C++ ways of getting the same advantages as with an obstack (efficient allocation for a bunch of objects allocated/freed at the same time, data locality)? [1] For example, is there a reason why mapped_index is allocated on the objfile obstack, and not newed just like mapped_debug_names? Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions 2018-04-26 2:58 ` Simon Marchi @ 2018-04-26 17:42 ` Tom Tromey 0 siblings, 0 replies; 7+ messages in thread From: Tom Tromey @ 2018-04-26 17:42 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, Simon Marchi, gdb-patches >>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: >> foo *f = new (obstack) foo (); Simon> I thought about this. The downside I see is that it forces new to Simon> allocate on an obstack. What if you want to use the standard new for Simon> the same type in another context? Maybe this doesn't happen in Simon> practice though. Good point. We could have another mixin to provide a second operator new, but that seems sort of ugly. Simon> So if that was the main reason an obstack is used, you Simon> might as well reconsider using an obstack at all [1]. But if the Simon> obstack is used for the more efficient memory allocation, then it Simon> would still make sense to use an obstack and track the objects Simon> separately so that they can be destroyed properly. What we need is Simon> (not sure this is realistic): everywhere we allocate an object on an Simon> obstack and don't keep track of it for further destruction, have a Simon> static_assert there that the type should be trivially destructible. Yeah. And I'm not sure if this is doable either. Maybe if we removed allocate_on_obstack and switched completely to your obstack_new, and then made it static_assert is_trivially_destructible. Simon> Or maybe there are better C++ ways of getting the same advantages as Simon> with an obstack (efficient allocation for a bunch of objects Simon> allocated/freed at the same time, data locality)? I'm not totally sure that obstacks are so great. For example see https://sourceware.org/bugzilla/show_bug.cgi?id=17143 In the past I think many times, something was put on the objfile obstack simply because it was convenient. For example I know there were cases where this was done with hash tables (even though hash resizing means leaking storage) just because it was simpler than trying to find the correct spot to clean up. This aspect is certainly something we can do more easily now with C++ -- just make the members clean up after themselves. Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-26 17:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-25 19:40 [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Simon Marchi 2018-04-25 19:40 ` [PATCH 2/2] Use XOBNEW when possible Simon Marchi 2018-04-25 22:27 ` Tom Tromey 2018-04-26 2:59 ` Simon Marchi 2018-04-25 22:36 ` [PATCH 1/2] Introduce obstack_new, poison other "typed" obstack functions Tom Tromey 2018-04-26 2:58 ` Simon Marchi 2018-04-26 17:42 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).