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

[Martin Sebor <msebor at gmail dot com>]

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 :)
I like the consistency between -Walloca and -Wvla! (And despite
the volume of my remaining comments, the rest of the patch too!

1) Optimization.  Without -O2 GCC prints:

  sorry, unimplemented: -Walloca ignored without -O2

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.

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'

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

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

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);

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);
  }

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.

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

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.

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);
  }

  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);
  }

  x181cb74 alloca_call_type
  /src/gcc/walloca/gcc/gimple-ssa-warn-alloca.c:257
  ...