From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16651 invoked by alias); 5 Aug 2016 08:17:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 16630 invoked by uid 89); 5 Aug 2016 08:17:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=life X-HELO: mail-wm0-f47.google.com Received: from mail-wm0-f47.google.com (HELO mail-wm0-f47.google.com) (74.125.82.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 05 Aug 2016 08:17:21 +0000 Received: by mail-wm0-f47.google.com with SMTP id o80so25495280wme.1 for ; Fri, 05 Aug 2016 01:17:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=KjQfX9O2GqBQWKKnMM4UA7/T8vS6ITmFB1bWGeOvwHg=; b=iernQ89QNmtXT6Ju2sSExwv1w+cXGnwScXjtk/FNjp5+1yPGXHek2A/bGCEqSeEVhT TXhh9fib3/yr+NpY8Gqynv3jYoobyZw18N0eXVSqOqUrwHT4sAwjZalmXW9v3XDrXZa9 Sn8hStb9q4QQPW3rmqHSbb2dNs9ahDviryT0WTSBXCtRuY1jhZxUY3jyaIlNSrI0xzi0 15dxP6lEqIh1krV24yp1vRcN4IFdeomDICVpjknVx/NOwkMLO61UZ0ofzXPBKUSDAKx1 FP3SPXLr+00I6rRYaPefw0CpKCr9Nzs18wBNsb+bb2XY8Sfrvczux2WSttWCM9J5q/MW MBIA== X-Gm-Message-State: AEkoouvouEo1+Tzsv8ClMX4MFH3VxdN6+UR2LNvRBz58iOQbwDOu/PCtnwBogn5qPOjvZJQkKq8x+D4GXYowWw== X-Received: by 10.28.9.194 with SMTP id 185mr1975123wmj.37.1470385038237; Fri, 05 Aug 2016 01:17:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.137.202 with HTTP; Fri, 5 Aug 2016 01:17:17 -0700 (PDT) In-Reply-To: <57A35CF6.1040504@redhat.com> References: <57A32741.7010003@redhat.com> <57A35CF6.1040504@redhat.com> From: Richard Biener Date: Fri, 05 Aug 2016 08:17:00 -0000 Message-ID: Subject: Re: protected alloca class for malloc fallback To: Aldy Hernandez Cc: gcc-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg00420.txt.bz2 On Thu, Aug 4, 2016 at 5:19 PM, Aldy Hernandez wrote: > On 08/04/2016 08:57 AM, Richard Biener wrote: >> >> On Thu, Aug 4, 2016 at 1:30 PM, Aldy Hernandez wrote: >>> >>> Howdy! >>> >>> As part of my -Walloca-larger-than=life work, I've been running said pass >>> over gcc, binutils, and gdb, and trying to fix things along the way. >>> >>> Particularly irritating and error prone is having to free malloc'd >>> pointers >>> on every function exit point. We end up with a lot of: >>> >>> foo(size_t len) >>> { >>> void *p, *m_p = NULL; >>> if (len < HUGE) >>> p = alloca(len); >>> else >>> p = m_p = malloc(len); >>> if (something) >>> goto out; >>> stuff(); >>> out: >>> free (m_p); >>> } >>> >>> ...which nobody really likes. >>> >>> I've been thinking that for GCC we could have a protected_alloca class >>> whose >>> destructor frees any malloc'd memory: >>> >>> void foo() >>> { >>> char *p; >>> protected_alloca chunk(50000); >>> p = (char *) chunk.pointer(); >>> f(p); >>> } >>> >>> This would generate: >>> >>> void foo() () >>> { >>> void * _3; >>> >>> : >>> _3 = malloc (50000); >>> f (_3); >>> >>> : >>> free (_3); [tail call] >>> return; >>> } >>> >>> Now the problem with this is that the memory allocated by chunk is freed >>> when it goes out of scope, which may not be what you want. For example: >>> >>> func() >>> { >>> char *str; >>> { >>> protected_alloca chunk (99999999); >>> // malloc'd pointer will be freed when chunk goes out of scope. >>> str = (char *) chunk.pointer (); >>> } >>> use (str); // BAD! Use after free. >>> } >> >> >> But how's that an issue if the chunk is created at the exact place where >> there >> previously was an alloca? > > > The pointer can escape if you assign it to a variable outside the scope of > chunk? Take for instance the following snippet in tree.c: > > { > ... > ... > q = (char *) alloca (9 + 17 + len + 1); > memcpy (q, file, len + 1); > > snprintf (q + len, 9 + 17 + 1, "_%08X_" HOST_WIDE_INT_PRINT_HEX, > crc32_string (0, name), get_random_seed (false)); > > p = q; > } > > clean_symbol_name (q); > > If you define `protected_alloca chunk(9 + 17 + len + 1)' at the alloca() > call, chunk will be destroyed at the "}", whereas `q' is still being used > outside of that scope. > > What I am suggesting for this escaping case is to define "protected_alloca > chunk()" at function scope, and then do chunk.alloc(N) in the spot where the > alloca() call was previously at. > > Or am I missing something? Ah, ok - alloca memory is only freed at the end of the function. Only VLAs are freed at scope boundary. >> >> Your class also will not work when internal_alloc is not inlined and >> the alloca path >> is taken like when using non-GCC host compilers. > > > Does not work, or is not optimal? Because defining _ALLOCA_INLINE_ to > nothing and forcing no-inline with: > > g++ -c b.cc -fno-inline -fdump-tree-all -O1 -fno-exceptions > > I still see correct code. It's just that it's inefficient, which we > shouldn't care because bootstrap fixes the non-GCC inlining problem :). As alloca() frees memory at function return code cannot be "correct". > protected_alloca chunk(123); > str = (char *) chunk.pointer(); > use(str); > > becomes: > > : > protected_alloca::protected_alloca (&chunk, 123); > str_3 = protected_alloca::pointer (&chunk); > use (str_3); > protected_alloca::~protected_alloca (&chunk); > return; > > What am I missing? The memory is released after internal_alloc returns. So if you do protected_alloca::protected_alloca (&chunk, 123); ... foo (); // function needing some stack space or GCC spilling ... you'll corrupt memory. >> >>> In the attached patch implementing this class I have provided another >>> idiom >>> for avoiding this problem: >>> >>> func() >>> { >>> void *ptr; >>> protected_alloca chunk; >>> { >>> chunk.alloc (9999999); >>> str = (char *) chunk.pointer (); >>> } >>> // OK, pointer will be freed on function exit. >>> use (str); >>> } >>> >>> So I guess it's between annoying gotos and keeping track of multiple exit >>> points to a function previously calling alloca, or making sure the >>> protected_alloca object always resides in the scope where the memory is >>> going to be used. >>> >>> Is there a better blessed C++ way? If not, is this OK? >> >> >> It looks like you want to replace _all_ alloca uses? What's the point >> in doing this >> at all? Just to be able to enable the warning during bootstrap? > > > Well, it did cross my mind to nix anything that had 0 bounds checks, but I > was mostly interested in things like this: > > gcc.c: > temp = env.get ("COMPILER_PATH"); > if (temp) > { > const char *startp, *endp; > char *nstore = (char *) alloca (strlen (temp) + 3); > > I was just providing a generic interface for dealing with these cases in the > future, instead of gotoing my way out of it. > >> >> Having the conditional malloc/alloca will also inhibit optimization like >> eliding >> the malloc or alloca calls completely. > > > If we can elide the alloca, we can surely elide a conditional alloca / > malloc pair, can't we? We can't at the moment. We don't for malloc (we only remove malloc/free pairs when the memory is not used), we can elide alloca to use a local decl instead. Richard. > > Aldy