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

[Aldy Hernandez <aldyh at redhat dot com>]

On 07/10/2016 06:09 PM, Martin Sebor wrote:
> On 07/08/2016 05:48 AM, Aldy Hernandez wrote:
> > [New thread now that I actually have a tested patch :)].
> ...
> > 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 like it!
>
> > 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!
>
> I've played with the patch a bit over the weekend and have a few
> comments and suggestions (I hope you won't regret encouraging me :)

Not at all.  Your feedback is quite valuable.

> I like the consistency between -Walloca and -Wvla! (And despite
> the volume of my remaining comments, the rest of the patch too!

Well, after Manuel's comments I decided to split things up as previously 
discussed:

-Wvla: warn on any VLA uses (as currently on mainline).
-Wvla-larger-than=N: warn on unbounded use of VLAs, etc.
-Walloca: same as -Wvla but for alloca.
-Walloca-larger-than=N: same as -Wvla-larger-than=N but for alloca.

> 1) Optimization.  Without -O2 GCC prints:
>
>    sorry, unimplemented: -Walloca ignored without -O2

Changed to only sorry() on the -Wvla-larger-than=* and 
-Walloca-larger-than=* options.  The -Wvla and -Walloca warnings work 
without optimization as you suggest below.

> It seems that a warning would be more appropriate here than
> a hard error, but the checker could, and I would say should, be
> made available (in a limited form) without optimization because
>   -Walloca with no argument doesn't rely on it.  I suspect in this
> form, -Walloca is probably mainly going to be useful as a mechanism
> to enforce a "thou shall not use alloca" policy, though not much
> more beyond that.  Some development processes only require that
> code build without optimization in order to allow a commit and
> do more extensive testing with optimization during continuous
> integration, and not enabling it at -O0 would make it difficult
> to adopt the warning on projects that use such a process.

Done.  -Walloca and -Wvla warn on any use of alloca and VLAs 
accordingly, with or without optimization.  I sorry() on the bounded cases.

> 2) When passed an argument of a signed type, GCC prints
>
>    warning: cast from signed type in alloca
>
> even though there is no explicit cast in the code.  It may not
> be obvious why the conversion is a problem in this context.  I
> would suggest to rephrase the warning along the lines of
> -Wsign-conversion which prints:
>
>    conversion to ‘long unsigned int’ from ‘int’ may change the sign of
> the result
>
> and add why it's a potential problem.  Perhaps something like:
>
>    argument to alloca may be too large due to conversion from
>    'int to 'long unsigned int'

Fixed:

void
g2 (short int n)
{
  void *p;
  p = __builtin_alloca (n);
  f (p);
}

b.c:9:5: warning: argument to alloca may be too large due to conversion 
from 'short int' to 'long unsigned int' [-Walloca-larger-than=]
   p = __builtin_alloca (n);
   ~~^~~~~~~~~~~~~~~~~~~~~~

I wonder whether we could do all this in the front-ends as in 
-Wsign-conversion, but this is something that can be done as a follow-up 
if we really want it.

> (In addition, assuming one accepts the lack of range checking and
> constant propagation, this aspect of -Walloca also doesn't need
> optimization.)

I did not do this.  It is technically possible, but I did not want to 
complicate things further.  -Walloca works without optimization, and 
flags everything.  -Walloca-larger-than=* works with -O2 and above.

> 3) I wonder if the warning should also detect alloca calls with
> a zero argument and VLAs of zero size.  They are both likely to
> be programmer errors.  (Although it seems that that would need
> to be done earlier than in the alloca pass.)

Fixed and test added as well.

> 4) I wasn't able to trigger the -Wvla=N warning with VLAs used
> in loops even though VRP provides the range of the value:
>
>    $ cat t.c && /build/gcc-walloca/gcc/xgcc -B /build/gcc-walloca/gcc
> -O2 -S -Wall -Wextra -Wpedantic -Wvla=3 -fdump-tree-vrp=/dev/stdout t.c
> | grep _2
>    void f0 (void*);
>
>    void f1 (void)
>    {
>      for (int i = 0; i != 32; ++i)
>      {
>        char a [i];
>        f0 (a);
>      }
>    }
>
>    _2: [0, 31]
>      sizetype _2;
>      _2 = (sizetype) i_16;
>      a.1_8 = __builtin_alloca_with_align (_2, 8);

There is a "documented" reason for this: :)

      // Do not warn on VLAs occurring in a loop, since VLAs are
      // guaranteed to be cleaned up when they go out of scope.
      // That is, there is a corresponding __builtin_stack_restore
      // at the end of the scope in which the VLA occurs.

