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

Reply via email to