[1/2] Introduce obstack_new, poison other "typed" obstack functions
Commit Message
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(-)
Comments
>>>>> "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
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
>>>>> "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
@@ -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 */
@@ -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
@@ -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;
@@ -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(). */
@@ -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. */
@@ -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
@@ -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. */
@@ -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