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]

RFA: new pass to warn on questionable uses of alloca() and VLAs


[New thread now that I actually have a tested patch :)].

I think detecting potentially problematic uses of alloca would
be useful, especially when done in an intelligent way like in
your patch (as opposed to simply diagnosing every call to
the function regardless of the value of its argument).  At
the same time, it seems that an even more reliable solution
than pointing out potentially unsafe calls to the function
and relying on users to modify their code to use malloc for
large/unbounded allocations would be to let GCC do it for
them automatically (i.e., in response to some other option,
emit a call to malloc instead, and insert a call to free when
appropriate).

As Jeff said, we were thinking the other way around: notice a malloced area that doesn't escape and replace it with a call to alloca. But all this is beyond the scope of this patch.


I applied the patch and experimented with it a bit (I haven't
studied the code in any detail yet) and found a few opportunities
for improvements.  I describe them below (Sorry in advance for
the length of my comments!)

BTW, thank you so much for taking the time to look into this. Your feedback has been invaluable.


I found the "warning: unbounded use of alloca" misleading when
a call to the function was, in fact, bounded but to a limit
that's greater than alloca-max-size as in the program below:

I have added various levels of granularity for the warning, along with appropriately different messages:

// Possible problematic uses of alloca.
enum alloca_type {
  // Alloca argument is within known bounds that are appropriate.
  ALLOCA_OK,

  // Alloca argument is KNOWN to have a value that is too large.
  ALLOCA_BOUND_DEFINITELY_LARGE,

  // Alloca argument may be too large.
  ALLOCA_BOUND_MAYBE_LARGE,

  // Alloca argument is bounded but of an indeterminate size.
  ALLOCA_BOUND_UNKNOWN,

  // Alloca argument was casted from a signed integer.
  ALLOCA_CAST_FROM_SIGNED,

  // Alloca appears in a loop.
  ALLOCA_IN_LOOP,

  // Alloca call is unbounded.  That is, there is no controlling
  // predicate for its argument.
  ALLOCA_UNBOUNDED
};

Of course, there are plenty of cases where we can't get the exact diagnosis (due to the limitations on our range info) and we fall back to ALLOCA_UNBOUNDED or ALLOCA_BOUND_MAYBE_LARGE. In practice, I'm wondering whether we should lump everything into 2-3 warnings instead of trying so hard to get the exact reason for the problematic use of alloca. (More details on upcoming changes to range info further down.)


  void f (void*);

  void g (int n)
  {
    void *p;
    if (n < 4096)
      p = __builtin_alloca (n);
    else
      p = __builtin_malloc (n);
    f (p);
  }
  t.C: In function ‘g’:
  t.C:7:7: warning: unbounded use of alloca [-Walloca]
       p = __builtin_alloca (n);

Well, in this particular case you are using a signed int, so n < 4096 can cause the value passed to alloca to be rather large in the case of n < 0.


I would suggest to rephrase the diagnostic to mention the limit,
e.g.,

  warning: calling alloca with an argument in excess of '4000'
  bytes

In the attached patch I try to diagnose these cases with:

a.c: In function ‘g’:
a.c:7:10: warning: cast from signed type in alloca [-Walloca]
p = __builtin_alloca (n);

I'm not 100% convinced this the best idea, and I could be easily convinced to narrow the wide variety of warning cases I currently have into just a handful less specific ones.


I'm not sure I understand how -Walloca-max-size is supposed to
be used.  For example, it has no effect on the test case above
(i.e., I couldn't find a way to use it to raise the limit to
avoid the warning).  Maybe the interaction of the two options
is more subtle than I think.  I would have expected either

If by subtle you mean buggy, then yes-- thank you for your kind words :). I have fixed it all, and those found responsible have been sacked.

a single option to control whether alloca warnings are to be
emitted and also (optionally) the allocation threshold, or
perhaps two options, one to turn the warning on and off, and
another just to override the threshold (though this latter
approach seems superfluous given that a single option can do
both).
...
I also think that VLA diagnostics would be better controlled
by a separate option, and emit a different diagnostic (one

I have overhauled the options and added extensive documentation to invoke.texi explaining them. See the included testcases. I have tried to add a testcase for everything the pass currently handles.

In the interest of keeping a consistent relationship with -Wvla, we now have:

-Walloca:	Warn on every use of alloca (not VLAs).
-Walloca=999:	Warn on unbounded uses of alloca, bounded uses with
		no known limit, and bounded uses where the number of
		bytes is greater than 999.
-Wvla:		Behaves as currently (warn on every use of VLAs).
-Wvla=999:	Similar to -Walloca=999, but for -Wvla.

Notice plain -Walloca doesn't have a default, and just warns on every use of alloca, just like -Wvla does for VLAs. This way we can be consistent with -Wvla, and just add the -Wvla=999 variant.

I have added appropriate warnings to avoid confusion with -Wvla=0 and -Walloca=0, because they would conflict with -Wno-vla and -Wno-alloca (setting warn_vla=0 and warn_alloca=0 respectively).

The option handling is a bit convoluted because no -Wvla passed is warn_vla == -1, and CPP and the FE's depend on this information to know when no -Wvla was passed (pedantic warnings and some CPP pre-defines). So to keep everything consistent I had to differentiate between no -Wvla passed, plain -Wvla passed, and -Wvla=999 passed, without getting confused by -Wvla=0 and -Wno-vla. Ughhh...

Beyond a separate option for VLAs, in my limited testing I
couldn't figure out how constrain the VLA size so that using
-Walloca wouldn't complain for example on the following code:

  void f (void*);

  void h (int n)
  {
    if (n < 100)
    {
      int a [n];
      f (a);
    }
  }

Fixed. Also, you have a signed int here as well. That could cause a very large allocation.

This patch depends on range information, which is less than stellar past the VRP pass. To get better range information, we'd have to incorporate this somehow into the VRP pass and use the ASSERT_EXPRs therein. And still, VRP gets awfully confused with PHI merge points. Andrew Macleod is working on a new fancy range info infrastructure that should eliminate a lot of the false positives we may get and/or help us diagnose a wider range of cases. Hence the reason that I'm not bending over backwards to incorporate this pass into tree-vrp.c.

FYI, I have a few XFAILed tests in this patch, to make sure we don't lose track of possible improvements (which in this case should be handled by better range info). What's the blessed way of doing this? Are adding new XFAILed tests acceptable?

I have regstrapped this patch on x86-64 Linux, and have tested the resulting compiler by building glibc with:

/source/glibc/configure --prefix=/usr CC="/path/bin/gcc -Walloca=5000" --disable-werror

This of course, pointed out all sorts of interesting things!

Fire away!

Aldy

Attachment: curr
Description: Text document


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