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

[Aldy Hernandez]

On 08/01/2016 04:35 PM, Joseph Myers wrote:

On Tue, 19 Jul 2016, Aldy Hernandez wrote:


+  // 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.


Given this ...


+   case ALLOCA_IN_LOOP:
+ warning_at (loc, wcode,
+ is_vla ? "use of variable-length array "
+ "within a loop"
+ : "use of % within a loop");
+ break;


  ... why is there a VLA case for this diagnostic at all?  I'd expect an
assertion that only the alloca case can reach this diagnostic.



Indeed.  Fixed.


Also, if the format string for a diagnostic function is a ? : conditional
expression you need to mark up each half with G_() so that both halves are
properly extracted for translation.  This applies to lots of diagnostics
in this patch.


Hugh, I didn't know about the G_() magic for marking format strings. 
Fixed everywhere.


OK?
gcc/

* Makefile.in (OBJS): Add gimple-ssa-warn-alloca.o.
* passes.def: Add two instances of pass_walloca.
* tree-pass.h (make_pass_walloca): New.
* gimple-ssa-warn-walloca.c: New file.
* doc/invoke.texi: Document -Walloca, -Walloca-larger-than=, and
-Wvla-larger-than= options.

gcc/c-family/

* c.opt (Walloca): New.
(Walloca-larger-than=): New.
(Wvla-larger-than=): New.

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 7a0160f..210153a 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1296,6 +1296,7 @@ OBJS = \
gimple-ssa-nonnull-compare.o \
gimple-ssa-split-paths.o \
gimple-ssa-strength-reduction.o \
+   gimple-ssa-warn-alloca.o \
gimple-streamer-in.o \
gimple-streamer-out.o \
gimple-walk.o \
diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index c11e7e7..6e82fc8 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -376,6 +376,16 @@ c_common_handle_option (size_t scode, const char *arg, int 
value,
   cpp_opts->warn_num_sign_change = value;
   break;
 
+case OPT_Walloca_larger_than_:
+  if (!value)
+   inform (loc, "-Walloca-larger-than=0 is meaningless");
+  break;
+
+case OPT_Wvla_larger_than_:
+  if (!value)
+   inform (loc, "-Wvla-larger-than=0 is meaningless");
+  break;
+
 case OPT_Wunknown_pragmas:
   /* Set to greater than 1, so that even unknown pragmas in
 system headers will be warned about.  */
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index a5358ed..c9caffe 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -275,6 +275,16 @@ Wall
 C ObjC C++ ObjC++ Warning
 Enable most warning messages.
 
+Walloca
+C ObjC C++ ObjC++ Var(warn_alloca) Warning
+Warn on any use of alloca.
+
+Walloca-larger-than=
+C ObjC C++ ObjC++ Var(warn_alloca_limit) Warning Joined RejectNegative UInteger
+-Walloca-larger-than= Warn on unbounded uses of
+alloca, and on bounded uses of alloca whose bound can be larger than
+ bytes.
+
 Warray-bounds
 LangEnabledBy(C ObjC C++ ObjC++,Wall)
 ; in common.opt
@@ -980,6 +990,12 @@ Wvla
 C ObjC C++ ObjC++ Var(warn_vla) Init(-1) Warning
 Warn if a variable length array is used.
 
+Wvla-larger-than=
+C ObjC C++ ObjC++ Var(warn_vla_limit) Warning Joined RejectNegative UInteger
+-Wvla-larger-than= Warn on unbounded uses of variable-length arrays, 
and
+on bounded uses of variable-length arrays whose bound can be
+larger than  bytes.
+
 Wvolatile-register-var
 C ObjC C++ ObjC++ Var(warn_volatile_register_var) Warning LangEnabledBy(C ObjC 
C++ ObjC++,Wall)
 Warn when a register variable is declared volatile.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 22001f9..82fb89e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -255,6 +255,7 @@ Objective-C and Objective-C++ Dialects}.
 @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -Wpedantic @gol
 -pedantic-errors @gol
 -w  -Wextra  -Wall  -Waddress  -Waggregate-return  @gol
+-Walloca -Walloca-larger-than=@var{n} @gol
 -Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
 -Wno-attributes -Wbool-compare -Wno-builtin-macro-redefined @gol
 -Wc90-c99-compat -Wc99-c11-compat @gol
@@ -311,7 +312,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wunused-const-variable -Wunused-const-variable=@var{n} @gol
 -Wunused-but-set-parameter -Wunused-but-set-variable @gol
 -Wuseless-cast -Wvariadic-macros -Wvector-operation-performance @gol
--Wvla -Wvolatile-register-var  -Wwrite-strings @gol
+-Wvla -Wvla-larger-than=@var{n} -Wvolatile-register-var  -Wwrite-strings @gol
 -Wzero-as-null-pointer-constant -Whsa}
 
 @item C and Objective-C-only Warning Options
@@ -4666,6 +4667,61 @@ annotations.
 Warn about overriding virtual functions that are not marked with the override
 keyword.
 

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

[Joseph Myers]

On Tue, 19 Jul 2016, Aldy Hernandez wrote:

> +  // 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.

Given this ...

> + case ALLOCA_IN_LOOP:
> +   warning_at (loc, wcode,
> +   is_vla ? "use of variable-length array "
> +   "within a loop"
> +   : "use of % within a loop");
> +   break;

 ... why is there a VLA case for this diagnostic at all?  I'd expect an 
assertion that only the alloca case can reach this diagnostic.

Also, if the format string for a diagnostic function is a ? : conditional 
expression you need to mark up each half with G_() so that both halves are 
properly extracted for translation.  This applies to lots of diagnostics 
in this patch.

-- 
Joseph S. Myers
jos...@codesourcery.com

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

