This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: protected alloca class for malloc fallback


On 08/04/2016 08:57 AM, Richard Biener wrote:
On Thu, Aug 4, 2016 at 1:30 PM, Aldy Hernandez <aldyh@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?


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 :).

  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?


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?

Aldy


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]