From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125165 invoked by alias); 23 Nov 2017 15:02:35 -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 125153 invoked by uid 89); 23 Nov 2017 15:02:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=yay!, H*M:206b X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Nov 2017 15:02:25 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CF7B6C058EC2; Thu, 23 Nov 2017 15:02:23 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id D1B276313D; Thu, 23 Nov 2017 15:02:22 +0000 (UTC) Subject: Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete To: Simon Marchi , gdb-patches@sourceware.org References: <1511368867-19365-1-git-send-email-simon.marchi@ericsson.com> <1511368867-19365-5-git-send-email-simon.marchi@ericsson.com> Cc: Simon Marchi From: Pedro Alves Message-ID: Date: Thu, 23 Nov 2017 15:02:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1511368867-19365-5-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-11/txt/msg00540.txt.bz2 On 11/22/2017 04:41 PM, Simon Marchi wrote: > From: Simon Marchi > > 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; > 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_void>; > > 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) 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 > #include > > +#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 > +static void > +xfree (T *ptr) > +{ > + static_assert (IsFreeable::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 > +using IsMallocatable = std::is_pod; > + > +template > +using IsFreeable = gdb::Or, std::is_void>; > + > +template >>> > +void free (T *ptr) = delete; > + > +template > +static T * > +xnew () > +{ > + static_assert (IsMallocatable::value, "Trying to use XNEW with a non-POD \ > +data type. Use operator new instead."); > + return XNEW (T); > +} > + > +#undef XNEW > +#define XNEW(T) xnew() > + > +template > +static T * > +xcnew () > +{ > + static_assert (IsMallocatable::value, "Trying to use XCNEW with a non-POD \ > +data type. Use operator new instead."); > + return XCNEW (T); > +} > + > +#undef XCNEW > +#define XCNEW(T) xcnew() > + > +template > +static void > +xdelete (T *p) > +{ > + static_assert (IsFreeable::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 > +static T* Missing space after T in several of these functions. > +xnewvec (size_t n) > +{ > + static_assert (IsMallocatable::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