[Aldy Hernandez]

On 07/19/2016 01:47 PM, Jeff Law wrote:

On 07/17/2016 09:52 AM, Manuel López-Ibáñez wrote:

+  if (is_vla)
+gcc_assert (warn_vla_limit > 0);
+  if (!is_vla)
+gcc_assert (warn_alloca_limit > 0);

if-else ? Or perhaps:

Shouldn't really matter, except perhaps in a -O0 compilation.  Though I
think else-if makes it slightly clearer.





My preference would've been the if/else.  The missing else was an oversight.

However, since I really don't care, the last posted patch uses this:

>> gcc_assert (!is_vla || warn_vla_limit > 0);
>> gcc_assert (is_vla || warn_alloca_limit > 0);
> Would be acceptable as well.  I think any of the 3 is fine and leave it
> to Aldy's discretion which to use.
>
> Jeff

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

[Aldy Hernandez]

On 07/19/2016 01:54 PM, Jeff Law wrote:

On 07/19/2016 05:03 AM, Aldy Hernandez wrote:

(Same thing with alloca).  There should be no warning for VLAs,
and for alloca, the warning should say "use of variable-length
array within a loop."  The VRP dump suggests the range information
is available within the loop.  Is the get_range_info() function
not returning the corresponding bounds?


This is a false positive, but there's little we can do with the current
range infrastructure.  The range information becomes less precise the
further down the optimization pipeline we get.  So, even though as far
as *.c.126t.crited1, we still see appropriate range information:

[ ... ]
So I think we can live with the false positive as an XFAIL while we wait
for improved infrastructure.

I will note that you could use a two-stage approach to help with this
kind of issue.  You note the set of potential large allocations early
(before PRE or anyone else messes it up).  Then you allow the other
optimizers to run, then go back and recheck the allocations after the
last optimizer pass.  You end up with

flagged early  && flagged late --> warn
flagged early && ! flagged late -> optimization eliminated the false
positive (which you can optionally issue a diagnostic for)
! flagged early -- never warn

I don't think you strictly need it here, but it's a way to approach some
of these problems where you want to run a warning pass late (to allow
the optimizers to eliminate false positives).


If you feel strongly about this I can certainly do so, but until we get 
better range info, I'd prefer to work on other stuff ;-).


Let me know.

Aldy

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

[Manuel López-Ibáñez]

On 19 July 2016 at 18:47, Jeff Law  wrote:
> On 07/17/2016 09:52 AM, Manuel López-Ibáñez wrote:
>>
>> +  if (is_vla)
>> +gcc_assert (warn_vla_limit > 0);
>> +  if (!is_vla)
>> +gcc_assert (warn_alloca_limit > 0);
>>
>> if-else ? Or perhaps:
>
> Shouldn't really matter, except perhaps in a -O0 compilation.  Though I
> think else-if makes it slightly clearer.

Of course, I mentioned it because of clarity. It was difficult to
distinguish !i versus (i in my screen and I had to stop to read it
again.

Cheers,

Manuel.

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

[Jeff Law]

On 07/19/2016 05:03 AM, Aldy Hernandez wrote:

(Same thing with alloca).  There should be no warning for VLAs,
and for alloca, the warning should say "use of variable-length
array within a loop."  The VRP dump suggests the range information
is available within the loop.  Is the get_range_info() function
not returning the corresponding bounds?


This is a false positive, but there's little we can do with the current
range infrastructure.  The range information becomes less precise the
further down the optimization pipeline we get.  So, even though as far
as *.c.126t.crited1, we still see appropriate range information:

[ ... ]
So I think we can live with the false positive as an XFAIL while we wait 
for improved infrastructure.


I will note that you could use a two-stage approach to help with this 
kind of issue.  You note the set of potential large allocations early 
(before PRE or anyone else messes it up).  Then you allow the other 
optimizers to run, then go back and recheck the allocations after the 
last optimizer pass.  You end up with


flagged early  && flagged late --> warn
flagged early && ! flagged late -> optimization eliminated the false 
positive (which you can optionally issue a diagnostic for)

! flagged early -- never warn

I don't think you strictly need it here, but it's a way to approach some 
of these problems where you want to run a warning pass late (to allow 
the optimizers to eliminate false positives).


jeff

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

[Jeff Law]

On 07/17/2016 09:52 AM, Manuel López-Ibáñez wrote:

+  if (is_vla)
+gcc_assert (warn_vla_limit > 0);
+  if (!is_vla)
+gcc_assert (warn_alloca_limit > 0);

if-else ? Or perhaps:
Shouldn't really matter, except perhaps in a -O0 compilation.  Though I 
think else-if makes it slightly clearer.




gcc_assert (!is_vla || warn_vla_limit > 0);
gcc_assert (is_vla || warn_alloca_limit > 0);
Would be acceptable as well.  I think any of the 3 is fine and leave it 
to Aldy's discretion which to use.


Jeff

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

[Aldy Hernandez]

On 07/18/2016 11:14 PM, Martin Sebor wrote:

How does this look?


I think it's 99% there.  You've addressed all of my comments so
far -- thanks for that and for being so patient.  I realize it
would be a lot more efficient to get all the feedback (or as much
of it as possible) up front.  Unfortunately, some things don't get
noticed until round 2 or 3 (or even 4).  Please take this in lieu
of an apology for not spotting the issues below until now(*).


No problem, although I think we're getting to the point of diminishing 
returns with regards to functionality.  It may be best to involve Jeff 
or another global reviewer at this point for a review.  We can address 
any more minor things as a follow-up patch.  (Unless you find any show 
stoppers before then :)).




For this code:

   void f (void*);

   void g (int n)
   {
 int a [n];
 f (a);
   }

