On Thu, Aug 4, 2016 at 5:19 PM, Aldy Hernandez <al...@redhat.com> wrote:
> On 08/04/2016 08:57 AM, Richard Biener wrote:
>>
>> On Thu, Aug 4, 2016 at 1:30 PM, Aldy Hernandez <al...@redhat.com> 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;
>>>
>>> <bb 2>:
>>> _3 = malloc (50000);
>>> f (_3);
>>>
>>> <bb 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:
>
> <bb 2>:
> 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
