From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110523 invoked by alias); 23 Nov 2017 17:27:47 -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 110509 invoked by uid 89); 23 Nov 2017 17:27:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Yeah X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Nov 2017 17:27:45 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id vANHRcCe001793 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 23 Nov 2017 12:27:43 -0500 Received: by simark.ca (Postfix, from userid 112) id 2C5E11E585; Thu, 23 Nov 2017 12:27:38 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 6B0721E02D; Thu, 23 Nov 2017 12:27:17 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 23 Nov 2017 17:27:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete In-Reply-To: References: <1511368867-19365-1-git-send-email-simon.marchi@ericsson.com> <1511368867-19365-5-git-send-email-simon.marchi@ericsson.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.2 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 23 Nov 2017 17:27:38 +0000 X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg00565.txt.bz2 On 2017-11-23 10:02, Pedro Alves wrote: > 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. 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; >> > > 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_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. 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) 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 > gdb::Requires>>> >> +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. Oops, thanks. >> +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, I'm glad that I won't have to carry that branch on the side anymore :) Simon