-Wvla-larger-than=32 prints:

   warning: argument to variable-length array may be too large
   note: limit is 32 bytes, but argument may be 18446744073709551612

An int argument cannot be that large.  I suspect the printed value
is actually the size of the VLA in bytes when N is -1, truncated
to size_t, rather than the value of the VLA bound.  To avoid
confusion the note should be corrected to say something like:

   note: limit is 32 bytes, but the variable-length array may be
   as large as 18446744073709551612


Note adjusted.



Also, the checker prints false positives for code like:

   void f (void*);

   void g (unsigned x, int *y)
   {
 if (1000 < x) return;

 while (*y) {
   char a [x];
   f (a);
 }
   }

With -Wvla-larger-than=1000 and greater it prints:

   warning: unbounded use of variable-length array

(Same thing with alloca).  There should be no warning for VLAs,
and for alloca, the warning should say "use of variable-length
array within a loop."  The VRP dump suggests the range information
is available within the loop.  Is the get_range_info() function
not returning the corresponding bounds?


This is a false positive, but there's little we can do with the current 
range infrastructure.  The range information becomes less precise the 
further down the optimization pipeline we get.  So, even though as far 
as *.c.126t.crited1, we still see appropriate range information:


   # RANGE [0, 1000] NONZERO 1023
   _10 = (sizetype) x_3(D);
...
   a.1_12 = __builtin_alloca_with_align (_10, 8);

The PRE pass cleans things up in such a way that we end up with:

   :
   if (x_3(D) > 1000)
 goto ;
   else
 goto ;
...
   :
   # VUSE <.MEM_2(D)>
   _16 = *y_1(D);
   if (_16 != 0)
 goto ;
   else
 goto ;

   :

   :
   # <-NO RANGE INFO->
   _4 = (sizetype) x_3(D);

...

a.1_12 = __builtin_alloca_with_align (_4, 8);

The -Walloca pass comes after PRE, which means we no longer have any 
range information for _4, and chasing the IL to glean this information 
would be fragile at best.  We will just have to live with this until we 
have better pervasive range information.


Updated patch tested on x86-64 Linux.

Aldy

gcc/

* Makefile.in (OBJS): Add gimple-ssa-warn-walloca.o.
* passes.def: Add two instances of pass_walloca.
* tree-pass.h (make_pass_walloca): New.
* gimple-ssa-warn-walloca.c: New file.
* opts.c (finish_options): Warn when using -Wvla-larger-than= and
-Walloca-larger-than= without -O2 or greater.
* doc/invoke.texi: Document -Walloca, -Walloca-larger-than=, and
-Wvla-larger-than= options.

gcc/c-family/

* c.opt (Walloca): New.
(Walloca-larger-than=): New.
(Wvla-larger-than=): New.

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 776f6d7..2a13b8f 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1284,6 +1284,7 @@ OBJS = \
gimple-ssa-nonnull-compare.o \
gimple-ssa-split-paths.o \
gimple-ssa-strength-reduction.o \
+   gimple-ssa-warn-alloca.o \
gimple-streamer-in.o \
gimple-streamer-out.o \
gimple-walk.o \
diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index ff6339c..dc2be2d 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -376,6 +376,16 @@ c_common_handle_option (size_t scode, const char *arg, int 
value,
   cpp_opts->warn_num_sign_change = value;
   break;
 
+case OPT_Walloca_larger_than_:
+  if (!value)
+   inform (loc, "-Walloca-larger-than=0 is meaningless");
+  break;
+
+case OPT_Wvla_larger_than_:
+  if (!value)
+   inform (loc, "-Wvla-larger-than=0 is meaningless");
+  break;
+
 case OPT_Wunknown_pragmas:
   /* Set to greater than 1, so that even unknown pragmas in
 system headers will be warned about.  */
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 83fd84c..1d4ebf0 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -275,6 +275,16 @@ Wall
 C ObjC C++ ObjC++ Warning
 Enable most warning messages.
 
+Walloca
+C ObjC C++ 

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

[Martin Sebor]

How does this look?


I think it's 99% there.  You've addressed all of my comments so
far -- thanks for that and for being so patient.  I realize it
would be a lot more efficient to get all the feedback (or as much
of it as possible) up front.  Unfortunately, some things don't get
noticed until round 2 or 3 (or even 4).  Please take this in lieu
of an apology for not spotting the issues below until now(*).

For this code:

  void f (void*);

  void g (int n)
  {
int a [n];
f (a);
  }

-Wvla-larger-than=32 prints:

  warning: argument to variable-length array may be too large
  note: limit is 32 bytes, but argument may be 18446744073709551612

An int argument cannot be that large.  I suspect the printed value
is actually the size of the VLA in bytes when N is -1, truncated
to size_t, rather than the value of the VLA bound.  To avoid
confusion the note should be corrected to say something like:

  note: limit is 32 bytes, but the variable-length array may be
  as large as 18446744073709551612

Also, the checker prints false positives for code like:

  void f (void*);

  void g (unsigned x, int *y)
  {
if (1000 < x) return;

while (*y) {
  char a [x];
  f (a);
}
  }

With -Wvla-larger-than=1000 and greater it prints:

  warning: unbounded use of variable-length array

(Same thing with alloca).  There should be no warning for VLAs,
and for alloca, the warning should say "use of variable-length
array within a loop."  The VRP dump suggests the range information
is available within the loop.  Is the get_range_info() function
not returning the corresponding bounds?

Martin

[*] If you want to get me back I invite you (with a bit of
selfishness ;-) to review my -Wformat-length patch.

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

[Aldy Hernandez]

On 07/16/2016 05:07 PM, Martin Sebor wrote:

[Addressed all of Manu's suggestions as well.]


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


I think it's an improvement though I suspect we each have a slightly
different understanding of what the sorry message is meant to be used
for.  It's documented in the Diagnostics Conventions section of the
GCC Coding Conventions as:

   sorry is for correct user input programs but unimplemented
   functionalities.

I take that to mean that it should be used for what is considered
valid user input that cannot be processed because the functionality
is not yet implemented (but eventually will be).  So unless this
case falls into this category I would expect GCC to issue a warning
saying that the options have no effect (or limited effect, whatever
the case may be) without optimization. But maybe I'm not reading
the coding conventions text right.


Technically we could add the functionality later.  I don't know whether 
the new range info work can be made to work with lower optimization 
levels.  But really, I don't care :).  Adjusted to a warning.





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:


Cool.

FWIW, by coincidence I was just educated about the subtle nuances
of quoting in GCC messages in a discussion with David and Manu.
Types, functions, variables, and literals that appear in the source
code should be referenced in diagnostics by using the "%qT", "%qD",
and "%qE" directives so that GCC can add the right quotes and
highlighting.  Enclosing "'%T'" in quotes will not use the same
kind of quotes as "%qT" and won't highlight the type name.


Fixed.



   https://gcc.gnu.org/wiki/DiagnosticsGuidelines


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.


Yes, I understand that VLAs in loops are treated differently than
alloca.  But I don't think this is quite how the logic should work.
I.e., an excessively large VLA should be diagnosed regardless of
whether it's in a loop or outside.  Consider the following case
where with the patch as is, the warning is issued only for one
of the two functions, even though they both allocate a VLA in
excess of the threshold.


Agreed...



   #define FOO(n) if (1) { \
   char a [n]; \
   f (a); \
 } else (void)0

   #define BAR(n) do { \
   char a [n]; \
   f (a); \
 } while (0)

   void f (void*);

   void foo (void)
   {
 int n = 8000;
 FOO (n);// warning with -Wla-larger-than=4000
   }

   void bar (void)
   {
 int n = 8000;
 BAR (n);// no warning
   }


