This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete
On 11/22/2017 04:41 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> This patch (finally!) makes it so that trying to use XNEW with a type
> that requires "new" will cause a compilation error.
Yay!
The criterion I
> initially used to allow a type to use XNEW (which calls malloc in the
> end) was std::is_trivially_constructible, but then realized that gcc 4.8
> did not have it. Instead, I went with:
>
Yeah, in the memset poisoning I just disabled the poisoning on older
compilers. This is the "#if HAVE_IS_TRIVIALLY_COPYABLE" check in
poison.h.
> using IsMallocatable = std::is_pod<T>;
>
Nit, I think "malloc-able" is more common than "malloc-atable".
At least, the latter sounds odd to me. Or sounds like
"m-allocatable", which is a different thing. :-)
> which is just a bit more strict, which doesn't hurt. A similar thing is
> done for macros that free instead of allocated, the criterion is:
>
> using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
>
> For simplicity, we could also do for std::is_pod for IsFreeable as well,
> if you prefer.
>
> I chose to put static_assert in the functions, instead of using
> gdb::Requires in the template as SFINAE, because it allows to put a
> message, which I think makes the compiler error more understandable.
> With gdb::Requires, the error is:
Right, I think static_assert is what I had suggest initially too.
SFINAE alone wouldn't sound right to me here.
The right alternative to static_assert would be SFINAE+"=delete;",
which is what we do for the memcpy/memcpy poisoning, and what
you're doing to "free".
=delete keeps the function/overload in the overload set, serving
as "honeypot", so you get an error about trying to call a
deleted function.
You could do that to xfree instead
of the static_assert, but I guess inlining xfree ends up being
a good thing.
> I think the first one is more likely to just make people yell at their
> screen, especially those less used to C++.
Yes, don't do that. :-)
>
> Generated-code-wise, it adds one more function call (xnew<T>) when using
> XNEW and building with -O0, but it all goes away with optimizations
> enabled.
Good.
>
> gdb/ChangeLog:
>
> * common/common-utils.h: Include poison.h.
> (xfree): Remove declaration, add definition with static_assert.
> * common/common-utils.c (xfree): Remove.
> * common/poison.h (IsMallocatable): Define.
> (IsFreeable): Define.
> (free): Delete for non-freeable types.
> (xnew): New.
> (XNEW): Undef and redefine.
> (xcnew): New.
> (XCNEW): Undef and redefine.
> (xdelete): New.
> (XDELETE): Undef and redefine.
> (xnewvec): New.
> (XNEWVEC): Undef and redefine.
> (xcnewvec): New.
> (XCNEWVEC): Undef and redefine.
> (xresizevec): New.
> (XRESIZEVEC): Undef and redefine.
> (xdeletevec): New.
> (XDELETEVEC): Undef and redefine.
> (xnewvar): New.
> (XNEWVAR): Undef and redefine.
> (xcnewvar): New.
> (XCNEWVAR): Undef and redefine.
> (xresizevar): New.
> (XRESIZEVAR): Undef and redefine.
> ---
> gdb/common/common-utils.c | 7 ---
> gdb/common/common-utils.h | 14 ++++-
> gdb/common/poison.h | 132 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 145 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
> index 7139302..66d6161 100644
> --- a/gdb/common/common-utils.c
> +++ b/gdb/common/common-utils.c
> @@ -95,13 +95,6 @@ xzalloc (size_t size)
> }
>
> void
> -xfree (void *ptr)
> -{
> - if (ptr != NULL)
> - free (ptr); /* ARI: free */
> -}
> -
> -void
> xmalloc_failed (size_t size)
> {
> malloc_failure (size);
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 4926a32..feb4790 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -23,6 +23,8 @@
> #include <string>
> #include <vector>
>
> +#include "poison.h"
> +
> /* If possible, define FUNCTION_NAME, a macro containing the name of
> the function being defined. Since this macro may not always be
> defined, all uses must be protected by appropriate macro definition
> @@ -47,7 +49,17 @@
> /* Like xmalloc, but zero the memory. */
> void *xzalloc (size_t);
>
> -void xfree (void *);
> +template <typename T>
> +static void
> +xfree (T *ptr)
> +{
> + static_assert (IsFreeable<T>::value, "Trying to use xfree with a non-POD \
> +data type. Use operator delete instead.");
> +
> + if (ptr != NULL)
> + free (ptr); /* ARI: free */
> +}
> +
>
> /* Like asprintf and vasprintf, but return the string, throw an error
> if no memory. */
> diff --git a/gdb/common/poison.h b/gdb/common/poison.h
> index 37dd35e..de4cefa 100644
> --- a/gdb/common/poison.h
> +++ b/gdb/common/poison.h
> @@ -84,4 +84,136 @@ void *memmove (D *dest, const S *src, size_t n) = delete;
>
> #endif /* HAVE_IS_TRIVIALLY_COPYABLE */
>
> +/* Poison XNEW and friends to catch usages of malloc-style allocations on
> + objects that require new/delete. */
> +
> +template<typename T>
> +using IsMallocatable = std::is_pod<T>;
> +
> +template<typename T>
> +using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
> +
> +template <typename T, typename = gdb::Requires<gdb::Not<IsFreeable<T>>>>
> +void free (T *ptr) = delete;
> +
> +template<typename T>
> +static T *
> +xnew ()
> +{
> + static_assert (IsMallocatable<T>::value, "Trying to use XNEW with a non-POD \
> +data type. Use operator new instead.");
> + return XNEW (T);
> +}
> +
> +#undef XNEW
> +#define XNEW(T) xnew<T>()
> +
> +template<typename T>
> +static T *
> +xcnew ()
> +{
> + static_assert (IsMallocatable<T>::value, "Trying to use XCNEW with a non-POD \
> +data type. Use operator new instead.");
> + return XCNEW (T);
> +}
> +
> +#undef XCNEW
> +#define XCNEW(T) xcnew<T>()
> +
> +template<typename T>
> +static void
> +xdelete (T *p)
> +{
> + static_assert (IsFreeable<T>::value, "Trying to use XDELETE with a non-POD \
> +data type. Use operator delete instead.");
> + XDELETE (p);
> +}
> +
> +#undef XDELETE
> +#define XDELETE(P) xdelete (p)
> +
> +template<typename T>
> +static T*
Missing space after T in several of these functions.
> +xnewvec (size_t n)
> +{
> + static_assert (IsMallocatable<T>::value, "Trying to use XNEWVEC with a \
> +non-POD data type. Use operator new[] (or std::vector) instead.");
> + return XNEWVEC (T, n);
> +}
> +
Other than the formatting, this LGTM. Thanks a lot for doing this.
Thanks,
Pedro Alves