Using gnu::unique_ptr to avoid manual cleanups (was Re: [PATCH 2/2] use unique_ptr some)

[Trevor Saunders]

On Tue, Oct 17, 2017 at 04:23:10PM +0100, Pedro Alves wrote:
> On 10/17/2017 03:57 PM, David Malcolm wrote:
> 
> > Given that we build with -fno-exceptions, what are we guaranteed about
> > what happens when "new" fails?  (am I right in thinking that a failed
> > allocation returns NULL in this case?).  Is XNEWVEC preferable here?
> 
> No, that's incorrect.  Even with -fno-exceptions, if new fails,
> then an exception is thrown.  And then because the unwinder
> doesn't find a frame that handles the exception, std::terminate
> is called...
> 
> You can easily see it with this:
> 
> $ cat new-op.cc 
> int main ()
> {
>   char * p = new char [-1];
>   return 0;
> }
> 
> $ g++ new-op.cc -o new-op -g3 -O0 -fno-exceptions
> $ ./new-op 
> terminate called after throwing an instance of 'std::bad_alloc'
>   what():  std::bad_alloc
> Aborted (core dumped)
> 
> If you want new to return NULL on allocation failure, you either
> have to call the std::nothrow_t overload of new:
> 
>   char* p = new (std::nothrow) char [-1];
> 
> or you have to replace [1] the global operator new to not throw.
> 
> > 
> > i.e. do we prefer:
> > 
> > (A)
> > 
> >   gnu::unique_ptr<uint32_t[]> data (new uint32_t[size + 1]);
> > 
> > (implicitly delete-ing the data, with an access-through-NULL if the
> > allocation fails)
> 
> That'd be my preference too [2].
> 
> Though I think that GCC should replace the global
> operator new/new[] functions to call xmalloc instead of malloc.
> 
> Like:
> 
> void *
> operator new (std::size_t sz)
> {
>   return xmalloc (sz);
> }
> 
> void *
> operator new (std::size_t sz, const std::nothrow_t&)
> {
>   /* malloc (0) is unpredictable; avoid it.  */
>   if (sz == 0)
>     sz = 1;
>   return malloc (sz);
> }
> 
> Then memory allocated by all new expressions, including those done
> via the standard allocators within standard containers, std::vector,
> std::string, etc., ends up in xmalloc, which calls xmalloc_failed on
> allocation failure.  I.e., replace operator new to behave just
> like xmalloc instead of letting it throw an exception that is
> guaranteed to bring down gcc, badly.

There's also the benefit that the replacement operator new can be
inlined and you  end up only having to call through the PLT once for
malloc rather than twice once for malloc and once for new if you use the
default new.

> Note there are already many "new" expressions/calls in GCC
> today.  Any of those can bring down GCC.
> 
> This is very much like what I did for GDB.  See GDB's (latest)
> version here:
> 
>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/new-op.c;hb=bbe910e6e1140cb484a74911f3cea854cf9e7e2a
> 
> GDB's version still throws, because well, GDB uses exceptions.
> 
> [1] Replacing global operator new is something that is totally
> defined by the standard (unlike -fno-exceptions).
> See "global replacements" 
> at <http://en.cppreference.com/w/cpp/memory/new/operator_new>.
> 
> [2] - Or in many cases, use a std::vector instead.  And if
> you care about not value-initializing elements, see gdb's
> gdb/common/{def-vector, default-init-alloc}.h.

I actually kept this patch fairly small because I think a lot of these
should be turned into auto_vecs so they can live on the stack in the
majority of cases.

Trev

> 
> Thanks,
> Pedro Alves
>