...though it looks like your testcases may get optimized away.

I've added this testcase:

void
f6 (unsigned stuff)
{
  int n = 7000;
  do {
char a[n]; // { dg-warning "variable-length array is too large" }
f0 (a);
  } while (stuff--);
}


I've changed the logic so we warn on large allocas whether they're for 
VLAs or otherwise, but no warning given on VLAs within a loop when we 
know the bounds.





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.


Looks good.

I did notice one minor glitch, though not one caused by your patch.
GCC apparently transforms simple VLAs that are 256 bytes or less
in size into ordinary arrays (i.e., it doesn't call
__builtin_alloca_with_align).  Because of that, specifying
-Wvla-larger-than=N with N less than 256 may not give a warning,
as in the example below.  I suspect there may not be anything
the Walloca pass can do about this so perhaps just mentioning
it in the manual might be enough to avoid bug reports about false
negatives.


Documentation updated.



   void f0 (void*);

   unsigned f1 (void) { return 256; }

   void f2 (void)
   {
 unsigned n = f1 ();
 char a [n];
 f0 (a);
   }

GCC doesn't do the same transformation for alloca calls so the

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

[Aldy Hernandez]

On 07/17/2016 11:52 AM, Manuel López-Ibáñez wrote:

On 15/07/16 18:05, Aldy Hernandez wrote:

+case OPT_Walloca_larger_than_:
+  if (!value)
+inform (loc, "-Walloca-larger-than=0 is meaningless");
+  break;
+
+case OPT_Wvla_larger_than_:
+  if (!value)
+inform (loc, "-Wvla-larger-than=0 is meaningless");
+  break;
+

We don't give similar notes for any of the other Wx-larger-than=
options. If -Wvla-larger-than=0 suppresses a previous
-Wvla-larger-than=, then it doesn't seem meaningless, but a useful thing
to have.


I'm trying to avoid confusing users that may think that 
-Walloca-larger-than=0 means warn on any use of alloca.  That is what 
-Walloca is for.  But really, I don't care.  If you feel strongly about 
it, I can just remove the block of code.


Aldy

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

[Manuel López-Ibáñez]

On 15/07/16 18:05, Aldy Hernandez wrote:

+case OPT_Walloca_larger_than_:
+  if (!value)
+   inform (loc, "-Walloca-larger-than=0 is meaningless");
+  break;
+
+case OPT_Wvla_larger_than_:
+  if (!value)
+   inform (loc, "-Wvla-larger-than=0 is meaningless");
+  break;
+

We don't give similar notes for any of the other Wx-larger-than= options. If 
-Wvla-larger-than=0 suppresses a previous -Wvla-larger-than=, then it doesn't 
seem meaningless, but a useful thing to have.


+  if (is_vla)
+gcc_assert (warn_vla_limit > 0);
+  if (!is_vla)
+gcc_assert (warn_alloca_limit > 0);

if-else ? Or perhaps:

gcc_assert (!is_vla || warn_vla_limit > 0);
gcc_assert (is_vla || warn_alloca_limit > 0);

--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -275,6 +275,15 @@ Wall
 C ObjC C++ ObjC++ Warning
 Enable most warning messages.

+Walloca
+C ObjC C++ ObjC++ Var(warn_alloca) Warning
+
+Walloca-larger-than=
+C ObjC C++ ObjC++ Var(warn_alloca_limit) Warning Joined RejectNegative UInteger
+-Walloca-larger-than= Warn on unbounded uses of
+alloca, and on bounded uses of alloca whose bound can be larger than
+ bytes.

No description for Walloca.

+ if (warn_alloca)
+   warning_at (loc, OPT_Walloca, "use of alloca");
+ continue;

Since alloca is a source code entity, it would be good to quote it using %< %> 
(this hints translators to not translate it).



