Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-11-23 10:02, Pedro Alves wrote:
> 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.

I'm not sure if you are suggesting adding 
HAVE_IS_TRIVIALLY_CONSTRUCTIBLE, but since we can use std::is_pod 
easily, I think it's simpler just to use that (even if it's a bit more 
strict than needed).

>>   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.  :-)

Agreed.
> 
>> 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.

Ah yeah, it would probably make the message a bit clearer.  Instead of 
saying "I can't find a function that matches", it would say "I found a 
function that matches, but you can't use it".  I'll just keep 
static_assert for xfree as well, since it ends up clearer anyway.

>> 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.

Oops, thanks.

>> +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, I'm glad that I won't have to carry that branch on the side 
anymore :)

Simon