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

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

+		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 %<alloca%> 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.

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

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

On 19 July 2016 at 18:47, Jeff Law <law@redhat.com> 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.

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