+ const char *alloca_str
+   = is_vla ? "variable-length array" : "alloca";
+ char buff[WIDE_INT_MAX_PRECISION / 4 + 4];
+ switch (w)
+   {
+   case ALLOCA_OK:
+ break;
+   case ALLOCA_BOUND_MAYBE_LARGE:
+ gcc_assert (assumed_limit != 0);
+ if (warning_at (loc, wcode,
+ "argument to %s may be too large", alloca_str))
+   {
+ print_decu (assumed_limit, buff);
+ inform (loc, "limit is '%u' bytes, but argument may be '%s'",
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+   }
+ break;
+   case ALLOCA_BOUND_DEFINITELY_LARGE:
+ gcc_assert (assumed_limit != 0);
+ if (warning_at (loc, wcode,
+ "argument to %s is too large", alloca_str))
+   {
+ print_decu (assumed_limit, buff);
+ inform (loc, "limit is %u' bytes, but argument is '%s'",
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+   }
+ break;

https://gcc.gnu.org/codingconventions.html#Diagnostics :

All diagnostics should be full sentences without English fragments substituted 
in them, to facilitate translation.


Example:

if (warning_at (loc, wcode,
is_vla ? "argument to variable-length array may be too large"
   : "argument to % may be too large"))

+ print_decu (assumed_limit, buff);
+ inform (loc, "limit is '%u' bytes, but argument may be '%s'",
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+ }

https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Quoting :
Other elements such as numbers that do no refer to numeric constants that 
appear in the source code should not be quoted.


+ warning_at (loc, wcode, "argument to %s may be too large due to "
+ "conversion from '%T' to '%T'",
+ alloca_str, invalid_casted_type, size_type_node);

From the same link:

Text should be quoted by either using the q modifier in a directive such as 
%qE, or by enclosing the quoted text in a pair of %< and %> directives, and 
never by using explicit quote characters. The directives handle the appropriate 
quote characters for each language and apply the correct color or highlighting.


I don't think the above are critical problems, they could be fixed by a follow 
up patch.


Cheers,
Manuel.

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

[Martin Sebor]

I forgot to ask, Aldy:  Have you tried using LTO with the warning?
I haven't managed to get it to warn even though I expected it to.
The test I used is below.

[...more testing...]

After some more testing I discovered that LTO somehow seems to
suppress this and other warnings (including the new -Wformat-length
I've been working on).  I haven't spent any time figuring out why
but I have raised bug 71907 for it.

Thanks
Martin

$ (CC='/build/gcc-walloca/gcc/xgcc -B /build/gcc-walloca/gcc'; 
CFLAGS='-O2 -Wall -Wextra -Wpedantic -Walloca-larger-than=32 -flto'; cat 
x.c && $CC -c x.c && $CC -DMAIN -DN=12345678 -c -o main.o x.c && $CC 
$CFLAGS x.o main.o && gdb -batch -q -ex r -ex bt ./a.out )

extern void f0 (void*, void*);
extern unsigned f1 (void);
extern void f2 (void);

#if MAIN
void f0 (void *p, void *q) {
  __builtin_printf ("p = %p, q = %p, d = %td\n", p, q, (char*)p - 
(char*)q);

}

unsigned f1 (void) { return N; }

int main (void) { f2 (); }

#else

void f2 (void)
{
  void *p = 
  void *q = __builtin_alloca (f1 ());
  f0 (p, q);
}

#endif

Program received signal SIGSEGV, Segmentation fault.
0x00400592 in f2 ()
#0  0x00400592 in f2 ()
#1  0x004005e9 in main ()

On 07/16/2016 03:07 PM, Martin Sebor wrote:

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


I think it's an improvement though I suspect we each have a slightly
different understanding of what the sorry message is meant to be used
for.  It's documented in the Diagnostics Conventions section of the
GCC Coding Conventions as:

   sorry is for correct user input programs but unimplemented
   functionalities.

I take that to mean that it should be used for what is considered
valid user input that cannot be processed because the functionality
is not yet implemented (but eventually will be).  So unless this
case falls into this category I would expect GCC to issue a warning
saying that the options have no effect (or limited effect, whatever
the case may be) without optimization. But maybe I'm not reading
the coding conventions text right.


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:


Cool.

FWIW, by coincidence I was just educated about the subtle nuances
of quoting in GCC messages in a discussion with David and Manu.
Types, functions, variables, and literals that appear in the source
code should be referenced in diagnostics by using the "%qT", "%qD",
and "%qE" directives so that GCC can add the right quotes and
highlighting.  Enclosing "'%T'" in quotes will not use the same
kind of quotes as "%qT" and won't highlight the type name.

   https://gcc.gnu.org/wiki/DiagnosticsGuidelines


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.


Yes, I understand that VLAs in loops are treated differently than
alloca.  But I don't think this is quite how the logic should work.
I.e., an excessively large VLA should be diagnosed regardless of
whether it's in a loop or outside.  Consider the following case
where with the patch as is, the warning is issued only for one
of the two functions, even though they both allocate a VLA in
excess of the threshold.

   #define FOO(n) if (1) { \
   char a [n]; \
   f (a); \
 } else (void)0

   #define BAR(n) do { \
   char a [n]; \
   f (a); \
 } while (0)

   void f (void*);

   void foo (void)
   {
 int n = 8000;
 FOO (n);// warning with -Wla-larger-than=4000
   }

   void bar (void)
   {
 int n = 8000;
 BAR (n);// no warning
   }


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.


Looks good.

I did notice one minor glitch, though not one caused by your patch.
GCC apparently transforms simple VLAs that are 256 bytes or less
in size into ordinary arrays (i.e., it doesn't call
__builtin_alloca_with_align).  Because of that, 

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

[Martin Sebor]

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


I think it's an improvement though I suspect we each have a slightly
different understanding of what the sorry message is meant to be used
for.  It's documented in the Diagnostics Conventions section of the
GCC Coding Conventions as:

  sorry is for correct user input programs but unimplemented
  functionalities.

I take that to mean that it should be used for what is considered
valid user input that cannot be processed because the functionality
is not yet implemented (but eventually will be).  So unless this
case falls into this category I would expect GCC to issue a warning
saying that the options have no effect (or limited effect, whatever
the case may be) without optimization. But maybe I'm not reading
the coding conventions text right.


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:


Cool.

FWIW, by coincidence I was just educated about the subtle nuances
of quoting in GCC messages in a discussion with David and Manu.
Types, functions, variables, and literals that appear in the source
code should be referenced in diagnostics by using the "%qT", "%qD",
and "%qE" directives so that GCC can add the right quotes and
highlighting.  Enclosing "'%T'" in quotes will not use the same
kind of quotes as "%qT" and won't highlight the type name.

  https://gcc.gnu.org/wiki/DiagnosticsGuidelines


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.


Yes, I understand that VLAs in loops are treated differently than
alloca.  But I don't think this is quite how the logic should work.
I.e., an excessively large VLA should be diagnosed regardless of
whether it's in a loop or outside.  Consider the following case
where with the patch as is, the warning is issued only for one
of the two functions, even though they both allocate a VLA in
excess of the threshold.

  #define FOO(n) if (1) { \
  char a [n]; \
  f (a); \
} else (void)0

  #define BAR(n) do { \
  char a [n]; \
  f (a); \
} while (0)

  void f (void*);

  void foo (void)
  {
int n = 8000;
FOO (n);// warning with -Wla-larger-than=4000
  }

  void bar (void)
  {
int n = 8000;
BAR (n);// no warning
  }


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.