> 5) The -Wvla=N logic only seems to take into consideration the number
> of elements but not the size of the element type. For example, I wasn't
> able to get it to warn on the following with -Wvla=255 or greater:
>
>    void f0 (void*);
>
>    void f1 (unsigned char a)
>    {
>      int x [a];   // or even char a [n][__INT_MAX__];
>      f0 (x);
>    }

That was a huge oversight (or should I say over-engineering) on my part. 
 Fixed.

> 6) The patch seems to assume that __builtin_alloca_with_align implies
> a VLA, but that need not be the case.  Based on tree dumps it looks
> like there is a way to distinguish a VLA from a call to the built-in.
> For accuracy's sake I think it would be worth making that distinction
> in the diagnostic.

Good catch.  Fixed:

-Walloca* only applies to alloca(), __builtin_alloca, and 
__builtin_alloca_with_align() called directly.  Whereas, 
__builtin_alloca_with_align() called through a VLA is warned as such. 
There is no wording distinction between direct calls to __builtin_alloca 
and direct calls to __builtin_alloca_with_align though, FYI.

> 7) The -Walloca=N and -Wvla=N argument is a 32-bit integer and no
> checking appears to be done to make sure it doesn't overflow, so
> that invoking GCC with an argument of UINT_MAX results in:
>
>    cc1: note: -Wvla=0 disables -Wvla.  Use -Wno-vla instead.
>
> and with larger arguments in imposing a limit that's modulo UINT_MAX.
> Although stack size in excess of UINT_MAX bytes is unlikely, it seems
> that such exceedingly large arguments should be handled more gracefully
> (if they cannot be stored in a size_t argument it would be nice give
> a warning).

This seems to be a limitation of the option handling machinery.  For 
example, by the time type I get a hold of the passed value in 
c_common_handle_option(), we don't even know there was an overflow.

That is, when you pass -Walloca-larger-than=4294967296 (MAX_UINT + 1), 
c_common_handle_option() just sees value == 0.  However, passing 
MAX_UINT is handled prior to c_common_handle_option in the generic 
machinery:

./cc1 a.c -fdump-tree-all-vops-alias-details-graph -quiet 
-Walloca-larger-than=4294967295 -O2

cc1: error: argument to ‘-Walloca-larger-than=’ should be a non-negative 
integer

This is a problem with the generic machinery so I would prefer someone 
file a bugzilla report :), as this affects other options.

For that matter, "type" in c_common_handle_option() seems to be of 
"signed int", whereas the option was defined in c.opt as UInteger.  That 
should probably be addressed as well.

Good catch, though.

> 8) Finally, I think it would be helpful to provide information about
> the values GCC used to decide to issue the warning, especially when
> optimization is involved.  In many cases it will not be immediately
> obvious how GCC determined that an alloca argument is too big, or
> what the limit even is (for instance, when the -Walloca=N option
> is set via a #pragma GCC diagnostic far away from the call).
> Mentioning both the argument value or range and the threshold for
> the warning will help make it clear.

Ughhh...you're making me write user friendly stuff.  The reason I got 
into compilers was so I wouldn't have to deal with the user :).

  if (n < 2000)
    {
    p = __builtin_alloca (n);
    f (p);
    }

./cc1 a.c -fdump-tree-all-vops-alias-details-graph -quiet -I/tmp 
-Walloca-larger-than=100 -O2
a.c: In function ‘g2’:
a.c:9:7: warning: argument to alloca may be too large 
[-Walloca-larger-than=]
     p = __builtin_alloca (n);
     ~~^~~~~~~~~~~~~~~~~~~~~~
a.c:9:7: note: limit is '100' bytes, but argument may be '1999'

Happy? :-)

Of course, the non-obvious cases may get ranges you may not expect. 
This should get better with improved range info.

> Martin
>
> PS I also ran into a couple internal compiler errors with the test
> cases below:
>
>    void f0 (void*);
>
>    void f1 (int a, int b)
>    {
>      int x [a][b];
>      f0 (x);
>    }

Fixed.  There were issues with multi-dimensional VLAs, but mainly it was 
me being overly smart with VLA inner types.

>    0x11def63 tree_to_uhwi(tree_node const*)
>    /src/gcc/walloca/gcc/tree.c:7339
>    ...
>
>    void f0 (void*);
>
>    void f1 (unsigned char a)
>    {
>      void *p = __builtin_alloca_with_align (a, 8);
>      f0 (p);
>    }

Fixed now that the distinction between direct 
__builtin_alloca_with_align calls and VLAs has been addressed.

Phew.  I think that's it.

Tested on x86-64 Linux.

What do you think?

Aldy

Attachment: curr
Description: Text document