Looks good.

I did notice one minor glitch, though not one caused by your patch.
GCC apparently transforms simple VLAs that are 256 bytes or less
in size into ordinary arrays (i.e., it doesn't call
__builtin_alloca_with_align).  Because of that, specifying
-Wvla-larger-than=N with N less than 256 may not give a warning,
as in the example below.  I suspect there may not be anything
the Walloca pass can do about this so perhaps just mentioning
it in the manual might be enough to avoid bug reports about false
negatives.

  void f0 (void*);

  unsigned f1 (void) { return 256; }

  void f2 (void)
  {
unsigned n = f1 ();
char a [n];
f0 (a);
  }

GCC doesn't do the same transformation for alloca calls so the
-Walloca-larger-than warning doesn't have this quirk.


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


I raised bug 71905 for this.


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


I along with untold numbers of users 

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

[Aldy Hernandez]

On 07/11/2016 01:56 PM, Martin Sebor wrote:

On 07/11/2016 09:40 AM, Aldy Hernandez wrote:

On 07/11/2016 11:08 AM, Martin Sebor wrote:


Hey, I'm all for different options.  It would definitely be easier for
me :).  I wast trying really hard to use -Wvla= and keep the current
-Wvla meaning the same.  Though I see your point about using -Wvla* but
using different variables for representing -Wvla and -Wvla=blah.

The easiest thing would be:

-Wvla: Current behavior (warn on everything).

-Wvla-larger-than=: Warn on unbounded VLA and bounded VLAs > .

-Walloca: Warn on every use of alloca (analogous to -Wvla).

-Walloca-larger-than=: Warn on unbounded alloca and bounded allocas
 > .

This seems straightforward and avoids all the overloading you see.
Would
this be ok with everyone?


I like the consistency.

Given the common root with -Wlarger-than=N, what do envision the effect
of setting -Wlarger-than=N to be on the two new options?

FWIW, I tend to view -Wlarger-than= and certainly -Wframe-larger-than=
not warning on alloca larger than N as a defect.  It could be fixed by
simply hooking -Walloca-larger-than= up to it.


I'm not touching any of these independent options.  -Wvla-larger-than=
and -Walloca-larger-than= will be implemented independently of whatever
-Wlarger-than= and -Wframe-larger-than=.  Any problems with these last
two options can be treated indpendently (PRs).


Strictly speaking there is no defect in -Wframe-larger-than= because
it's documented not to warn for alloca or VLAs.  I asked because it
seems like an unnecessary (and IMO undesirable) limitation that could
be easily removed as part of this patch.  Not removing it, OTOH, and
providing a separate solution, feels like highlighting the limitation.
With the new options, users interested in detecting all forms of
excessive stack allocation will be able to do so but not using
a single option.  Instead, they will need to use all three.


I'll address this as a follow-up patch.

Aldy

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

[Aldy Hernandez]

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 

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

[Martin Sebor]

On 07/11/2016 09:40 AM, Aldy Hernandez wrote:

On 07/11/2016 11:08 AM, Martin Sebor wrote:


Hey, I'm all for different options.  It would definitely be easier for
me :).  I wast trying really hard to use -Wvla= and keep the current
-Wvla meaning the same.  Though I see your point about using -Wvla* but
using different variables for representing -Wvla and -Wvla=blah.

The easiest thing would be:

-Wvla: Current behavior (warn on everything).

-Wvla-larger-than=: Warn on unbounded VLA and bounded VLAs > .

-Walloca: Warn on every use of alloca (analogous to -Wvla).

-Walloca-larger-than=: Warn on unbounded alloca and bounded allocas
 > .

This seems straightforward and avoids all the overloading you see. Would
this be ok with everyone?


I like the consistency.

Given the common root with -Wlarger-than=N, what do envision the effect
of setting -Wlarger-than=N to be on the two new options?

FWIW, I tend to view -Wlarger-than= and certainly -Wframe-larger-than=
not warning on alloca larger than N as a defect.  It could be fixed by
simply hooking -Walloca-larger-than= up to it.


I'm not touching any of these independent options.  -Wvla-larger-than=
and -Walloca-larger-than= will be implemented independently of whatever
-Wlarger-than= and -Wframe-larger-than=.  Any problems with these last
two options can be treated indpendently (PRs).


Strictly speaking there is no defect in -Wframe-larger-than= because
it's documented not to warn for alloca or VLAs.  I asked because it
seems like an unnecessary (and IMO undesirable) limitation that could
be easily removed as part of this patch.  Not removing it, OTOH, and
providing a separate solution, feels like highlighting the limitation.
With the new options, users interested in detecting all forms of
excessive stack allocation will be able to do so but not using
a single option.  Instead, they will need to use all three.

Martin

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

[Aldy Hernandez]

On 07/11/2016 11:08 AM, Martin Sebor wrote:


Hey, I'm all for different options.  It would definitely be easier for
me :).  I wast trying really hard to use -Wvla= and keep the current
-Wvla meaning the same.  Though I see your point about using -Wvla* but
using different variables for representing -Wvla and -Wvla=blah.

The easiest thing would be:

-Wvla: Current behavior (warn on everything).

-Wvla-larger-than=: Warn on unbounded VLA and bounded VLAs > .

-Walloca: Warn on every use of alloca (analogous to -Wvla).

-Walloca-larger-than=: Warn on unbounded alloca and bounded allocas
 > .

This seems straightforward and avoids all the overloading you see. Would
this be ok with everyone?


I like the consistency.

Given the common root with -Wlarger-than=N, what do envision the effect
of setting -Wlarger-than=N to be on the two new options?

FWIW, I tend to view -Wlarger-than= and certainly -Wframe-larger-than=
not warning on alloca larger than N as a defect.  It could be fixed by
simply hooking -Walloca-larger-than= up to it.


I'm not touching any of these independent options.  -Wvla-larger-than= 
and -Walloca-larger-than= will be implemented independently of whatever 
-Wlarger-than= and -Wframe-larger-than=.  Any problems with these last 
two options can be treated indpendently (PRs).


Aldy

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

[Martin Sebor]

Hey, I'm all for different options.  It would definitely be easier for
me :).  I wast trying really hard to use -Wvla= and keep the current
-Wvla meaning the same.  Though I see your point about using -Wvla* but
using different variables for representing -Wvla and -Wvla=blah.

The easiest thing would be:

-Wvla: Current behavior (warn on everything).

-Wvla-larger-than=: Warn on unbounded VLA and bounded VLAs > .

-Walloca: Warn on every use of alloca (analogous to -Wvla).

-Walloca-larger-than=: Warn on unbounded alloca and bounded allocas
 > .

This seems straightforward and avoids all the overloading you see. Would
this be ok with everyone?


I like the consistency.

Given the common root with -Wlarger-than=N, what do envision the effect
of setting -Wlarger-than=N to be on the two new options?

FWIW, I tend to view -Wlarger-than= and certainly -Wframe-larger-than=
not warning on alloca larger than N as a defect.  It could be fixed by
simply hooking -Walloca-larger-than= up to it.

  void f (void*);
  void g (void)
  {
void *p = __builtin_alloca (1024);
f (p);
  }

Martin

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

[Jeff Law]

On 07/10/2016 04:09 PM, Martin Sebor wrote:

On 07/08/2016 05:48 AM, Aldy Hernandez wrote:

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.
:-)  Which would be fine with me -- the difference is, we'd be able to 
back up my "programmers can't correctly use alloca" rant from several 
years ago with compiler analysis showing why each particular alloca was 
unsafe.




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'

I like Martin's much better.



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.)
Seems like Aldy ought to add this as a testcase, even if it's XFAIL'd 
for now.




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:

Similarly for the others in your message.



Jeff

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

[Jeff Law]

On 07/08/2016 05:48 AM, Aldy Hernandez wrote:

[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.
Definitely out of scope for this set of changes.BUt it's part of my 
evil plan to squash explicit alloca calls while still allowing its use 
when it's provably safe.  But we're not in that world yet.



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.)
I'll trust your judgment on this -- I kind of like the more precise 
warnings -- if for no other reason than it makes it easier for someone 
looking at legacy code (think glibc) to see why GCC determined their 
alloca call was unsafe.






  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.
Right.  And if the bad guys can get control of "N" in this case, they 
can pass in a negative value and perform a "stack shift" -- essentially 
moving the stack so that it points somewhere else  more "interesting" in 
memory.





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.

Right.  Getting better range information is important in so many ways.



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'd go with a new XFAIL'd test.

Jeff

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

[Manuel López-Ibáñez]

On 11 July 2016 at 11:10, Aldy Hernandez  wrote:
> On 07/10/2016 07:41 PM, Manuel López-Ibáñez wrote:
>>>
>>> +Walloca
>>> +LangEnabledBy(C ObjC C++ ObjC++)
>>> +; in common.opt
>>> +
>>> +Walloca=
>>> +LangEnabledBy(C ObjC C++ ObjC++)
>>> +; in common.opt
>>> +
>>
>>
>> I'm not sure what you think the above means, but this is an invalid use
>> of LangEnabledBy(). (It would be nice if the .awk scripts were able to
>> catch it and give an error.)
>
>
> I was following the practice for -Warray-bounds in c-family/c.opt:
>
> Warray-bounds
> LangEnabledBy(C ObjC C++ ObjC++,Wall)
> ; in common.opt
>
> Warray-bounds=
> LangEnabledBy(C ObjC C++ ObjC++,Wall,1,0)
> ; in common.opt

The ones you quoted mean: For that list of languages, -Warray-bounds
is enabled by -Wall.

https://gcc.gnu.org/onlinedocs/gccint/Option-properties.html#Option-properties

But the entries that you added do not specify an option. It should
give an error by the *.awk scripts that parse .opt files. I'm actually
not sure what the scripts are generating in your case.

> I *thought* you defined the option in common.opt, and narrowed the language
> variants in c-family/c.opt.  ??

No, options that are language specific just need to be defined for the
respective FEs using the respective language flags: Wvla is an example
of that. It doesn't appear in common.opt.

Adding language flags to a common option is just redundant (again,
this is not what LangEnabledBy is doing).

Cheers,

Manuel.

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

[Aldy Hernandez]

On 07/10/2016 07:41 PM, Manuel López-Ibáñez wrote:

+Walloca
+LangEnabledBy(C ObjC C++ ObjC++)
+; in common.opt
+
+Walloca=
+LangEnabledBy(C ObjC C++ ObjC++)
+; in common.opt
+


I'm not sure what you think the above means, but this is an invalid use
of LangEnabledBy(). (It would be nice if the .awk scripts were able to
catch it and give an error.)


I was following the practice for -Warray-bounds in c-family/c.opt:

Warray-bounds
LangEnabledBy(C ObjC C++ ObjC++,Wall)
; in common.opt

Warray-bounds=
LangEnabledBy(C ObjC C++ ObjC++,Wall,1,0)
; in common.opt

...as well as the generic version in common.opt:

Warray-bounds
Common Var(warn_array_bounds) Warning
Warn if an array is accessed out of bounds.

Warray-bounds=
Common Joined RejectNegative UInteger Var(warn_array_bounds) Warning
Warn if an array is accessed out of bounds.

I *thought* you defined the option in common.opt, and narrowed the 
language variants in c-family/c.opt.  ??






+;; warn_vla == 0 for -Wno-vla
+;; warn_vla == -1 for nothing passed.
+;; warn_vla == -2 for -Wvla passed
+;; warn_vla == NUM for -Wvla=NUM
 Wvla
 C ObjC C++ ObjC++ Var(warn_vla) Init(-1) Warning
 Warn if a variable length array is used.

+Wvla=
+C ObjC C++ ObjC++ Var(warn_vla) Warning Joined RejectNegative UInteger
+-Wvla= Warn on unbounded uses of variable-length arrays, and
+warn on bounded uses of variable-length arrays whose bound can be
+larger than  bytes.
+


This overloading of warn_vla seems confusing (as shown by all the places
that require updating). Why not call it Wvla-larger-than= and use a
different warn_vla_larger_than variable? We already have -Wlarger-than=
and -Wframe-larger-than=.


Hey, I'm all for different options.  It would definitely be easier for 
me :).  I wast trying really hard to use -Wvla= and keep the current 
-Wvla meaning the same.  Though I see your point about using -Wvla* but 
using different variables for representing -Wvla and -Wvla=blah.


The easiest thing would be:

-Wvla: Current behavior (warn on everything).

-Wvla-larger-than=: Warn on unbounded VLA and bounded VLAs > .

-Walloca: Warn on every use of alloca (analogous to -Wvla).

-Walloca-larger-than=: Warn on unbounded alloca and bounded allocas 
> .


This seems straightforward and avoids all the overloading you see. 
Would this be ok with everyone?




Using warn_vla_larger_than (even if you wish to keep -Wvla= as the
option name) as a variable distinct from warn_vla would make things
simpler:

-Wvla => warn_vla == 1
-Wno-vla => warn_vla == 0
-Wvla=N => warn_vla_larger_than == N (where N == 0 means disable).

If you wish that -Wno-vla implies -Wvla=0 then you'll need to handle
that manually. I don't think we have support for that in the .opt files.
But that seems far less complex than having a single shared Var().

Cheers,
 Manuel.



Thanks.
Aldy

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

[Manuel López-Ibáñez]

+Walloca
+LangEnabledBy(C ObjC C++ ObjC++)
+; in common.opt
+
+Walloca=
+LangEnabledBy(C ObjC C++ ObjC++)
+; in common.opt
+


I'm not sure what you think the above means, but this is an invalid use of 
LangEnabledBy(). (It would be nice if the .awk scripts were able to catch it 
and give an error.)




+;; warn_vla == 0 for -Wno-vla
+;; warn_vla == -1 for nothing passed.
+;; warn_vla == -2 for -Wvla passed
+;; warn_vla == NUM for -Wvla=NUM
 Wvla
 C ObjC C++ ObjC++ Var(warn_vla) Init(-1) Warning
 Warn if a variable length array is used.

+Wvla=
+C ObjC C++ ObjC++ Var(warn_vla) Warning Joined RejectNegative UInteger
+-Wvla= Warn on unbounded uses of variable-length arrays, and
+warn on bounded uses of variable-length arrays whose bound can be
+larger than  bytes.
+


This overloading of warn_vla seems confusing (as shown by all the places that 
require updating). Why not call it Wvla-larger-than= and use a different 
warn_vla_larger_than variable? We already have -Wlarger-than= and 
-Wframe-larger-than=.


Using warn_vla_larger_than (even if you wish to keep -Wvla= as the option name) 
as a variable distinct from warn_vla would make things simpler:


-Wvla => warn_vla == 1
-Wno-vla => warn_vla == 0
-Wvla=N => warn_vla_larger_than == N (where N == 0 means disable).

If you wish that -Wno-vla implies -Wvla=0 then you'll need to handle that 
manually. I don't think we have support for that in the .opt files. But that 
seems far less complex than having a single shared Var().


Cheers,
Manuel.

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

[Martin Sebor]

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 

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

[Aldy Hernandez]

[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,