* PING: new pass to warn on questionable uses of alloca() and VLAs
@ 2016-07-27 9:01 Aldy Hernandez
2016-07-27 9:26 ` Rainer Orth
2016-08-04 16:38 ` Jeff Law
0 siblings, 2 replies; 26+ messages in thread
From: Aldy Hernandez @ 2016-07-27 9:01 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 182 bytes --]
Just in case this got lost in noise, since I know there was a lot of
back and forth between Martin Sebor and I.
This is the last iteration.
Tested on x86-64 Linux.
OK for trunk?
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 34232 bytes --]
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.
* 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 0786fa3..8411bed 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 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 8c70152..017a8f9 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=<number> Warn on unbounded uses of
+alloca, and on bounded uses of alloca whose bound can be larger than
+<number> 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=<number> Warn on unbounded uses of variable-length arrays, and
+on bounded uses of variable-length arrays whose bound can be
+larger than <number> 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 9a4db38..341ebb8 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -254,6 +254,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
@@ -310,7 +311,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
@@ -4660,6 +4661,61 @@ annotations.
Warn about overriding virtual functions that are not marked with the override
keyword.
+@item -Walloca
+@opindex Wno-alloca
+@opindex Walloca
+This option warns on all uses of @code{alloca} in the source.
+
+@item -Walloca-larger-than=@var{n}
+This option warns on calls to @code{alloca} that are not bounded by a
+controlling predicate limiting its size to @var{n} bytes, or calls to
+@code{alloca} where the bound is unknown.
+
+For example, a bounded case of @code{alloca} could be:
+
+@smallexample
+unsigned int n;
+...
+if (n <= 1000)
+ alloca (n);
+@end smallexample
+
+In the above example, passing @code{-Walloca=1000} would not issue a
+warning because the call to @code{alloca} is known to be at most 1000
+bytes. However, if @code{-Walloca=500} was passed, the compiler would
+have emitted a warning.
+
+Unbounded uses, on the other hand, are uses of @code{alloca} with no
+controlling predicate verifying its size. For example:
+
+@smallexample
+stuff ();
+alloca (n);
+@end smallexample
+
+If @code{-Walloca=500} was passed, the above would trigger a warning,
+but this time because of the lack of bounds checking.
+
+Note, that even seemingly correct code involving signed integers could
+cause a warning:
+
+@smallexample
+signed int n;
+...
+if (n < 500)
+ alloca (n);
+@end smallexample
+
+In the above example, @var{n} could be negative, causing a larger than
+expected argument to be implicitly casted into the @code{alloca} call.
+
+This option also warns when @code{alloca} is used in a loop.
+
+This warning is not enabled by @option{-Wall}, and is only active when
+@option{-ftree-vrp} is active (default for @option{-O2} and above).
+
+See also @option{-Wvla-larger-than=@var{n}}.
+
@item -Warray-bounds
@itemx -Warray-bounds=@var{n}
@opindex Wno-array-bounds
@@ -5824,9 +5880,25 @@ moving from a moved-from object, this warning can be disabled.
@item -Wvla
@opindex Wvla
@opindex Wno-vla
-Warn if variable length array is used in the code.
+Warn if a variable-length array is used in the code.
@option{-Wno-vla} prevents the @option{-Wpedantic} warning of
-the variable length array.
+the variable-length array.
+
+@item -Wvla-larger-than=@var{n}
+If this option is used, the compiler will warn on uses of
+variable-length arrays where the size is either unbounded, or bounded
+by an argument that can be larger than @var{n} bytes. This is similar
+to how @option{-Walloca-larger-than=@var{n}} works, but with
+variable-length arrays.
+
+Note that GCC may optimize small variable-length arrays of a known
+value into plain arrays, so this warning may not get triggered for
+such arrays.
+
+This warning is not enabled by @option{-Wall}, and is only active when
+@option{-ftree-vrp} is active (default for @option{-O2} and above).
+
+See also @option{-Walloca-larger-than=@var{n}}.
@item -Wvolatile-register-var
@opindex Wvolatile-register-var
diff --git a/gcc/gimple-ssa-warn-alloca.c b/gcc/gimple-ssa-warn-alloca.c
new file mode 100644
index 0000000..065a72b
--- /dev/null
+++ b/gcc/gimple-ssa-warn-alloca.c
@@ -0,0 +1,517 @@
+/* Warn on problematic uses of alloca and variable length arrays.
+ Copyright (C) 2016 Free Software Foundation, Inc.
+ Contributed by Aldy Hernandez <aldyh@redhat.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "gimple-pretty-print.h"
+#include "diagnostic-core.h"
+#include "fold-const.h"
+#include "gimple-iterator.h"
+#include "tree-ssa.h"
+#include "params.h"
+#include "tree-cfg.h"
+#include "calls.h"
+#include "cfgloop.h"
+
+const pass_data pass_data_walloca = {
+ GIMPLE_PASS,
+ "walloca",
+ OPTGROUP_NONE,
+ TV_NONE,
+ PROP_cfg, // properties_required
+ 0, // properties_provided
+ 0, // properties_destroyed
+ 0, // properties_start
+ 0, // properties_finish
+};
+
+class pass_walloca : public gimple_opt_pass
+{
+public:
+ pass_walloca (gcc::context *ctxt)
+ : gimple_opt_pass(pass_data_walloca, ctxt), first_time_p (false)
+ {}
+ opt_pass *clone () { return new pass_walloca (m_ctxt); }
+ void set_pass_param (unsigned int n, bool param)
+ {
+ gcc_assert (n == 0);
+ first_time_p = param;
+ }
+ virtual bool gate (function *);
+ virtual unsigned int execute (function *);
+
+ private:
+ // Set to TRUE the first time we run this pass on a function.
+ bool first_time_p;
+};
+
+bool
+pass_walloca::gate (function *fun ATTRIBUTE_UNUSED)
+{
+ // The first time this pass is called, it is called before
+ // optimizations have been run and range information is unavailable,
+ // so we can only perform strict alloca checking.
+ if (first_time_p)
+ return warn_alloca != 0;
+
+ return warn_alloca_limit > 0 || warn_vla_limit > 0;
+}
+
+// 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 argument is 0.
+ ALLOCA_ARG_IS_ZERO,
+
+ // Alloca call is unbounded. That is, there is no controlling
+ // predicate for its argument.
+ ALLOCA_UNBOUNDED
+};
+
+// We have a few heuristics up our sleeve to determine if a call to
+// alloca() is within bounds. Try them out and return the type of
+// alloca call this is based on its argument.
+//
+// Given a known argument (ARG) to alloca() and an EDGE (E)
+// calculating said argument, verify that the last statement in the BB
+// in E->SRC is a gate comparing ARG to an acceptable bound for
+// alloca(). See examples below.
+//
+// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
+// in bytes we allow for arg.
+//
+// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
+// set to the bound used to determine this. ASSUMED_LIMIT is only set
+// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
+//
+// Returns the alloca type.
+
+static enum alloca_type
+alloca_call_type_by_arg (tree arg, edge e, unsigned max_size,
+ wide_int *assumed_limit)
+{
+ // All the tests bellow depend on the jump being on the TRUE path.
+ if (!(e->flags & EDGE_TRUE_VALUE))
+ return ALLOCA_UNBOUNDED;
+
+ basic_block bb = e->src;
+ gimple_stmt_iterator gsi = gsi_last_bb (bb);
+ gimple *last = gsi_stmt (gsi);
+ if (!last || gimple_code (last) != GIMPLE_COND)
+ return ALLOCA_UNBOUNDED;
+
+ /* Check for:
+ if (ARG <= N)
+ goto <bb 3>;
+ else
+ goto <bb 4>;
+ <bb 3>:
+ alloca(ARG);
+ */
+ if (gimple_cond_code (last) == LE_EXPR
+ && gimple_cond_lhs (last) == arg)
+ {
+ if (TREE_CODE (gimple_cond_rhs (last)) == INTEGER_CST)
+ {
+ tree rhs = gimple_cond_rhs (last);
+ if (tree_to_uhwi (rhs) > max_size)
+ {
+ *assumed_limit = rhs;
+ return ALLOCA_BOUND_MAYBE_LARGE;
+ }
+ return ALLOCA_OK;
+ }
+ else
+ return ALLOCA_BOUND_UNKNOWN;
+ }
+
+ /* Check for:
+ if (arg .cond. LIMIT) -or- if (LIMIT .cond. arg)
+ alloca(arg);
+
+ Where LIMIT has a bound of unknown range. */
+ tree limit = NULL;
+ if (gimple_cond_lhs (last) == arg)
+ limit = gimple_cond_rhs (last);
+ else if (gimple_cond_rhs (last) == arg)
+ limit = gimple_cond_lhs (last);
+ if (limit && TREE_CODE (limit) == SSA_NAME)
+ {
+ wide_int min, max;
+ value_range_type range_type = get_range_info (limit, &min, &max);
+ if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
+ return ALLOCA_BOUND_UNKNOWN;
+ // FIXME: We could try harder here and handle a possible range
+ // or anti-range. Hopefully the upcoming changes to range info
+ // will give us finer grained info, and we can avoid somersaults
+ // here.
+ }
+
+ return ALLOCA_UNBOUNDED;
+}
+
+// Return TRUE if SSA's definition is a cast from a signed type.
+// If so, set *INVALID_CASTED_TYPE to the signed type.
+
+static bool
+cast_from_signed_p (tree ssa, tree *invalid_casted_type)
+{
+ gimple *def = SSA_NAME_DEF_STMT (ssa);
+ if (def
+ && !gimple_nop_p (def)
+ && gimple_assign_cast_p (def)
+ && !TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def))))
+ {
+ *invalid_casted_type = TREE_TYPE (gimple_assign_rhs1 (def));
+ return true;
+ }
+ return false;
+}
+
+// Return TURE if X has a maximum range of MAX, basically covering the
+// entire domain, in which case it's no range at all.
+
+static bool
+is_max (tree x, wide_int max)
+{
+ return wi::max_value (TREE_TYPE (x)) == max;
+}
+
+// Analyze the alloca call in STMT and return an `enum alloca_type'
+// explaining what type of alloca it is. IS_VLA is set if the alloca
+// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
+//
+// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
+// set to the bound used to determine this. ASSUMED_LIMIT is only set
+// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
+//
+// If the alloca call may be too large because of a cast from a signed
+// type to an unsigned type, set *INVALID_CASTED_TYPE to the
+// problematic signed type.
+
+static enum alloca_type
+alloca_call_type (gimple *stmt, bool is_vla, wide_int *assumed_limit,
+ tree *invalid_casted_type)
+{
+ gcc_assert (gimple_alloca_call_p (stmt));
+ tree len = gimple_call_arg (stmt, 0);
+ enum alloca_type w = ALLOCA_UNBOUNDED;
+ wide_int min, max;
+
+ gcc_assert (!is_vla || warn_vla_limit > 0);
+ gcc_assert (is_vla || warn_alloca_limit > 0);
+
+ // Adjust warn_alloca_max_size for VLAs, by taking the underlying
+ // type into account.
+ unsigned HOST_WIDE_INT max_size;
+ if (is_vla)
+ max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
+ else
+ max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
+
+ // Check for the obviously bounded case.
+ if (TREE_CODE (len) == INTEGER_CST)
+ {
+ if (tree_to_uhwi (len) > max_size)
+ {
+ *assumed_limit = len;
+ return ALLOCA_BOUND_DEFINITELY_LARGE;
+ }
+ if (integer_zerop (len))
+ return ALLOCA_ARG_IS_ZERO;
+ w = ALLOCA_OK;
+ }
+ else if (TREE_CODE (len) != SSA_NAME)
+ return ALLOCA_UNBOUNDED;
+ // Check the range info if available.
+ else
+ {
+ if (value_range_type range_type = get_range_info (len, &min, &max))
+ {
+ if (range_type == VR_RANGE)
+ {
+ if (wi::leu_p (max, max_size))
+ w = ALLOCA_OK;
+ else if (is_max (len, max))
+ {
+ // A cast may have created a range we don't care
+ // about. For instance, a cast from 16-bit to
+ // 32-bit creates a range of 0..65535, even if there
+ // is not really a determinable range in the
+ // underlying code. In this case, look through the
+ // cast at the original argument, and fall through
+ // to look at other alternatives.
+ gimple *def = SSA_NAME_DEF_STMT (len);
+ if (gimple_assign_cast_p (def))
+ len = gimple_assign_rhs1 (def);
+ }
+ else
+ {
+ /* If `len' is merely a cast that is being
+ calculated right before the call to alloca, look
+ at the range for the original value.
+
+ This avoids the cast creating a range where the
+ original expression did not have a range:
+
+ # RANGE [0, 18446744073709551615] NONZERO 4294967295
+ _2 = (long unsigned int) n_7(D);
+ p_9 = __builtin_alloca (_2);
+
+ The correct thing would've been for the user to
+ use size_t, which in the case above would've been
+ 'long unsigned int', and everything would've
+ worked. But we have to catch cases where the
+ user is using some other compatible type for the
+ call argument to alloca (say unsigned short). */
+ gimple *def = SSA_NAME_DEF_STMT (len);
+ if (gimple_assign_cast_p (def))
+ {
+ len = gimple_assign_rhs1 (def);
+ range_type = get_range_info (len, &min, &max);
+ }
+
+ if (range_type != VR_VARYING && is_max (len, max))
+ {
+ // Treat a max of the entire domain as if it had no
+ // range info, and fall through the try other
+ // alternatives.
+ }
+ else
+ {
+ *assumed_limit = max;
+ return ALLOCA_BOUND_MAYBE_LARGE;
+ }
+ }
+ }
+ else if (range_type == VR_ANTI_RANGE)
+ {
+ // There may be some wrapping around going on. Catch it
+ // with this heuristic. Hopefully, this VR_ANTI_RANGE
+ // nonsense will go away, and we won't have to catch the
+ // sign conversion problems with this crap.
+ if (cast_from_signed_p (len, invalid_casted_type))
+ return ALLOCA_CAST_FROM_SIGNED;
+
+ // Fall thru and try other things.
+ }
+ else if (range_type == VR_VARYING)
+ {
+ // No easily determined range. Try other things.
+ }
+ }
+ }
+
+ // If we couldn't find anything, try a few heuristics for things we
+ // can easily determine. Check these misc cases but only accept
+ // them if all predecessors have a known bound.
+ basic_block bb = gimple_bb (stmt);
+ if (w == ALLOCA_UNBOUNDED)
+ {
+ w = ALLOCA_OK;
+ for (unsigned ix = 0; ix < EDGE_COUNT (bb->preds); ix++)
+ {
+ enum alloca_type w
+ = alloca_call_type_by_arg (len, EDGE_PRED (bb, ix), max_size,
+ assumed_limit);
+ if (w != ALLOCA_OK)
+ return w;
+ }
+ }
+
+ return w;
+}
+
+// Return TRUE if the alloca call in STMT is in a loop, otherwise
+// return FALSE. As an exception, ignore alloca calls for VLAs that
+// occur in a loop since those will be cleaned up when they go out of
+// scope.
+
+static bool
+in_loop_p (bool is_vla, gimple *stmt)
+{
+ basic_block bb = gimple_bb (stmt);
+ if (bb->loop_father
+ // ?? Functions with no loops get a loop_father? I
+ // don't get it. The following conditional seems to do
+ // the trick to exclude such nonsense.
+ && bb->loop_father->header != ENTRY_BLOCK_PTR_FOR_FN (cfun))
+ {
+ // 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.
+ tree fndecl = gimple_call_fn (stmt);
+ while (TREE_CODE (fndecl) == ADDR_EXPR)
+ fndecl = TREE_OPERAND (fndecl, 0);
+ if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+ && is_vla
+ && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_ALLOCA_WITH_ALIGN)
+ return false;
+
+ return true;
+ }
+ return false;
+}
+
+unsigned int
+pass_walloca::execute (function *fun)
+{
+ basic_block bb;
+ FOR_EACH_BB_FN (bb, fun)
+ {
+ for (gimple_stmt_iterator si = gsi_start_bb (bb); !gsi_end_p (si);
+ gsi_next (&si))
+ {
+ gimple *stmt = gsi_stmt (si);
+ location_t loc = gimple_location (stmt);
+
+ if (!gimple_alloca_call_p (stmt))
+ continue;
+ gcc_assert (gimple_call_num_args (stmt) >= 1);
+
+ bool is_vla = gimple_alloca_call_p (stmt)
+ && gimple_call_alloca_for_var_p (as_a <gcall *> (stmt));
+
+ // Strict mode whining for VLAs is handled by the front-end,
+ // so we can safely ignore this case. Also, ignore VLAs if
+ // the user doesn't care about them.
+ if (is_vla
+ && (warn_vla > 0 || !warn_vla_limit))
+ continue;
+
+ if (!is_vla && (warn_alloca || !warn_alloca_limit))
+ {
+ if (warn_alloca)
+ warning_at (loc, OPT_Walloca, "use of %<alloca%>");
+ continue;
+ }
+
+ wide_int assumed_limit
+ = wi::to_wide (integer_zero_node,
+ TYPE_PRECISION (size_type_node));
+ tree invalid_casted_type = NULL;
+ enum alloca_type w = alloca_call_type (stmt, is_vla, &assumed_limit,
+ &invalid_casted_type);
+
+ // Even if we think the alloca call is OK, make sure it's
+ // not in a loop.
+ if (w == ALLOCA_OK && in_loop_p (is_vla, stmt))
+ w = ALLOCA_IN_LOOP;
+
+ enum opt_code wcode
+ = is_vla ? OPT_Wvla_larger_than_ : OPT_Walloca_larger_than_;
+ 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,
+ 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 as large as %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,
+ is_vla ? "argument to variable-length array "
+ "is too large"
+ : "argument to %<alloca%> is too large"))
+ {
+ 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;
+ case ALLOCA_BOUND_UNKNOWN:
+ warning_at (loc, wcode,
+ is_vla ? "variable-length array bound is unknown"
+ : "%<alloca%> bound is unknown");
+ break;
+ case ALLOCA_UNBOUNDED:
+ warning_at (loc, wcode,
+ is_vla ? "unbounded use of variable-length array"
+ : "unbounded use of %<alloca%>");
+ break;
+ case ALLOCA_IN_LOOP:
+ warning_at (loc, wcode,
+ is_vla ? "use of variable-length array "
+ "within a loop"
+ : "use of %<alloca%> within a loop");
+ break;
+ case ALLOCA_CAST_FROM_SIGNED:
+ gcc_assert (invalid_casted_type != NULL_TREE);
+ warning_at (loc, wcode,
+ is_vla ? "argument to variable-length array "
+ "may be too large due to "
+ "conversion from %qT to %qT"
+ : "argument to %<alloca%> may be too large due to "
+ "conversion from %qT to %qT",
+ invalid_casted_type, size_type_node);
+ break;
+ case ALLOCA_ARG_IS_ZERO:
+ warning_at (loc, wcode,
+ is_vla ? "argument to variable-length array is zero"
+ : "argument to %<alloca%> is zero");
+ break;
+ default:
+ gcc_unreachable ();
+ }
+ }
+ }
+ return 0;
+}
+
+gimple_opt_pass *
+make_pass_walloca (gcc::context *ctxt)
+{
+ return new pass_walloca (ctxt);
+}
diff --git a/gcc/passes.def b/gcc/passes.def
index 3647e90..591add2 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. If not see
NEXT_PASS (pass_warn_function_return);
NEXT_PASS (pass_expand_omp);
NEXT_PASS (pass_build_cgraph_edges);
+ NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
TERMINATE_PASS_LIST (all_lowering_passes)
/* Interprocedural optimization passes. */
@@ -303,6 +304,7 @@ along with GCC; see the file COPYING3. If not see
NEXT_PASS (pass_simduid_cleanup);
NEXT_PASS (pass_lower_vector_ssa);
NEXT_PASS (pass_cse_reciprocals);
+ NEXT_PASS (pass_walloca, /*strict_mode_p=*/false);
NEXT_PASS (pass_reassoc, false /* insert_powi_p */);
NEXT_PASS (pass_strength_reduction);
NEXT_PASS (pass_split_paths);
diff --git a/gcc/testsuite/gcc.dg/Walloca-1.c b/gcc/testsuite/gcc.dg/Walloca-1.c
new file mode 100644
index 0000000..34a20c3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-1.c
@@ -0,0 +1,63 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+#define alloca __builtin_alloca
+
+typedef __SIZE_TYPE__ size_t;
+extern size_t strlen(const char *);
+
+extern void useit (char *);
+
+int num;
+
+void foo1 (size_t len, size_t len2, size_t len3)
+{
+ int i;
+
+ for (i=0; i < 123; ++i)
+ {
+ char *s = alloca (566); /* { dg-warning "'alloca' within a loop" } */
+ useit (s);
+ }
+
+ char *s = alloca (123);
+ useit (s); // OK, constant argument to alloca
+
+ s = alloca (num); // { dg-warning "large due to conversion" }
+ useit (s);
+
+ s = alloca(90000); /* { dg-warning "is too large" } */
+ useit (s);
+
+ if (len < 2000)
+ {
+ s = alloca(len); // OK, bounded
+ useit (s);
+ }
+
+ if (len + len2 < 2000) // OK, bounded
+ {
+ s = alloca(len + len2);
+ useit (s);
+ }
+
+ if (len3 <= 2001)
+ {
+ s = alloca(len3); /* { dg-warning "may be too large" } */
+ useit(s);
+ }
+}
+
+void foo2 (__SIZE_TYPE__ len)
+{
+ // Test that a direct call to __builtin_alloca_with_align is not confused
+ // with a VLA.
+ void *p = __builtin_alloca_with_align (len, 8); // { dg-warning "unbounded use of 'alloca'" }
+ useit (p);
+}
+
+void foo3 (unsigned char a)
+{
+ if (a == 0)
+ useit (__builtin_alloca (a)); // { dg-warning "argument to 'alloca' is zero" }
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-2.c b/gcc/testsuite/gcc.dg/Walloca-2.c
new file mode 100644
index 0000000..284b34e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-2.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+void f (void *);
+
+void
+g1 (int n)
+{
+ void *p;
+ if (n > 0 && n < 2000)
+ p = __builtin_alloca (n);
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
+
+void
+g2 (int n)
+{
+ void *p;
+ if (n < 2000)
+ p = __builtin_alloca (n); // { dg-warning "large due to conversion" }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
+
+void
+g3 (int n)
+{
+ void *p;
+ if (n > 0 && n < 3000)
+ {
+ p = __builtin_alloca (n); // { dg-warning "'alloca' may be too large" }
+ // { dg-message "note:.*argument may be as large as 2999" "note" { target *-*-* } 34 }
+ }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-3.c b/gcc/testsuite/gcc.dg/Walloca-3.c
new file mode 100644
index 0000000..5345197
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-3.c
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+void f (void *);
+
+__SIZE_TYPE__ LIMIT;
+
+// Warn when there is an alloca bound, but it is an unknown bound.
+
+void
+g1 (__SIZE_TYPE__ n)
+{
+ void *p;
+ if (n < LIMIT)
+ p = __builtin_alloca (n); // { dg-warning "'alloca' bound is unknown" }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
+
+// Similar to the above, but do not get confused by the upcast.
+
+unsigned short SHORT_LIMIT;
+void
+g2 (unsigned short n)
+{
+ void *p;
+ if (n < SHORT_LIMIT)
+ p = __builtin_alloca (n); // { dg-warning "'alloca' bound is unknown" }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-4.c b/gcc/testsuite/gcc.dg/Walloca-4.c
new file mode 100644
index 0000000..d96cc4e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-4.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=5000 -O2" } */
+/* { dg-xfail-if "Currently broken but Andrew's work should fix this" { *-*-* } } */
+
+// Should be another variant of Walloca-7.c.
+// This should not warn, as we have a known bound within limits.
+
+ char *
+ _i18n_number_rewrite (char *w, char *rear_ptr)
+{
+
+ char *src;
+ _Bool
+ use_alloca = (((rear_ptr - w) * sizeof (char)) < 4096U);
+ if (use_alloca)
+ src = (char *) __builtin_alloca ((rear_ptr - w) * sizeof (char));
+ else
+ src = (char *) __builtin_malloc ((rear_ptr - w) * sizeof (char));
+ return src;
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-5.c b/gcc/testsuite/gcc.dg/Walloca-5.c
new file mode 100644
index 0000000..5ed1171
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-5.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=123 -O2" } */
+/* { dg-xfail-if "Currently broken but Andrew's work should fix this" { *-*-* } } */
+
+/* The argument to alloca ends up having a range of 0..MAXINT(32bits),
+ so we think we have a range because of the upcast. Consequently,
+ we warn with "alloca may be too large", but we should technically
+ warn with "unbounded use of alloca".
+
+ We currently drill through casts to figure this stuff out, but we
+ get confused because it's not just a cast. It's a cast, plus a
+ multiply.
+
+ <bb 2>:
+ # RANGE [0, 4294967295] NONZERO 4294967295
+ _1 = (long unsigned int) something_4(D);
+ # RANGE [0, 34359738360] NONZERO 34359738360
+ _2 = _1 * 8;
+ _3 = __builtin_alloca (_2);
+
+ I don't know whether it's even worth such fine-grained warnings.
+ Perhaps we should generically warn everywhere with "alloca may be
+ too large".
+
+ I'm hoping that this particular case will be easier to diagnose with
+ Andrew's work. */
+
+void useit(void *);
+void foobar(unsigned int something)
+{
+ useit(__builtin_alloca (something * sizeof (const char *))); // { dg-warning "unbounded use of alloca" "" { xfail *-*-* } }
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-6.c b/gcc/testsuite/gcc.dg/Walloca-6.c
new file mode 100644
index 0000000..b4d8d41
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-6.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=256 -O2" } */
+/* { dg-xfail-if "Currently broken but Andrew's work should fix this" { *-*-* } } */
+
+void f (void*);
+void g (__SIZE_TYPE__ n)
+{
+ // No warning on this case. Range is easily determinable.
+ if (n > 0 && n < 256)
+ f (__builtin_alloca (n));
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-7.c b/gcc/testsuite/gcc.dg/Walloca-7.c
new file mode 100644
index 0000000..d6581a5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-7.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca -O0" } */
+
+extern void f(void *);
+
+void foo(void)
+{
+ // Test that strict -Walloca works even without optimization.
+ f (__builtin_alloca(500)); // { dg-warning "use of 'alloca'" }
+}
+
+void bar(void)
+{
+ // Test that we warn on alloca() calls, not just __builtin_alloca calls.
+ extern void *alloca(__SIZE_TYPE__);
+ f (alloca (123)); // { dg-warning "use of 'alloca'" }
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-8.c b/gcc/testsuite/gcc.dg/Walloca-8.c
new file mode 100644
index 0000000..0d7c9c4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-8.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+/* { dg-xfail-if "Currently broken but Andrew's work should fix this" { *-*-* } } */
+
+void *p;
+void
+foo (__SIZE_TYPE__ len)
+{
+ if (len < 2000 / sizeof (void *))
+ p = __builtin_alloca (len * sizeof (void *));
+ else
+ p = __builtin_malloc (len * sizeof (void *));
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-1.c b/gcc/testsuite/gcc.dg/Wvla-1.c
new file mode 100644
index 0000000..384c930
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-1.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-Wvla-larger-than=100 -O2" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void useit (char *);
+
+int num;
+
+void test_vlas (size_t num)
+{
+ char str2[num]; /* { dg-warning "unbounded use" } */
+ useit(str2);
+
+ num = 98;
+ for (int i=0; i < 1234; ++i) {
+ char str[num]; // OK, VLA in a loop, but it is a
+ // known size *AND* the compiler takes
+ // care of cleaning up between
+ // iterations with
+ // __builtin_stack_restore.
+ useit(str);
+ }
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-2.c b/gcc/testsuite/gcc.dg/Wvla-2.c
new file mode 100644
index 0000000..96814dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-2.c
@@ -0,0 +1,70 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target stdint_types } */
+/* { dg-options "-O2 -Wvla-larger-than=40" } */
+
+#include <stdint.h>
+
+void f0 (void *);
+void
+f1 (__SIZE_TYPE__ a)
+{
+ if (a <= 10)
+ {
+ // 10 * 4 bytes = 40: OK!
+ uint32_t x[a];
+ f0 (x);
+ }
+}
+
+void
+f2 (__SIZE_TYPE__ a)
+{
+ if (a <= 11)
+ {
+ // 11 * 4 bytes = 44: Not OK.
+ uint32_t x[a]; // { dg-warning "array may be too large" }
+ // { dg-message "note:.*argument may be as large as 44" "note" { target *-*-* } 25 }
+ f0 (x);
+ }
+}
+
+void
+f3 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
+{
+ if (a <= 5 && b <= 3)
+ {
+ // 5 * 3 * 4 bytes = 60: Not OK.
+ uint32_t x[a][b]; // { dg-warning "array may be too large" }
+ f0 (x);
+ }
+}
+
+void
+f4 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
+{
+ if (a <= 5 && b <= 2)
+ {
+ // 5 * 2 * 4 bytes = 40 bytes: OK!
+ uint32_t x[a][b];
+ f0 (x);
+ }
+}
+
+void
+f5 (__SIZE_TYPE__ len)
+{
+ // Test that a direct call to __builtin_alloca_with_align is not
+ // confused with a VLA.
+ void *p = __builtin_alloca_with_align (len, 8);
+ f0 (p);
+}
+
+void
+f6 (unsigned stuff)
+{
+ int n = 7000;
+ do {
+ char a[n]; // { dg-warning "variable-length array is too large" }
+ f0 (a);
+ } while (stuff--);
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-3.c b/gcc/testsuite/gcc.dg/Wvla-3.c
new file mode 100644
index 0000000..5124476
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-3.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca -O2" } */
+
+// Make sure we don't warn on VLA with -Walloca.
+
+void f (void*);
+
+void h1 (unsigned n)
+{
+ int a [n];
+ f (a);
+}
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 36299a6..57b61f4 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -469,6 +469,7 @@ extern simple_ipa_opt_pass *make_pass_ipa_oacc (gcc::context *ctxt);
extern simple_ipa_opt_pass *make_pass_ipa_oacc_kernels (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_gen_hsail (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_nonnull_compare (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_walloca (gcc::context *ctxt);
/* IPA Passes */
extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt);
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-07-27 9:01 PING: new pass to warn on questionable uses of alloca() and VLAs Aldy Hernandez
@ 2016-07-27 9:26 ` Rainer Orth
2016-08-04 16:38 ` Jeff Law
1 sibling, 0 replies; 26+ messages in thread
From: Rainer Orth @ 2016-07-27 9:26 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches
Hi Aldy,
> Just in case this got lost in noise, since I know there was a lot of back
> and forth between Martin Sebor and I.
>
> This is the last iteration.
>
> Tested on x86-64 Linux.
>
> OK for trunk?
>
> 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.
just a nit: the files is called gimple-ssa-warn-alloca.[co].
Rainer
--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-07-27 9:01 PING: new pass to warn on questionable uses of alloca() and VLAs Aldy Hernandez
2016-07-27 9:26 ` Rainer Orth
@ 2016-08-04 16:38 ` Jeff Law
[not found] ` <57B6D2F1.6050709@redhat.com>
2016-08-19 14:59 ` Aldy Hernandez
1 sibling, 2 replies; 26+ messages in thread
From: Jeff Law @ 2016-08-04 16:38 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 07/27/2016 03:01 AM, Aldy Hernandez wrote:
> Just in case this got lost in noise, since I know there was a lot of
> back and forth between Martin Sebor and I.
>
> This is the last iteration.
>
> Tested on x86-64 Linux.
>
> OK for trunk?
>
> curr
>
>
> 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.
> * 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.
As someone already noted, it's gimple-ssa-warn-alloca, not
gimple-ssa-warn-walloca for the ChangeLog entry.
On the nittish side, you're mixing C and C++ comment styles. Choosing
one and sticking with it seems better :-)
>
> +@item -Walloca
> +@opindex Wno-alloca
> +@opindex Walloca
> +This option warns on all uses of @code{alloca} in the source.
> +
> +@item -Walloca-larger-than=@var{n}
> +This option warns on calls to @code{alloca} that are not bounded by a
> +controlling predicate limiting its size to @var{n} bytes, or calls to
> +@code{alloca} where the bound is unknown.
So for each of these little examples, I'd stuff the code into a trivial
function definition and make "n" a parameter. That way it's obvious the
value of "n" comes from a context where we don't initially know its
range, but we may be able to narrow the range due to statements in the
function.
;
> +
> +class pass_walloca : public gimple_opt_pass
> +{
> +public:
> + pass_walloca (gcc::context *ctxt)
> + : gimple_opt_pass(pass_data_walloca, ctxt), first_time_p (false)
> + {}
> + opt_pass *clone () { return new pass_walloca (m_ctxt); }
> + void set_pass_param (unsigned int n, bool param)
> + {
> + gcc_assert (n == 0);
> + first_time_p = param;
> + }
ISTM that you're using "first_time_p" here, but in passes.def you refer
to this parameter as "strict_mode_p" in comments.
ie:
+ NEXT_PASS (pass_walloca, /*strict_mode_p=*/false);
I'd just drop the /*strict_mode_p*/ comment in both places it appears in
your patch's change to passes.def. I think we've generally frowned on
those embedded comments, even though some have snuck in.
> +
> +// We have a few heuristics up our sleeve to determine if a call to
> +// alloca() is within bounds. Try them out and return the type of
> +// alloca call this is based on its argument.
> +//
> +// Given a known argument (ARG) to alloca() and an EDGE (E)
> +// calculating said argument, verify that the last statement in the BB
> +// in E->SRC is a gate comparing ARG to an acceptable bound for
> +// alloca(). See examples below.
> +//
> +// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
> +// in bytes we allow for arg.
> +//
> +// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
> +// set to the bound used to determine this. ASSUMED_LIMIT is only set
> +// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
> +//
> +// Returns the alloca type.
> +
> +static enum alloca_type
> +alloca_call_type_by_arg (tree arg, edge e, unsigned max_size,
> + wide_int *assumed_limit)
So I wonder if you ought to have a structure here for the return value
which contains the alloca type and assumed limit. I know in the past we
avoided aggregate returns, but these days that doesn't seem necessary.
Seems cleaner than having a return value and output parameters.
> +{
> + // All the tests bellow depend on the jump being on the TRUE path.
> + if (!(e->flags & EDGE_TRUE_VALUE))
> + return ALLOCA_UNBOUNDED;
Seems like a fairly arbitrary and undesirable limitation. Couldn't the
developer just have easily written
if (arg > N>
x = malloc (...)
else
x = alloca (...)
It also seems like you'd want to handle the set of LT/LE/GT/GE rather
than just LE. Or is it the case that we always canonicalize LT into LE
by adjusting the constant (I vaguely remember running into that in RTL,
so it's entirely possible and there'd likely be a canonicalization of
GT/GE as well).
It also seems that once Andrew's infrastructure is in place this becomes
dead code as we can just ask for the range at a point in the program,
including for each incoming edge. You might want a comment to that effect.
> +
> + /* Check for:
> + if (arg .cond. LIMIT) -or- if (LIMIT .cond. arg)
> + alloca(arg);
> +
> + Where LIMIT has a bound of unknown range. */
> + tree limit = NULL;
> + if (gimple_cond_lhs (last) == arg)
> + limit = gimple_cond_rhs (last);
> + else if (gimple_cond_rhs (last) == arg)
> + limit = gimple_cond_lhs (last);
> + if (limit && TREE_CODE (limit) == SSA_NAME)
> + {
> + wide_int min, max;
> + value_range_type range_type = get_range_info (limit, &min, &max);
> + if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
> + return ALLOCA_BOUND_UNKNOWN;
> + // FIXME: We could try harder here and handle a possible range
> + // or anti-range. Hopefully the upcoming changes to range info
> + // will give us finer grained info, and we can avoid somersaults
> + // here.
Ah, can't you set *assumed_limit here? It's just a matter of walking
through the cases and making the most conservative assumption. So
assume the condition is LT, both objects are unsigned types and LIMIT
has a range like [5..25]. Then the resulting *assumed_limit must be 24.
ISTM it might also be worth checking VRP here -- I'd expect it to be
able to make this kind of determination. that would be independent of
this work (in the sense that if VRP isn't creating ranges for this, it
should be fixed independently).
> + }
> +
> + return ALLOCA_UNBOUNDED;
> +}
> +
> +// Return TRUE if SSA's definition is a cast from a signed type.
> +// If so, set *INVALID_CASTED_TYPE to the signed type.
> +
> +static bool
> +cast_from_signed_p (tree ssa, tree *invalid_casted_type)
> +{
> + gimple *def = SSA_NAME_DEF_STMT (ssa);
> + if (def
> + && !gimple_nop_p (def)
> + && gimple_assign_cast_p (def)
> + && !TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def))))
> + {
> + *invalid_casted_type = TREE_TYPE (gimple_assign_rhs1 (def));
> + return true;
> + }
> + return false;
Note that we may have a cast from a signed type, but if the RHS of that
cast has a positive range, then the cast isn't going to case the
wrap-around effect that is so problematical. That might help cut down
false positives.
> +}
> +
> +// Return TURE if X has a maximum range of MAX, basically covering the
> +// entire domain, in which case it's no range at all.
s/TURE/TRUE/
> +
> +static bool
> +is_max (tree x, wide_int max)
> +{
> + return wi::max_value (TREE_TYPE (x)) == max;
> +}
I'm a bit surprised we don't have this kind of utility function
elsewhere. I wonder if it'd be better to conceptualize this as a range
query since it looks like you're always using get_range_info to get an
object's range, then comparing what that returns to the maximal value of
hte object's type. Maybe that's too much bikeshedding...
> +
> +// Analyze the alloca call in STMT and return an `enum alloca_type'
> +// explaining what type of alloca it is. IS_VLA is set if the alloca
> +// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
> +//
> +// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
> +// set to the bound used to determine this. ASSUMED_LIMIT is only set
> +// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
> +//
> +// If the alloca call may be too large because of a cast from a signed
> +// type to an unsigned type, set *INVALID_CASTED_TYPE to the
> +// problematic signed type.
> +
> +static enum alloca_type
> +alloca_call_type (gimple *stmt, bool is_vla, wide_int *assumed_limit,
> + tree *invalid_casted_type)
Again, consider an aggregate return.
> +{
> + gcc_assert (gimple_alloca_call_p (stmt));
> + tree len = gimple_call_arg (stmt, 0);
> + enum alloca_type w = ALLOCA_UNBOUNDED;
> + wide_int min, max;
> +
> + gcc_assert (!is_vla || warn_vla_limit > 0);
> + gcc_assert (is_vla || warn_alloca_limit > 0);
> +
> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
> + // type into account.
> + unsigned HOST_WIDE_INT max_size;
> + if (is_vla)
> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
> + else
> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
> +
> + // Check for the obviously bounded case.
> + if (TREE_CODE (len) == INTEGER_CST)
> + {
> + if (tree_to_uhwi (len) > max_size)
> + {
> + *assumed_limit = len;
> + return ALLOCA_BOUND_DEFINITELY_LARGE;
> + }
> + if (integer_zerop (len))
> + return ALLOCA_ARG_IS_ZERO;
> + w = ALLOCA_OK;
> + }
> + else if (TREE_CODE (len) != SSA_NAME)
> + return ALLOCA_UNBOUNDED;
Hmm, other than INTEGER_CST and SSA_NAME, is there any other nodes we
can get here? Perhaps we get DECLs and such, particularly when not
optimizing?!?
> + // Check the range info if available.
> + else
> + {
> + if (value_range_type range_type = get_range_info (len, &min, &max))
> + {
> + if (range_type == VR_RANGE)
> + {
> + if (wi::leu_p (max, max_size))
> + w = ALLOCA_OK;
> + else if (is_max (len, max))
> + {
> + // A cast may have created a range we don't care
> + // about. For instance, a cast from 16-bit to
> + // 32-bit creates a range of 0..65535, even if there
> + // is not really a determinable range in the
> + // underlying code. In this case, look through the
> + // cast at the original argument, and fall through
> + // to look at other alternatives.
> + gimple *def = SSA_NAME_DEF_STMT (len);
> + if (gimple_assign_cast_p (def))
> + len = gimple_assign_rhs1 (def);
> + }
> + else
> + {
> + /* If `len' is merely a cast that is being
> + calculated right before the call to alloca, look
> + at the range for the original value.
Yea, this is similar to my comment earlier that the RHS of the cast may
have a known range (non-negative) that allows us to not worry about the
cast creating a huge integer. I think you can add handling that case
here without too much trouble. Though you might consider pulling all
the casting bits into a separate function.
> +
> + This avoids the cast creating a range where the
> + original expression did not have a range:
> +
> + # RANGE [0, 18446744073709551615] NONZERO 4294967295
> + _2 = (long unsigned int) n_7(D);
> + p_9 = __builtin_alloca (_2);
Note this example would make more sense if the type of n_7 was explicit.
> + else if (range_type == VR_ANTI_RANGE)
> + {
> + // There may be some wrapping around going on. Catch it
> + // with this heuristic. Hopefully, this VR_ANTI_RANGE
> + // nonsense will go away, and we won't have to catch the
> + // sign conversion problems with this crap.
> + if (cast_from_signed_p (len, invalid_casted_type))
> + return ALLOCA_CAST_FROM_SIGNED;
Another place where casting from an object with a non-negative range
ought to be filtered out as not problematical.
> +
> + // If we couldn't find anything, try a few heuristics for things we
> + // can easily determine. Check these misc cases but only accept
> + // them if all predecessors have a known bound.
> + basic_block bb = gimple_bb (stmt);
> + if (w == ALLOCA_UNBOUNDED)
> + {
> + w = ALLOCA_OK;
> + for (unsigned ix = 0; ix < EDGE_COUNT (bb->preds); ix++)
> + {
> + enum alloca_type w
> + = alloca_call_type_by_arg (len, EDGE_PRED (bb, ix), max_size,
> + assumed_limit);
> + if (w != ALLOCA_OK)
> + return w;
> + }
So this is an interesting tidbit that answers my questions about how we
use alloca_call_type_by_arg. Essentially this is meant to catch the
merge point for flow control and give a conservative warning. That's
fine and good. But ISTM it's really a bit of a hack. What if we have
something like this:
X Y Z
\ | /
\|/
A
/ \
/ \
B C
/ \
/ \
D E
Where the alloca call is in E and the incoming edges to A actually have
useful information about the argument to the alloca call.
ISTM you need to be doing something with the dominator tree here to find
the merge point(s) where we might know something useful. And it's this
kind of test that makes me wonder about re-purposing some of the path
analysis code from tree-ssa-uninit.c. It may be the case that the path
Z->A->C->E is unfeasible, but left in the CFG because duplication to
expose the unfeasible path was unprofitable. If it turns out that the
only argument that causes problems comes from the edge Z->A, then we
wouldn't want to warn in this case.
I don't see Andrew's work necessarily being able to solve this problem.
> + }
> +
> + return w;
> +}
> +
> +// Return TRUE if the alloca call in STMT is in a loop, otherwise
> +// return FALSE. As an exception, ignore alloca calls for VLAs that
> +// occur in a loop since those will be cleaned up when they go out of
> +// scope.
> +
> +static bool
> +in_loop_p (bool is_vla, gimple *stmt)
> +{
> + basic_block bb = gimple_bb (stmt);
> + if (bb->loop_father
> + // ?? Functions with no loops get a loop_father? I
> + // don't get it. The following conditional seems to do
> + // the trick to exclude such nonsense.
> + && bb->loop_father->header != ENTRY_BLOCK_PTR_FOR_FN (cfun))
I believe there is a "loop" that encompasses the whole function.
I'll probably have more comments after the next iteration. I'm also
real curious what you've found poking around at GDB with this warning :-)
Jeff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
[not found] ` <57B6D2F1.6050709@redhat.com>
@ 2016-08-19 14:57 ` Aldy Hernandez
0 siblings, 0 replies; 26+ messages in thread
From: Aldy Hernandez @ 2016-08-19 14:57 UTC (permalink / raw)
To: Jeff Law, gcc-patches
On 08/19/2016 05:35 AM, Aldy Hernandez wrote:
> On 08/04/2016 12:37 PM, Jeff Law wrote:
>> On 07/27/2016 03:01 AM, Aldy Hernandez wrote:
>>> Just in case this got lost in noise, since I know there was a lot of
>>> back and forth between Martin Sebor and I.
>>>
>>> This is the last iteration.
>>>
>>> Tested on x86-64 Linux.
>>>
>>> OK for trunk?
>>>
>>> curr
>>>
>>>
>>> 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.
>>> * 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.
>> As someone already noted, it's gimple-ssa-warn-alloca, not
>> gimple-ssa-warn-walloca for the ChangeLog entry.
>
> Fixed.
>
>>
>> On the nittish side, you're mixing C and C++ comment styles. Choosing
>> one and sticking with it seems better :-)
>
> Fixed. Settled for C++ comments, except the copyright headers and the
> testcases.
>
>>
>>
>>>
>>> +@item -Walloca
>>> +@opindex Wno-alloca
>>> +@opindex Walloca
>>> +This option warns on all uses of @code{alloca} in the source.
>>> +
>>> +@item -Walloca-larger-than=@var{n}
>>> +This option warns on calls to @code{alloca} that are not bounded by a
>>> +controlling predicate limiting its size to @var{n} bytes, or calls to
>>> +@code{alloca} where the bound is unknown.
>> So for each of these little examples, I'd stuff the code into a trivial
>> function definition and make "n" a parameter. That way it's obvious the
>> value of "n" comes from a context where we don't initially know its
>> range, but we may be able to narrow the range due to statements in the
>> function.
>
> Done.
>
>>
>> ;
>>> +
>>> +class pass_walloca : public gimple_opt_pass
>>> +{
>>> +public:
>>> + pass_walloca (gcc::context *ctxt)
>>> + : gimple_opt_pass(pass_data_walloca, ctxt), first_time_p (false)
>>> + {}
>>> + opt_pass *clone () { return new pass_walloca (m_ctxt); }
>>> + void set_pass_param (unsigned int n, bool param)
>>> + {
>>> + gcc_assert (n == 0);
>>> + first_time_p = param;
>>> + }
>> ISTM that you're using "first_time_p" here, but in passes.def you refer
>> to this parameter as "strict_mode_p" in comments.
>>
>> ie:
>>
>> + NEXT_PASS (pass_walloca, /*strict_mode_p=*/false);
>>
>> I'd just drop the /*strict_mode_p*/ comment in both places it appears in
>> your patch's change to passes.def. I think we've generally frowned on
>> those embedded comments, even though some have snuck in.
>
> I've seen a lot of embedded comments throughout GCC, especially in
> optional type arguments. ISTM it makes things clearer for these
> parameters. But hey, I don't care that much. Fixed.
>
>>
>>> +
>>> +// We have a few heuristics up our sleeve to determine if a call to
>>> +// alloca() is within bounds. Try them out and return the type of
>>> +// alloca call this is based on its argument.
>>> +//
>>> +// Given a known argument (ARG) to alloca() and an EDGE (E)
>>> +// calculating said argument, verify that the last statement in the BB
>>> +// in E->SRC is a gate comparing ARG to an acceptable bound for
>>> +// alloca(). See examples below.
>>> +//
>>> +// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
>>> +// in bytes we allow for arg.
>>> +//
>>> +// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
>>> +// set to the bound used to determine this. ASSUMED_LIMIT is only set
>>> +// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
>>> +//
>>> +// Returns the alloca type.
>>> +
>>> +static enum alloca_type
>>> +alloca_call_type_by_arg (tree arg, edge e, unsigned max_size,
>>> + wide_int *assumed_limit)
>> So I wonder if you ought to have a structure here for the return value
>> which contains the alloca type and assumed limit. I know in the past we
>> avoided aggregate returns, but these days that doesn't seem necessary.
>> Seems cleaner than having a return value and output parameters.
>
> Done, C++ style with a simple constructor :).
>
>>
>>> +{
>>> + // All the tests bellow depend on the jump being on the TRUE path.
>>> + if (!(e->flags & EDGE_TRUE_VALUE))
>>> + return ALLOCA_UNBOUNDED;
>> Seems like a fairly arbitrary and undesirable limitation. Couldn't the
>> developer just have easily written
>>
>> if (arg > N>
>> x = malloc (...)
>> else
>> x = alloca (...)
>>
>> It also seems like you'd want to handle the set of LT/LE/GT/GE rather
>> than just LE. Or is it the case that we always canonicalize LT into LE
>> by adjusting the constant (I vaguely remember running into that in RTL,
>> so it's entirely possible and there'd likely be a canonicalization of
>> GT/GE as well).
>
> Most of it gets canonicalized, but your testcase is definitely possible,
> so I fixed this.
>
>>
>> It also seems that once Andrew's infrastructure is in place this becomes
>> dead code as we can just ask for the range at a point in the program,
>> including for each incoming edge. You might want a comment to that
>> effect.
>
> Done.
>
>>
>>
>>
>>
>>> +
>>> + /* Check for:
>>> + if (arg .cond. LIMIT) -or- if (LIMIT .cond. arg)
>>> + alloca(arg);
>>> +
>>> + Where LIMIT has a bound of unknown range. */
>>> + tree limit = NULL;
>>> + if (gimple_cond_lhs (last) == arg)
>>> + limit = gimple_cond_rhs (last);
>>> + else if (gimple_cond_rhs (last) == arg)
>>> + limit = gimple_cond_lhs (last);
>>> + if (limit && TREE_CODE (limit) == SSA_NAME)
>>> + {
>>> + wide_int min, max;
>>> + value_range_type range_type = get_range_info (limit, &min, &max);
>>> + if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
>>> + return ALLOCA_BOUND_UNKNOWN;
>>> + // FIXME: We could try harder here and handle a possible range
>>> + // or anti-range. Hopefully the upcoming changes to range info
>>> + // will give us finer grained info, and we can avoid somersaults
>>> + // here.
>> Ah, can't you set *assumed_limit here? It's just a matter of walking
>> through the cases and making the most conservative assumption. So
>> assume the condition is LT, both objects are unsigned types and LIMIT
>> has a range like [5..25]. Then the resulting *assumed_limit must be 24.
>
> Well, it turns out that we don't ever hit the FIXME path. We either
> handle limits we know with the previous code block (which gets
> normalized to [arg .cond. LIMIT] always, or we handle the unknown path
> with this block with [LIMIT .cond. arg].
>
> I've simplified the code a bit, and updated the comments. I also added
> a gcc_unreachable() just in case we ever hit this path. Though I've
> verified at least building glibc and binutils that we never do.
>
>>
>> ISTM it might also be worth checking VRP here -- I'd expect it to be
>> able to make this kind of determination. that would be independent of
>> this work (in the sense that if VRP isn't creating ranges for this, it
>> should be fixed independently).
>>
>>
>>> + }
>>> +
>>> + return ALLOCA_UNBOUNDED;
>>> +}
>>> +
>>> +// Return TRUE if SSA's definition is a cast from a signed type.
>>> +// If so, set *INVALID_CASTED_TYPE to the signed type.
>>> +
>>> +static bool
>>> +cast_from_signed_p (tree ssa, tree *invalid_casted_type)
>>> +{
>>> + gimple *def = SSA_NAME_DEF_STMT (ssa);
>>> + if (def
>>> + && !gimple_nop_p (def)
>>> + && gimple_assign_cast_p (def)
>>> + && !TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def))))
>>> + {
>>> + *invalid_casted_type = TREE_TYPE (gimple_assign_rhs1 (def));
>>> + return true;
>>> + }
>>> + return false;
>> Note that we may have a cast from a signed type, but if the RHS of that
>> cast has a positive range, then the cast isn't going to case the
>> wrap-around effect that is so problematical. That might help cut down
>> false positives.
>
> Actually when the cast is from a known positive range we don't get a
> VR_ANTI_RANGE, we get a proper VR_RANGE.
>
> I've cleaned this code up a bit and merged some common conditionals. In
> the process, taking a subset of your advice, and cleaning up some things
> I've managed to handle 2 cases where I was previously XFAILing.
> So...less false positives. More coverage. Woo hoo!
>
>>
>>> +}
>>> +
>>> +// Return TURE if X has a maximum range of MAX, basically covering the
>>> +// entire domain, in which case it's no range at all.
>> s/TURE/TRUE/
>
> Fixed.
>
>>
>>
>>> +
>>> +static bool
>>> +is_max (tree x, wide_int max)
>>> +{
>>> + return wi::max_value (TREE_TYPE (x)) == max;
>>> +}
>> I'm a bit surprised we don't have this kind of utility function
>> elsewhere. I wonder if it'd be better to conceptualize this as a range
>> query since it looks like you're always using get_range_info to get an
>> object's range, then comparing what that returns to the maximal value of
>> hte object's type. Maybe that's too much bikeshedding...
>
> *shrug*. If you feel strongly, I can look into this, but I'm inherently
> lazy :).
>
>>
>>
>>
>>> +
>>> +// Analyze the alloca call in STMT and return an `enum alloca_type'
>>> +// explaining what type of alloca it is. IS_VLA is set if the alloca
>>> +// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
>>> +//
>>> +// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
>>> +// set to the bound used to determine this. ASSUMED_LIMIT is only set
>>> +// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
>>> +//
>>> +// If the alloca call may be too large because of a cast from a signed
>>> +// type to an unsigned type, set *INVALID_CASTED_TYPE to the
>>> +// problematic signed type.
>>> +
>>> +static enum alloca_type
>>> +alloca_call_type (gimple *stmt, bool is_vla, wide_int *assumed_limit,
>>> + tree *invalid_casted_type)
>> Again, consider an aggregate return.
>
> Done.
>
>>
>>> +{
>>> + gcc_assert (gimple_alloca_call_p (stmt));
>>> + tree len = gimple_call_arg (stmt, 0);
>>> + enum alloca_type w = ALLOCA_UNBOUNDED;
>>> + wide_int min, max;
>>> +
>>> + gcc_assert (!is_vla || warn_vla_limit > 0);
>>> + gcc_assert (is_vla || warn_alloca_limit > 0);
>>> +
>>> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
>>> + // type into account.
>>> + unsigned HOST_WIDE_INT max_size;
>>> + if (is_vla)
>>> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
>>> + else
>>> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
>>> +
>>> + // Check for the obviously bounded case.
>>> + if (TREE_CODE (len) == INTEGER_CST)
>>> + {
>>> + if (tree_to_uhwi (len) > max_size)
>>> + {
>>> + *assumed_limit = len;
>>> + return ALLOCA_BOUND_DEFINITELY_LARGE;
>>> + }
>>> + if (integer_zerop (len))
>>> + return ALLOCA_ARG_IS_ZERO;
>>> + w = ALLOCA_OK;
>>> + }
>>> + else if (TREE_CODE (len) != SSA_NAME)
>>> + return ALLOCA_UNBOUNDED;
>> Hmm, other than INTEGER_CST and SSA_NAME, is there any other nodes we
>> can get here? Perhaps we get DECLs and such, particularly when not
>> optimizing?!?
>
> Nope. We don't even run without optimization (because we need VRP/range
> info). I added a gcc_unreachable() to make sure and added an
> appropriate comment. It doesn't get triggered on tests or
> glibc/binutils builds.
>
>>
>>> + // Check the range info if available.
>>> + else
>>> + {
>>> + if (value_range_type range_type = get_range_info (len, &min,
>>> &max))
>>> + {
>>> + if (range_type == VR_RANGE)
>>> + {
>>> + if (wi::leu_p (max, max_size))
>>> + w = ALLOCA_OK;
>>> + else if (is_max (len, max))
>>> + {
>>> + // A cast may have created a range we don't care
>>> + // about. For instance, a cast from 16-bit to
>>> + // 32-bit creates a range of 0..65535, even if there
>>> + // is not really a determinable range in the
>>> + // underlying code. In this case, look through the
>>> + // cast at the original argument, and fall through
>>> + // to look at other alternatives.
>>> + gimple *def = SSA_NAME_DEF_STMT (len);
>>> + if (gimple_assign_cast_p (def))
>>> + len = gimple_assign_rhs1 (def);
>>> + }
>>> + else
>>> + {
>>> + /* If `len' is merely a cast that is being
>>> + calculated right before the call to alloca, look
>>> + at the range for the original value.
>> Yea, this is similar to my comment earlier that the RHS of the cast may
>> have a known range (non-negative) that allows us to not worry about the
>> cast creating a huge integer. I think you can add handling that case
>> here without too much trouble. Though you might consider pulling all
>> the casting bits into a separate function.
>
> See previous comments. I've merged and cleaned this up.
>
>>
>>> +
>>> + This avoids the cast creating a range where the
>>> + original expression did not have a range:
>>> +
>>> + # RANGE [0, 18446744073709551615] NONZERO 4294967295
>>> + _2 = (long unsigned int) n_7(D);
>>> + p_9 = __builtin_alloca (_2);
>> Note this example would make more sense if the type of n_7 was explicit.
>
> Merged and removed.
>
>>
>>
>>> + else if (range_type == VR_ANTI_RANGE)
>>> + {
>>> + // There may be some wrapping around going on. Catch it
>>> + // with this heuristic. Hopefully, this VR_ANTI_RANGE
>>> + // nonsense will go away, and we won't have to catch the
>>> + // sign conversion problems with this crap.
>>> + if (cast_from_signed_p (len, invalid_casted_type))
>>> + return ALLOCA_CAST_FROM_SIGNED;
>> Another place where casting from an object with a non-negative range
>> ought to be filtered out as not problematical.
>
> Same.
>
>>
>>
>>> +
>>> + // If we couldn't find anything, try a few heuristics for things we
>>> + // can easily determine. Check these misc cases but only accept
>>> + // them if all predecessors have a known bound.
>>> + basic_block bb = gimple_bb (stmt);
>>> + if (w == ALLOCA_UNBOUNDED)
>>> + {
>>> + w = ALLOCA_OK;
>>> + for (unsigned ix = 0; ix < EDGE_COUNT (bb->preds); ix++)
>>> + {
>>> + enum alloca_type w
>>> + = alloca_call_type_by_arg (len, EDGE_PRED (bb, ix), max_size,
>>> + assumed_limit);
>>> + if (w != ALLOCA_OK)
>>> + return w;
>>> + }
>> So this is an interesting tidbit that answers my questions about how we
>> use alloca_call_type_by_arg. Essentially this is meant to catch the
>> merge point for flow control and give a conservative warning. That's
>> fine and good. But ISTM it's really a bit of a hack. What if we have
>> something like this:
>>
>> X Y Z
>> \ | /
>> \|/
>> A
>> / \
>> / \
>> B C
>> / \
>> / \
>> D E
>>
>>
>> Where the alloca call is in E and the incoming edges to A actually have
>> useful information about the argument to the alloca call.
>>
>> ISTM you need to be doing something with the dominator tree here to find
>> the merge point(s) where we might know something useful. And it's this
>> kind of test that makes me wonder about re-purposing some of the path
>> analysis code from tree-ssa-uninit.c. It may be the case that the path
>> Z->A->C->E is unfeasible, but left in the CFG because duplication to
>> expose the unfeasible path was unprofitable. If it turns out that the
>> only argument that causes problems comes from the edge Z->A, then we
>> wouldn't want to warn in this case.
>>
>> I don't see Andrew's work necessarily being able to solve this problem.
>
> In my limited testing I've seen that 95% of all cases (I'm pulling this
> number out of thin air ;-)) are relatively simple. Just looking at the
> definition of the SSA name in the alloca() call or the immediate
> predecessors yields most of the information we need. I haven't seen
> much more complicated things with an actual range.
>
> So I am hesitant to complicate things for something that doesn't seem as
> likely to happen. Perhaps, as a follow-up if it happens in the wild?
>
> However, if you feel strongly about this, I will finally get around to
> reading tree-ssa-uninit.c and getting down to business :).
>
>>
>>
>>
>>
>>
>>> + }
>>> +
>>> + return w;
>>> +}
>>> +
>>> +// Return TRUE if the alloca call in STMT is in a loop, otherwise
>>> +// return FALSE. As an exception, ignore alloca calls for VLAs that
>>> +// occur in a loop since those will be cleaned up when they go out of
>>> +// scope.
>>> +
>>> +static bool
>>> +in_loop_p (bool is_vla, gimple *stmt)
>>> +{
>>> + basic_block bb = gimple_bb (stmt);
>>> + if (bb->loop_father
>>> + // ?? Functions with no loops get a loop_father? I
>>> + // don't get it. The following conditional seems to do
>>> + // the trick to exclude such nonsense.
>>> + && bb->loop_father->header != ENTRY_BLOCK_PTR_FOR_FN (cfun))
>> I believe there is a "loop" that encompasses the whole function.
>
> Remove clueless comment.
>
> Phew. That took longer than expected.
>
> Regstrapped on x86-64 Linux and the resulting compiler was verified by
> building glibc and binutils.
>
> Aldy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-08-04 16:38 ` Jeff Law
[not found] ` <57B6D2F1.6050709@redhat.com>
@ 2016-08-19 14:59 ` Aldy Hernandez
2016-09-13 20:30 ` Jeff Law
1 sibling, 1 reply; 26+ messages in thread
From: Aldy Hernandez @ 2016-08-19 14:59 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 16990 bytes --]
On 08/04/2016 12:37 PM, Jeff Law wrote:
[ughhh, one more time but with actual content]
> On 07/27/2016 03:01 AM, Aldy Hernandez wrote:
>> Just in case this got lost in noise, since I know there was a lot of
>> back and forth between Martin Sebor and I.
>>
>> This is the last iteration.
>>
>> Tested on x86-64 Linux.
>>
>> OK for trunk?
>>
>> curr
>>
>>
>> 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.
>> * 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.
> As someone already noted, it's gimple-ssa-warn-alloca, not
> gimple-ssa-warn-walloca for the ChangeLog entry.
Fixed.
>
> On the nittish side, you're mixing C and C++ comment styles. Choosing
> one and sticking with it seems better :-)
Fixed. Settled for C++ comments, except the copyright headers and the
testcases.
>
>
>>
>> +@item -Walloca
>> +@opindex Wno-alloca
>> +@opindex Walloca
>> +This option warns on all uses of @code{alloca} in the source.
>> +
>> +@item -Walloca-larger-than=@var{n}
>> +This option warns on calls to @code{alloca} that are not bounded by a
>> +controlling predicate limiting its size to @var{n} bytes, or calls to
>> +@code{alloca} where the bound is unknown.
> So for each of these little examples, I'd stuff the code into a trivial
> function definition and make "n" a parameter. That way it's obvious the
> value of "n" comes from a context where we don't initially know its
> range, but we may be able to narrow the range due to statements in the
> function.
Done.
>
> ;
>> +
>> +class pass_walloca : public gimple_opt_pass
>> +{
>> +public:
>> + pass_walloca (gcc::context *ctxt)
>> + : gimple_opt_pass(pass_data_walloca, ctxt), first_time_p (false)
>> + {}
>> + opt_pass *clone () { return new pass_walloca (m_ctxt); }
>> + void set_pass_param (unsigned int n, bool param)
>> + {
>> + gcc_assert (n == 0);
>> + first_time_p = param;
>> + }
> ISTM that you're using "first_time_p" here, but in passes.def you refer
> to this parameter as "strict_mode_p" in comments.
>
> ie:
>
> + NEXT_PASS (pass_walloca, /*strict_mode_p=*/false);
>
> I'd just drop the /*strict_mode_p*/ comment in both places it appears in
> your patch's change to passes.def. I think we've generally frowned on
> those embedded comments, even though some have snuck in.
I've seen a lot of embedded comments throughout GCC, especially in
optional type arguments. ISTM it makes things clearer for these
parameters. But hey, I don't care that much. Fixed.
>
>> +
>> +// We have a few heuristics up our sleeve to determine if a call to
>> +// alloca() is within bounds. Try them out and return the type of
>> +// alloca call this is based on its argument.
>> +//
>> +// Given a known argument (ARG) to alloca() and an EDGE (E)
>> +// calculating said argument, verify that the last statement in the BB
>> +// in E->SRC is a gate comparing ARG to an acceptable bound for
>> +// alloca(). See examples below.
>> +//
>> +// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
>> +// in bytes we allow for arg.
>> +//
>> +// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
>> +// set to the bound used to determine this. ASSUMED_LIMIT is only set
>> +// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
>> +//
>> +// Returns the alloca type.
>> +
>> +static enum alloca_type
>> +alloca_call_type_by_arg (tree arg, edge e, unsigned max_size,
>> + wide_int *assumed_limit)
> So I wonder if you ought to have a structure here for the return value
> which contains the alloca type and assumed limit. I know in the past we
> avoided aggregate returns, but these days that doesn't seem necessary.
> Seems cleaner than having a return value and output parameters.
Done, C++ style with a simple constructor :).
>
>> +{
>> + // All the tests bellow depend on the jump being on the TRUE path.
>> + if (!(e->flags & EDGE_TRUE_VALUE))
>> + return ALLOCA_UNBOUNDED;
> Seems like a fairly arbitrary and undesirable limitation. Couldn't the
> developer just have easily written
>
> if (arg > N>
> x = malloc (...)
> else
> x = alloca (...)
>
> It also seems like you'd want to handle the set of LT/LE/GT/GE rather
> than just LE. Or is it the case that we always canonicalize LT into LE
> by adjusting the constant (I vaguely remember running into that in RTL,
> so it's entirely possible and there'd likely be a canonicalization of
> GT/GE as well).
Most of it gets canonicalized, but your testcase is definitely possible,
so I fixed this.
>
> It also seems that once Andrew's infrastructure is in place this becomes
> dead code as we can just ask for the range at a point in the program,
> including for each incoming edge. You might want a comment to that effect.
Done.
>
>
>
>
>> +
>> + /* Check for:
>> + if (arg .cond. LIMIT) -or- if (LIMIT .cond. arg)
>> + alloca(arg);
>> +
>> + Where LIMIT has a bound of unknown range. */
>> + tree limit = NULL;
>> + if (gimple_cond_lhs (last) == arg)
>> + limit = gimple_cond_rhs (last);
>> + else if (gimple_cond_rhs (last) == arg)
>> + limit = gimple_cond_lhs (last);
>> + if (limit && TREE_CODE (limit) == SSA_NAME)
>> + {
>> + wide_int min, max;
>> + value_range_type range_type = get_range_info (limit, &min, &max);
>> + if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
>> + return ALLOCA_BOUND_UNKNOWN;
>> + // FIXME: We could try harder here and handle a possible range
>> + // or anti-range. Hopefully the upcoming changes to range info
>> + // will give us finer grained info, and we can avoid somersaults
>> + // here.
> Ah, can't you set *assumed_limit here? It's just a matter of walking
> through the cases and making the most conservative assumption. So
> assume the condition is LT, both objects are unsigned types and LIMIT
> has a range like [5..25]. Then the resulting *assumed_limit must be 24.
Well, it turns out that we don't ever hit the FIXME path. We either
handle limits we know with the previous code block (which gets
normalized to [arg .cond. LIMIT] always, or we handle the unknown path
with this block with [LIMIT .cond. arg].
I've simplified the code a bit, and updated the comments. I also added
a gcc_unreachable() just in case we ever hit this path. Though I've
verified at least building glibc and binutils that we never do.
>
> ISTM it might also be worth checking VRP here -- I'd expect it to be
> able to make this kind of determination. that would be independent of
> this work (in the sense that if VRP isn't creating ranges for this, it
> should be fixed independently).
>
>
>> + }
>> +
>> + return ALLOCA_UNBOUNDED;
>> +}
>> +
>> +// Return TRUE if SSA's definition is a cast from a signed type.
>> +// If so, set *INVALID_CASTED_TYPE to the signed type.
>> +
>> +static bool
>> +cast_from_signed_p (tree ssa, tree *invalid_casted_type)
>> +{
>> + gimple *def = SSA_NAME_DEF_STMT (ssa);
>> + if (def
>> + && !gimple_nop_p (def)
>> + && gimple_assign_cast_p (def)
>> + && !TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def))))
>> + {
>> + *invalid_casted_type = TREE_TYPE (gimple_assign_rhs1 (def));
>> + return true;
>> + }
>> + return false;
> Note that we may have a cast from a signed type, but if the RHS of that
> cast has a positive range, then the cast isn't going to case the
> wrap-around effect that is so problematical. That might help cut down
> false positives.
Actually when the cast is from a known positive range we don't get a
VR_ANTI_RANGE, we get a proper VR_RANGE.
I've cleaned this code up a bit and merged some common conditionals. In
the process, taking a subset of your advice, and cleaning up some things
I've managed to handle 2 cases where I was previously XFAILing.
So...less false positives. More coverage. Woo hoo!
>
>> +}
>> +
>> +// Return TURE if X has a maximum range of MAX, basically covering the
>> +// entire domain, in which case it's no range at all.
> s/TURE/TRUE/
Fixed.
>
>
>> +
>> +static bool
>> +is_max (tree x, wide_int max)
>> +{
>> + return wi::max_value (TREE_TYPE (x)) == max;
>> +}
> I'm a bit surprised we don't have this kind of utility function
> elsewhere. I wonder if it'd be better to conceptualize this as a range
> query since it looks like you're always using get_range_info to get an
> object's range, then comparing what that returns to the maximal value of
> hte object's type. Maybe that's too much bikeshedding...
*shrug*. If you feel strongly, I can look into this, but I'm inherently
lazy :).
>
>
>
>> +
>> +// Analyze the alloca call in STMT and return an `enum alloca_type'
>> +// explaining what type of alloca it is. IS_VLA is set if the alloca
>> +// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
>> +//
>> +// If the alloca bound is determined to be too large, ASSUMED_LIMIT is
>> +// set to the bound used to determine this. ASSUMED_LIMIT is only set
>> +// for ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE.
>> +//
>> +// If the alloca call may be too large because of a cast from a signed
>> +// type to an unsigned type, set *INVALID_CASTED_TYPE to the
>> +// problematic signed type.
>> +
>> +static enum alloca_type
>> +alloca_call_type (gimple *stmt, bool is_vla, wide_int *assumed_limit,
>> + tree *invalid_casted_type)
> Again, consider an aggregate return.
Done.
>
>> +{
>> + gcc_assert (gimple_alloca_call_p (stmt));
>> + tree len = gimple_call_arg (stmt, 0);
>> + enum alloca_type w = ALLOCA_UNBOUNDED;
>> + wide_int min, max;
>> +
>> + gcc_assert (!is_vla || warn_vla_limit > 0);
>> + gcc_assert (is_vla || warn_alloca_limit > 0);
>> +
>> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
>> + // type into account.
>> + unsigned HOST_WIDE_INT max_size;
>> + if (is_vla)
>> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
>> + else
>> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
>> +
>> + // Check for the obviously bounded case.
>> + if (TREE_CODE (len) == INTEGER_CST)
>> + {
>> + if (tree_to_uhwi (len) > max_size)
>> + {
>> + *assumed_limit = len;
>> + return ALLOCA_BOUND_DEFINITELY_LARGE;
>> + }
>> + if (integer_zerop (len))
>> + return ALLOCA_ARG_IS_ZERO;
>> + w = ALLOCA_OK;
>> + }
>> + else if (TREE_CODE (len) != SSA_NAME)
>> + return ALLOCA_UNBOUNDED;
> Hmm, other than INTEGER_CST and SSA_NAME, is there any other nodes we
> can get here? Perhaps we get DECLs and such, particularly when not
> optimizing?!?
Nope. We don't even run without optimization (because we need VRP/range
info). I added a gcc_unreachable() to make sure and added an
appropriate comment. It doesn't get triggered on tests or
glibc/binutils builds.
>
>> + // Check the range info if available.
>> + else
>> + {
>> + if (value_range_type range_type = get_range_info (len, &min,
>> &max))
>> + {
>> + if (range_type == VR_RANGE)
>> + {
>> + if (wi::leu_p (max, max_size))
>> + w = ALLOCA_OK;
>> + else if (is_max (len, max))
>> + {
>> + // A cast may have created a range we don't care
>> + // about. For instance, a cast from 16-bit to
>> + // 32-bit creates a range of 0..65535, even if there
>> + // is not really a determinable range in the
>> + // underlying code. In this case, look through the
>> + // cast at the original argument, and fall through
>> + // to look at other alternatives.
>> + gimple *def = SSA_NAME_DEF_STMT (len);
>> + if (gimple_assign_cast_p (def))
>> + len = gimple_assign_rhs1 (def);
>> + }
>> + else
>> + {
>> + /* If `len' is merely a cast that is being
>> + calculated right before the call to alloca, look
>> + at the range for the original value.
> Yea, this is similar to my comment earlier that the RHS of the cast may
> have a known range (non-negative) that allows us to not worry about the
> cast creating a huge integer. I think you can add handling that case
> here without too much trouble. Though you might consider pulling all
> the casting bits into a separate function.
See previous comments. I've merged and cleaned this up.
>
>> +
>> + This avoids the cast creating a range where the
>> + original expression did not have a range:
>> +
>> + # RANGE [0, 18446744073709551615] NONZERO 4294967295
>> + _2 = (long unsigned int) n_7(D);
>> + p_9 = __builtin_alloca (_2);
> Note this example would make more sense if the type of n_7 was explicit.
Merged and removed.
>
>
>> + else if (range_type == VR_ANTI_RANGE)
>> + {
>> + // There may be some wrapping around going on. Catch it
>> + // with this heuristic. Hopefully, this VR_ANTI_RANGE
>> + // nonsense will go away, and we won't have to catch the
>> + // sign conversion problems with this crap.
>> + if (cast_from_signed_p (len, invalid_casted_type))
>> + return ALLOCA_CAST_FROM_SIGNED;
> Another place where casting from an object with a non-negative range
> ought to be filtered out as not problematical.
Same.
>
>
>> +
>> + // If we couldn't find anything, try a few heuristics for things we
>> + // can easily determine. Check these misc cases but only accept
>> + // them if all predecessors have a known bound.
>> + basic_block bb = gimple_bb (stmt);
>> + if (w == ALLOCA_UNBOUNDED)
>> + {
>> + w = ALLOCA_OK;
>> + for (unsigned ix = 0; ix < EDGE_COUNT (bb->preds); ix++)
>> + {
>> + enum alloca_type w
>> + = alloca_call_type_by_arg (len, EDGE_PRED (bb, ix), max_size,
>> + assumed_limit);
>> + if (w != ALLOCA_OK)
>> + return w;
>> + }
> So this is an interesting tidbit that answers my questions about how we
> use alloca_call_type_by_arg. Essentially this is meant to catch the
> merge point for flow control and give a conservative warning. That's
> fine and good. But ISTM it's really a bit of a hack. What if we have
> something like this:
>
> X Y Z
> \ | /
> \|/
> A
> / \
> / \
> B C
> / \
> / \
> D E
>
>
> Where the alloca call is in E and the incoming edges to A actually have
> useful information about the argument to the alloca call.
>
> ISTM you need to be doing something with the dominator tree here to find
> the merge point(s) where we might know something useful. And it's this
> kind of test that makes me wonder about re-purposing some of the path
> analysis code from tree-ssa-uninit.c. It may be the case that the path
> Z->A->C->E is unfeasible, but left in the CFG because duplication to
> expose the unfeasible path was unprofitable. If it turns out that the
> only argument that causes problems comes from the edge Z->A, then we
> wouldn't want to warn in this case.
>
> I don't see Andrew's work necessarily being able to solve this problem.
In my limited testing I've seen that 95% of all cases (I'm pulling this
number out of thin air ;-)) are relatively simple. Just looking at the
definition of the SSA name in the alloca() call or the immediate
predecessors yields most of the information we need. I haven't seen
much more complicated things with an actual range.
So I am hesitant to complicate things for something that doesn't seem as
likely to happen. Perhaps, as a follow-up if it happens in the wild?
However, if you feel strongly about this, I will finally get around to
reading tree-ssa-uninit.c and getting down to business :).
>
>
>
>
>
>> + }
>> +
>> + return w;
>> +}
>> +
>> +// Return TRUE if the alloca call in STMT is in a loop, otherwise
>> +// return FALSE. As an exception, ignore alloca calls for VLAs that
>> +// occur in a loop since those will be cleaned up when they go out of
>> +// scope.
>> +
>> +static bool
>> +in_loop_p (bool is_vla, gimple *stmt)
>> +{
>> + basic_block bb = gimple_bb (stmt);
>> + if (bb->loop_father
>> + // ?? Functions with no loops get a loop_father? I
>> + // don't get it. The following conditional seems to do
>> + // the trick to exclude such nonsense.
>> + && bb->loop_father->header != ENTRY_BLOCK_PTR_FOR_FN (cfun))
> I believe there is a "loop" that encompasses the whole function.
Remove clueless comment.
Phew. That took longer than expected.
Regstrapped on x86-64 Linux and the resulting compiler was verified by
building glibc and binutils.
Aldy
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 36976 bytes --]
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-alloca.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=<number> Warn on unbounded uses of
+alloca, and on bounded uses of alloca whose bound can be larger than
+<number> 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=<number> Warn on unbounded uses of variable-length arrays, and
+on bounded uses of variable-length arrays whose bound can be
+larger than <number> 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..a7f430e 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,70 @@ annotations.
Warn about overriding virtual functions that are not marked with the override
keyword.
+@item -Walloca
+@opindex Wno-alloca
+@opindex Walloca
+This option warns on all uses of @code{alloca} in the source.
+
+@item -Walloca-larger-than=@var{n}
+This option warns on calls to @code{alloca} that are not bounded by a
+controlling predicate limiting its size to @var{n} bytes, or calls to
+@code{alloca} where the bound is unknown.
+
+For example, a bounded case of @code{alloca} could be:
+
+@smallexample
+void func (size_t n)
+@{
+ void *p;
+ if (n <= 1000)
+ p = alloca (n);
+ else
+ p = malloc (n);
+ f (p);
+@}
+@end smallexample
+
+In the above example, passing @code{-Walloca=1000} would not issue a
+warning because the call to @code{alloca} is known to be at most 1000
+bytes. However, if @code{-Walloca=500} was passed, the compiler would
+have emitted a warning.
+
+Unbounded uses, on the other hand, are uses of @code{alloca} with no
+controlling predicate verifying its size. For example:
+
+@smallexample
+stuff ();
+p = alloca (n);
+@end smallexample
+
+If @code{-Walloca=500} was passed, the above would trigger a warning,
+but this time because of the lack of bounds checking.
+
+Note, that even seemingly correct code involving signed integers could
+cause a warning:
+
+@smallexample
+void func (signed int n)
+@{
+ if (n < 500)
+ @{
+ p = alloca (n);
+ f (p);
+ @}
+@}
+@end smallexample
+
+In the above example, @var{n} could be negative, causing a larger than
+expected argument to be implicitly casted into the @code{alloca} call.
+
+This option also warns when @code{alloca} is used in a loop.
+
+This warning is not enabled by @option{-Wall}, and is only active when
+@option{-ftree-vrp} is active (default for @option{-O2} and above).
+
+See also @option{-Wvla-larger-than=@var{n}}.
+
@item -Warray-bounds
@itemx -Warray-bounds=@var{n}
@opindex Wno-array-bounds
@@ -5830,9 +5895,25 @@ moving from a moved-from object, this warning can be disabled.
@item -Wvla
@opindex Wvla
@opindex Wno-vla
-Warn if variable length array is used in the code.
+Warn if a variable-length array is used in the code.
@option{-Wno-vla} prevents the @option{-Wpedantic} warning of
-the variable length array.
+the variable-length array.
+
+@item -Wvla-larger-than=@var{n}
+If this option is used, the compiler will warn on uses of
+variable-length arrays where the size is either unbounded, or bounded
+by an argument that can be larger than @var{n} bytes. This is similar
+to how @option{-Walloca-larger-than=@var{n}} works, but with
+variable-length arrays.
+
+Note that GCC may optimize small variable-length arrays of a known
+value into plain arrays, so this warning may not get triggered for
+such arrays.
+
+This warning is not enabled by @option{-Wall}, and is only active when
+@option{-ftree-vrp} is active (default for @option{-O2} and above).
+
+See also @option{-Walloca-larger-than=@var{n}}.
@item -Wvolatile-register-var
@opindex Wvolatile-register-var
diff --git a/gcc/gimple-ssa-warn-alloca.c b/gcc/gimple-ssa-warn-alloca.c
new file mode 100644
index 0000000..53b0b30
--- /dev/null
+++ b/gcc/gimple-ssa-warn-alloca.c
@@ -0,0 +1,534 @@
+/* Warn on problematic uses of alloca and variable length arrays.
+ Copyright (C) 2016 Free Software Foundation, Inc.
+ Contributed by Aldy Hernandez <aldyh@redhat.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "gimple-pretty-print.h"
+#include "diagnostic-core.h"
+#include "fold-const.h"
+#include "gimple-iterator.h"
+#include "tree-ssa.h"
+#include "params.h"
+#include "tree-cfg.h"
+#include "calls.h"
+#include "cfgloop.h"
+#include "intl.h"
+
+const pass_data pass_data_walloca = {
+ GIMPLE_PASS,
+ "walloca",
+ OPTGROUP_NONE,
+ TV_NONE,
+ PROP_cfg, // properties_required
+ 0, // properties_provided
+ 0, // properties_destroyed
+ 0, // properties_start
+ 0, // properties_finish
+};
+
+class pass_walloca : public gimple_opt_pass
+{
+public:
+ pass_walloca (gcc::context *ctxt)
+ : gimple_opt_pass(pass_data_walloca, ctxt), first_time_p (false)
+ {}
+ opt_pass *clone () { return new pass_walloca (m_ctxt); }
+ void set_pass_param (unsigned int n, bool param)
+ {
+ gcc_assert (n == 0);
+ first_time_p = param;
+ }
+ virtual bool gate (function *);
+ virtual unsigned int execute (function *);
+
+ private:
+ // Set to TRUE the first time we run this pass on a function.
+ bool first_time_p;
+};
+
+bool
+pass_walloca::gate (function *fun ATTRIBUTE_UNUSED)
+{
+ // The first time this pass is called, it is called before
+ // optimizations have been run and range information is unavailable,
+ // so we can only perform strict alloca checking.
+ if (first_time_p)
+ return warn_alloca != 0;
+
+ return warn_alloca_limit > 0 || warn_vla_limit > 0;
+}
+
+// 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 argument is 0.
+ ALLOCA_ARG_IS_ZERO,
+
+ // Alloca call is unbounded. That is, there is no controlling
+ // predicate for its argument.
+ ALLOCA_UNBOUNDED
+};
+
+// Type of an alloca call with its corresponding limit, if applicable.
+struct alloca_type_and_limit {
+ enum alloca_type type;
+ // For ALLOCA_BOUND_MAYBE_LARGE and ALLOCA_BOUND_DEFINITELY_LARGE
+ // types, this field indicates the assumed limit if known or
+ // integer_zero_node if unknown. For any other alloca types, this
+ // field is undefined.
+ wide_int limit;
+ alloca_type_and_limit ();
+ alloca_type_and_limit (enum alloca_type type,
+ wide_int i) : type(type), limit(i) { }
+ alloca_type_and_limit (enum alloca_type type) : type(type) { }
+};
+
+// NOTE: When we get better range info, this entire function becomes
+// irrelevant, as it should be possible to get range info for an SSA
+// name at any point in the program.
+//
+// We have a few heuristics up our sleeve to determine if a call to
+// alloca() is within bounds. Try them out and return the type of
+// alloca call with its assumed limit (if applicable).
+//
+// Given a known argument (ARG) to alloca() and an EDGE (E)
+// calculating said argument, verify that the last statement in the BB
+// in E->SRC is a gate comparing ARG to an acceptable bound for
+// alloca(). See examples below.
+//
+// If set, ARG_CASTED is the possible unsigned argument to which ARG
+// was casted to. This is to handle cases where the controlling
+// predicate is looking at a casted value, not the argument itself.
+// arg_casted = (size_t) arg;
+// if (arg_casted < N)
+// goto bb3;
+// else
+// goto bb5;
+//
+// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
+// in bytes we allow for arg.
+
+static struct alloca_type_and_limit
+alloca_call_type_by_arg (tree arg, tree arg_casted, edge e, unsigned max_size)
+{
+ basic_block bb = e->src;
+ gimple_stmt_iterator gsi = gsi_last_bb (bb);
+ gimple *last = gsi_stmt (gsi);
+ if (!last || gimple_code (last) != GIMPLE_COND)
+ return alloca_type_and_limit (ALLOCA_UNBOUNDED);
+
+ enum tree_code cond_code = gimple_cond_code (last);
+ if (e->flags & EDGE_TRUE_VALUE)
+ ;
+ else if (e->flags & EDGE_FALSE_VALUE)
+ cond_code = invert_tree_comparison (cond_code, false);
+ else
+ return alloca_type_and_limit (ALLOCA_UNBOUNDED);
+
+ // Check for:
+ // if (ARG .COND. N)
+ // goto <bb 3>;
+ // else
+ // goto <bb 4>;
+ // <bb 3>:
+ // alloca(ARG);
+ if ((cond_code == LE_EXPR
+ || cond_code == LT_EXPR
+ || cond_code == GT_EXPR
+ || cond_code == GE_EXPR)
+ && (gimple_cond_lhs (last) == arg
+ || gimple_cond_lhs (last) == arg_casted))
+ {
+ if (TREE_CODE (gimple_cond_rhs (last)) == INTEGER_CST)
+ {
+ tree rhs = gimple_cond_rhs (last);
+ int tst = wi::cmpu (wi::to_widest (rhs), max_size);
+ if ((cond_code == LT_EXPR && tst == -1)
+ || (cond_code == LE_EXPR && (tst == -1 || tst == 0)))
+ return alloca_type_and_limit (ALLOCA_OK);
+ else
+ {
+ // Let's not get too specific as to how large the limit
+ // may be. Someone's clearly an idiot when things
+ // degrade into "if (N > Y) alloca(N)".
+ if (cond_code == GT_EXPR || cond_code == GE_EXPR)
+ rhs = integer_zero_node;
+ return alloca_type_and_limit (ALLOCA_BOUND_MAYBE_LARGE, rhs);
+ }
+ }
+ else
+ return alloca_type_and_limit (ALLOCA_BOUND_UNKNOWN);
+ }
+
+ // Similarly, but check for a comparison with an unknown LIMIT.
+ // if (LIMIT .COND. ARG)
+ // alloca(arg);
+ //
+ // Where LIMIT has a bound of unknown range.
+ //
+ // Note: All conditions of the form (ARG .COND. XXXX) where covered
+ // by the previous check above, so we only need to look for (LIMIT
+ // .COND. ARG) here.
+ tree limit = gimple_cond_lhs (last);
+ if ((gimple_cond_rhs (last) == arg
+ || gimple_cond_rhs (last) == arg_casted)
+ && TREE_CODE (limit) == SSA_NAME)
+ {
+ wide_int min, max;
+ value_range_type range_type = get_range_info (limit, &min, &max);
+
+ if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
+ return alloca_type_and_limit (ALLOCA_BOUND_UNKNOWN);
+
+ // ?? It looks like the above `if' is unnecessary, as we never
+ // get any VR_RANGE or VR_ANTI_RANGE here. If we had a range
+ // for LIMIT, I suppose we would have taken care of it in
+ // alloca_call_type(), or handled above where we handle (ARG .COND. N).
+ //
+ // If this ever triggers, figure out why and handle it, though
+ // it is likely to be just an ALLOCA_UNBOUNDED.
+ gcc_unreachable ();
+ }
+
+ return alloca_type_and_limit (ALLOCA_UNBOUNDED);
+}
+
+// Return TRUE if SSA's definition is a cast from a signed type.
+// If so, set *INVALID_CASTED_TYPE to the signed type.
+
+static bool
+cast_from_signed_p (tree ssa, tree *invalid_casted_type)
+{
+ gimple *def = SSA_NAME_DEF_STMT (ssa);
+ if (def
+ && !gimple_nop_p (def)
+ && gimple_assign_cast_p (def)
+ && !TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def))))
+ {
+ *invalid_casted_type = TREE_TYPE (gimple_assign_rhs1 (def));
+ return true;
+ }
+ return false;
+}
+
+// Return TRUE if X has a maximum range of MAX, basically covering the
+// entire domain, in which case it's no range at all.
+
+static bool
+is_max (tree x, wide_int max)
+{
+ return wi::max_value (TREE_TYPE (x)) == max;
+}
+
+// Analyze the alloca call in STMT and return the alloca type with its
+// corresponding limit (if applicable). IS_VLA is set if the alloca
+// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
+//
+// If the alloca call may be too large because of a cast from a signed
+// type to an unsigned type, set *INVALID_CASTED_TYPE to the
+// problematic signed type.
+
+static struct alloca_type_and_limit
+alloca_call_type (gimple *stmt, bool is_vla, tree *invalid_casted_type)
+{
+ gcc_assert (gimple_alloca_call_p (stmt));
+ tree len = gimple_call_arg (stmt, 0);
+ tree len_casted = NULL;
+ wide_int min, max;
+ struct alloca_type_and_limit ret = alloca_type_and_limit (ALLOCA_UNBOUNDED);
+
+ gcc_assert (!is_vla || warn_vla_limit > 0);
+ gcc_assert (is_vla || warn_alloca_limit > 0);
+
+ // Adjust warn_alloca_max_size for VLAs, by taking the underlying
+ // type into account.
+ unsigned HOST_WIDE_INT max_size;
+ if (is_vla)
+ max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
+ else
+ max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
+
+ // Check for the obviously bounded case.
+ if (TREE_CODE (len) == INTEGER_CST)
+ {
+ if (tree_to_uhwi (len) > max_size)
+ return alloca_type_and_limit (ALLOCA_BOUND_DEFINITELY_LARGE, len);
+ if (integer_zerop (len))
+ return alloca_type_and_limit (ALLOCA_ARG_IS_ZERO);
+ ret = alloca_type_and_limit (ALLOCA_OK);
+ }
+ else if (TREE_CODE (len) != SSA_NAME)
+ {
+ // We only run with optimization and we have yet to trigger
+ // anything but an SSA_NAME here. Fail here just in case we get
+ // a non SSA_NAME here in the future, though I doubt it will
+ // hold anything of value, since we need range information for
+ // anything to make sense.
+ gcc_unreachable ();
+ return alloca_type_and_limit (ALLOCA_UNBOUNDED);
+ }
+ // Check the range info if available.
+ else if (value_range_type range_type = get_range_info (len, &min, &max))
+ {
+ if (range_type == VR_RANGE)
+ {
+ if (wi::leu_p (max, max_size))
+ ret = alloca_type_and_limit (ALLOCA_OK);
+ else
+ {
+ // A cast may have created a range we don't care
+ // about. For instance, a cast from 16-bit to
+ // 32-bit creates a range of 0..65535, even if there
+ // is not really a determinable range in the
+ // underlying code. In this case, look through the
+ // cast at the original argument, and fall through
+ // to look at other alternatives.
+ //
+ // We only look at through the cast when its from
+ // unsigned to unsigned, otherwise we may risk
+ // looking at SIGNED_INT < N, which is clearly not
+ // what we want. In this case, we'd be interested
+ // in a VR_RANGE of [0..N].
+ //
+ // Note: None of this is perfect, and should all go
+ // away with better range information. But it gets
+ // most of the cases.
+ gimple *def = SSA_NAME_DEF_STMT (len);
+ if (gimple_assign_cast_p (def)
+ && TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def))))
+
+ {
+ len_casted = gimple_assign_rhs1 (def);
+ range_type = get_range_info (len_casted, &min, &max);
+ }
+ // An unknown range or a range of the entire domain is
+ // really no range at all.
+ if (range_type == VR_VARYING
+ || (!len_casted && is_max (len, max))
+ || (len_casted && is_max (len_casted, max)))
+ {
+ // Fall through.
+ }
+ else if (range_type != VR_VARYING)
+ return
+ alloca_type_and_limit (ALLOCA_BOUND_MAYBE_LARGE, max);
+ }
+ }
+ else if (range_type == VR_ANTI_RANGE)
+ {
+ // There may be some wrapping around going on. Catch it
+ // with this heuristic. Hopefully, this VR_ANTI_RANGE
+ // nonsense will go away, and we won't have to catch the
+ // sign conversion problems with this crap.
+ if (cast_from_signed_p (len, invalid_casted_type))
+ return alloca_type_and_limit (ALLOCA_CAST_FROM_SIGNED);
+ }
+ // No easily determined range and try other things.
+ }
+
+ // If we couldn't find anything, try a few heuristics for things we
+ // can easily determine. Check these misc cases but only accept
+ // them if all predecessors have a known bound.
+ basic_block bb = gimple_bb (stmt);
+ if (ret.type == ALLOCA_UNBOUNDED)
+ {
+ ret.type = ALLOCA_OK;
+ for (unsigned ix = 0; ix < EDGE_COUNT (bb->preds); ix++)
+ {
+ gcc_assert (!len_casted || TYPE_UNSIGNED (TREE_TYPE (len_casted)));
+ ret = alloca_call_type_by_arg (len, len_casted,
+ EDGE_PRED (bb, ix), max_size);
+ if (ret.type != ALLOCA_OK)
+ return ret;
+ }
+ }
+
+ return ret;
+}
+
+// Return TRUE if the alloca call in STMT is in a loop, otherwise
+// return FALSE. As an exception, ignore alloca calls for VLAs that
+// occur in a loop since those will be cleaned up when they go out of
+// scope.
+
+static bool
+in_loop_p (bool is_vla, gimple *stmt)
+{
+ basic_block bb = gimple_bb (stmt);
+ if (bb->loop_father
+ && bb->loop_father->header != ENTRY_BLOCK_PTR_FOR_FN (cfun))
+ {
+ // 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.
+ tree fndecl = gimple_call_fn (stmt);
+ while (TREE_CODE (fndecl) == ADDR_EXPR)
+ fndecl = TREE_OPERAND (fndecl, 0);
+ if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+ && is_vla
+ && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_ALLOCA_WITH_ALIGN)
+ return false;
+
+ return true;
+ }
+ return false;
+}
+
+unsigned int
+pass_walloca::execute (function *fun)
+{
+ basic_block bb;
+ FOR_EACH_BB_FN (bb, fun)
+ {
+ for (gimple_stmt_iterator si = gsi_start_bb (bb); !gsi_end_p (si);
+ gsi_next (&si))
+ {
+ gimple *stmt = gsi_stmt (si);
+ location_t loc = gimple_location (stmt);
+
+ if (!gimple_alloca_call_p (stmt))
+ continue;
+ gcc_assert (gimple_call_num_args (stmt) >= 1);
+
+ bool is_vla = gimple_alloca_call_p (stmt)
+ && gimple_call_alloca_for_var_p (as_a <gcall *> (stmt));
+
+ // Strict mode whining for VLAs is handled by the front-end,
+ // so we can safely ignore this case. Also, ignore VLAs if
+ // the user doesn't care about them.
+ if (is_vla
+ && (warn_vla > 0 || !warn_vla_limit))
+ continue;
+
+ if (!is_vla && (warn_alloca || !warn_alloca_limit))
+ {
+ if (warn_alloca)
+ warning_at (loc, OPT_Walloca, G_("use of %<alloca%>"));
+ continue;
+ }
+
+ tree invalid_casted_type = NULL;
+ struct alloca_type_and_limit t
+ = alloca_call_type (stmt, is_vla, &invalid_casted_type);
+
+ // Even if we think the alloca call is OK, make sure it's
+ // not in a loop.
+ if (t.type == ALLOCA_OK && in_loop_p (is_vla, stmt))
+ t = alloca_type_and_limit (ALLOCA_IN_LOOP);
+
+ enum opt_code wcode
+ = is_vla ? OPT_Wvla_larger_than_ : OPT_Walloca_larger_than_;
+ char buff[WIDE_INT_MAX_PRECISION / 4 + 4];
+ switch (t.type)
+ {
+ case ALLOCA_OK:
+ break;
+ case ALLOCA_BOUND_MAYBE_LARGE:
+ if (warning_at (loc, wcode,
+ is_vla ? G_("argument to variable-length array "
+ "may be too large")
+ : G_("argument to %<alloca%> may be too large"))
+ && t.limit != integer_zero_node)
+ {
+ print_decu (t.limit, buff);
+ inform (loc, G_("limit is %u bytes, but argument "
+ "may be as large as %s"),
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+ }
+ break;
+ case ALLOCA_BOUND_DEFINITELY_LARGE:
+ if (warning_at (loc, wcode,
+ is_vla ? G_("argument to variable-length array "
+ "is too large")
+ : G_("argument to %<alloca%> is too large"))
+ && t.limit != integer_zero_node)
+ {
+ print_decu (t.limit, buff);
+ inform (loc, G_("limit is %u bytes, but argument is %s"),
+ is_vla ? warn_vla_limit : warn_alloca_limit, buff);
+ }
+ break;
+ case ALLOCA_BOUND_UNKNOWN:
+ warning_at (loc, wcode,
+ is_vla ? G_("variable-length array bound is unknown")
+ : G_("%<alloca%> bound is unknown"));
+ break;
+ case ALLOCA_UNBOUNDED:
+ warning_at (loc, wcode,
+ is_vla ? G_("unbounded use of variable-length array")
+ : G_("unbounded use of %<alloca%>"));
+ break;
+ case ALLOCA_IN_LOOP:
+ gcc_assert (!is_vla);
+ warning_at (loc, wcode, G_("use of %<alloca%> within a loop"));
+ break;
+ case ALLOCA_CAST_FROM_SIGNED:
+ gcc_assert (invalid_casted_type != NULL_TREE);
+ warning_at (loc, wcode,
+ is_vla ? G_("argument to variable-length array "
+ "may be too large due to "
+ "conversion from %qT to %qT")
+ : G_("argument to %<alloca%> may be too large "
+ "due to conversion from %qT to %qT"),
+ invalid_casted_type, size_type_node);
+ break;
+ case ALLOCA_ARG_IS_ZERO:
+ warning_at (loc, wcode,
+ is_vla ? G_("argument to variable-length array "
+ "is zero")
+ : G_("argument to %<alloca%> is zero"));
+ break;
+ default:
+ gcc_unreachable ();
+ }
+ }
+ }
+ return 0;
+}
+
+gimple_opt_pass *
+make_pass_walloca (gcc::context *ctxt)
+{
+ return new pass_walloca (ctxt);
+}
diff --git a/gcc/passes.def b/gcc/passes.def
index 3647e90..e2cb40e 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. If not see
NEXT_PASS (pass_warn_function_return);
NEXT_PASS (pass_expand_omp);
NEXT_PASS (pass_build_cgraph_edges);
+ NEXT_PASS (pass_walloca, true);
TERMINATE_PASS_LIST (all_lowering_passes)
/* Interprocedural optimization passes. */
@@ -247,6 +248,7 @@ along with GCC; see the file COPYING3. If not see
NEXT_PASS (pass_laddress);
NEXT_PASS (pass_lim);
NEXT_PASS (pass_split_crit_edges);
+ NEXT_PASS (pass_walloca, false);
NEXT_PASS (pass_pre);
NEXT_PASS (pass_sink_code);
NEXT_PASS (pass_sancov);
diff --git a/gcc/testsuite/gcc.dg/Walloca-1.c b/gcc/testsuite/gcc.dg/Walloca-1.c
new file mode 100644
index 0000000..34a20c3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-1.c
@@ -0,0 +1,63 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+#define alloca __builtin_alloca
+
+typedef __SIZE_TYPE__ size_t;
+extern size_t strlen(const char *);
+
+extern void useit (char *);
+
+int num;
+
+void foo1 (size_t len, size_t len2, size_t len3)
+{
+ int i;
+
+ for (i=0; i < 123; ++i)
+ {
+ char *s = alloca (566); /* { dg-warning "'alloca' within a loop" } */
+ useit (s);
+ }
+
+ char *s = alloca (123);
+ useit (s); // OK, constant argument to alloca
+
+ s = alloca (num); // { dg-warning "large due to conversion" }
+ useit (s);
+
+ s = alloca(90000); /* { dg-warning "is too large" } */
+ useit (s);
+
+ if (len < 2000)
+ {
+ s = alloca(len); // OK, bounded
+ useit (s);
+ }
+
+ if (len + len2 < 2000) // OK, bounded
+ {
+ s = alloca(len + len2);
+ useit (s);
+ }
+
+ if (len3 <= 2001)
+ {
+ s = alloca(len3); /* { dg-warning "may be too large" } */
+ useit(s);
+ }
+}
+
+void foo2 (__SIZE_TYPE__ len)
+{
+ // Test that a direct call to __builtin_alloca_with_align is not confused
+ // with a VLA.
+ void *p = __builtin_alloca_with_align (len, 8); // { dg-warning "unbounded use of 'alloca'" }
+ useit (p);
+}
+
+void foo3 (unsigned char a)
+{
+ if (a == 0)
+ useit (__builtin_alloca (a)); // { dg-warning "argument to 'alloca' is zero" }
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-10.c b/gcc/testsuite/gcc.dg/Walloca-10.c
new file mode 100644
index 0000000..69549fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-10.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+// Test when the conditionals are incorrectly reversed.
+
+void f (void *);
+void foo (__SIZE_TYPE__ len)
+{
+ void *p;
+ if (len < 500)
+ p = __builtin_malloc (len);
+ else
+ p = __builtin_alloca (len); // { dg-warning "argument to .alloca. may be too large" }
+ f (p);
+}
+
+void bar (__SIZE_TYPE__ len)
+{
+ void *p;
+ if (len > 500)
+ p = __builtin_alloca (len); // { dg-warning "argument to .alloca. may be too large" }
+ else
+ p = __builtin_malloc (len);
+ f (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-2.c b/gcc/testsuite/gcc.dg/Walloca-2.c
new file mode 100644
index 0000000..284b34e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-2.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+void f (void *);
+
+void
+g1 (int n)
+{
+ void *p;
+ if (n > 0 && n < 2000)
+ p = __builtin_alloca (n);
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
+
+void
+g2 (int n)
+{
+ void *p;
+ if (n < 2000)
+ p = __builtin_alloca (n); // { dg-warning "large due to conversion" }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
+
+void
+g3 (int n)
+{
+ void *p;
+ if (n > 0 && n < 3000)
+ {
+ p = __builtin_alloca (n); // { dg-warning "'alloca' may be too large" }
+ // { dg-message "note:.*argument may be as large as 2999" "note" { target *-*-* } 34 }
+ }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-3.c b/gcc/testsuite/gcc.dg/Walloca-3.c
new file mode 100644
index 0000000..5345197
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-3.c
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+void f (void *);
+
+__SIZE_TYPE__ LIMIT;
+
+// Warn when there is an alloca bound, but it is an unknown bound.
+
+void
+g1 (__SIZE_TYPE__ n)
+{
+ void *p;
+ if (n < LIMIT)
+ p = __builtin_alloca (n); // { dg-warning "'alloca' bound is unknown" }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
+
+// Similar to the above, but do not get confused by the upcast.
+
+unsigned short SHORT_LIMIT;
+void
+g2 (unsigned short n)
+{
+ void *p;
+ if (n < SHORT_LIMIT)
+ p = __builtin_alloca (n); // { dg-warning "'alloca' bound is unknown" }
+ else
+ p = __builtin_malloc (n);
+ f (p);
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-4.c b/gcc/testsuite/gcc.dg/Walloca-4.c
new file mode 100644
index 0000000..a559534
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-4.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=5000 -O2" } */
+
+ char *
+ _i18n_number_rewrite (char *w, char *rear_ptr)
+{
+
+ char *src;
+ _Bool
+ use_alloca = (((rear_ptr - w) * sizeof (char)) < 4096U);
+ if (use_alloca)
+ src = (char *) __builtin_alloca ((rear_ptr - w) * sizeof (char));
+ else
+ src = (char *) __builtin_malloc ((rear_ptr - w) * sizeof (char));
+ return src;
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-5.c b/gcc/testsuite/gcc.dg/Walloca-5.c
new file mode 100644
index 0000000..5ed1171
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-5.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=123 -O2" } */
+/* { dg-xfail-if "Currently broken but Andrew's work should fix this" { *-*-* } } */
+
+/* The argument to alloca ends up having a range of 0..MAXINT(32bits),
+ so we think we have a range because of the upcast. Consequently,
+ we warn with "alloca may be too large", but we should technically
+ warn with "unbounded use of alloca".
+
+ We currently drill through casts to figure this stuff out, but we
+ get confused because it's not just a cast. It's a cast, plus a
+ multiply.
+
+ <bb 2>:
+ # RANGE [0, 4294967295] NONZERO 4294967295
+ _1 = (long unsigned int) something_4(D);
+ # RANGE [0, 34359738360] NONZERO 34359738360
+ _2 = _1 * 8;
+ _3 = __builtin_alloca (_2);
+
+ I don't know whether it's even worth such fine-grained warnings.
+ Perhaps we should generically warn everywhere with "alloca may be
+ too large".
+
+ I'm hoping that this particular case will be easier to diagnose with
+ Andrew's work. */
+
+void useit(void *);
+void foobar(unsigned int something)
+{
+ useit(__builtin_alloca (something * sizeof (const char *))); // { dg-warning "unbounded use of alloca" "" { xfail *-*-* } }
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-6.c b/gcc/testsuite/gcc.dg/Walloca-6.c
new file mode 100644
index 0000000..b4d8d41
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-6.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=256 -O2" } */
+/* { dg-xfail-if "Currently broken but Andrew's work should fix this" { *-*-* } } */
+
+void f (void*);
+void g (__SIZE_TYPE__ n)
+{
+ // No warning on this case. Range is easily determinable.
+ if (n > 0 && n < 256)
+ f (__builtin_alloca (n));
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-7.c b/gcc/testsuite/gcc.dg/Walloca-7.c
new file mode 100644
index 0000000..d6581a5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-7.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca -O0" } */
+
+extern void f(void *);
+
+void foo(void)
+{
+ // Test that strict -Walloca works even without optimization.
+ f (__builtin_alloca(500)); // { dg-warning "use of 'alloca'" }
+}
+
+void bar(void)
+{
+ // Test that we warn on alloca() calls, not just __builtin_alloca calls.
+ extern void *alloca(__SIZE_TYPE__);
+ f (alloca (123)); // { dg-warning "use of 'alloca'" }
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-8.c b/gcc/testsuite/gcc.dg/Walloca-8.c
new file mode 100644
index 0000000..a4a1204
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-8.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=2000 -O2" } */
+
+void *p;
+void
+foo (__SIZE_TYPE__ len)
+{
+ if (len < 2000 / sizeof (void *))
+ p = __builtin_alloca (len * sizeof (void *));
+ else
+ p = __builtin_malloc (len * sizeof (void *));
+}
diff --git a/gcc/testsuite/gcc.dg/Walloca-9.c b/gcc/testsuite/gcc.dg/Walloca-9.c
new file mode 100644
index 0000000..c67d9d6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-9.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca-larger-than=5000 -O2" } */
+
+extern void useit(char *);
+
+int
+foobar (unsigned short length)
+{
+ char *pbuf;
+ __SIZE_TYPE__ size = (__SIZE_TYPE__) length;
+
+ if (size < 4032)
+ pbuf = (char *) __builtin_alloca(size);
+ else
+ pbuf = (char *) __builtin_malloc (size);
+
+ useit(pbuf);
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-1.c b/gcc/testsuite/gcc.dg/Wvla-1.c
new file mode 100644
index 0000000..384c930
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-1.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-Wvla-larger-than=100 -O2" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void useit (char *);
+
+int num;
+
+void test_vlas (size_t num)
+{
+ char str2[num]; /* { dg-warning "unbounded use" } */
+ useit(str2);
+
+ num = 98;
+ for (int i=0; i < 1234; ++i) {
+ char str[num]; // OK, VLA in a loop, but it is a
+ // known size *AND* the compiler takes
+ // care of cleaning up between
+ // iterations with
+ // __builtin_stack_restore.
+ useit(str);
+ }
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-2.c b/gcc/testsuite/gcc.dg/Wvla-2.c
new file mode 100644
index 0000000..96814dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-2.c
@@ -0,0 +1,70 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target stdint_types } */
+/* { dg-options "-O2 -Wvla-larger-than=40" } */
+
+#include <stdint.h>
+
+void f0 (void *);
+void
+f1 (__SIZE_TYPE__ a)
+{
+ if (a <= 10)
+ {
+ // 10 * 4 bytes = 40: OK!
+ uint32_t x[a];
+ f0 (x);
+ }
+}
+
+void
+f2 (__SIZE_TYPE__ a)
+{
+ if (a <= 11)
+ {
+ // 11 * 4 bytes = 44: Not OK.
+ uint32_t x[a]; // { dg-warning "array may be too large" }
+ // { dg-message "note:.*argument may be as large as 44" "note" { target *-*-* } 25 }
+ f0 (x);
+ }
+}
+
+void
+f3 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
+{
+ if (a <= 5 && b <= 3)
+ {
+ // 5 * 3 * 4 bytes = 60: Not OK.
+ uint32_t x[a][b]; // { dg-warning "array may be too large" }
+ f0 (x);
+ }
+}
+
+void
+f4 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
+{
+ if (a <= 5 && b <= 2)
+ {
+ // 5 * 2 * 4 bytes = 40 bytes: OK!
+ uint32_t x[a][b];
+ f0 (x);
+ }
+}
+
+void
+f5 (__SIZE_TYPE__ len)
+{
+ // Test that a direct call to __builtin_alloca_with_align is not
+ // confused with a VLA.
+ void *p = __builtin_alloca_with_align (len, 8);
+ f0 (p);
+}
+
+void
+f6 (unsigned stuff)
+{
+ int n = 7000;
+ do {
+ char a[n]; // { dg-warning "variable-length array is too large" }
+ f0 (a);
+ } while (stuff--);
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-3.c b/gcc/testsuite/gcc.dg/Wvla-3.c
new file mode 100644
index 0000000..5124476
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-3.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca -O2" } */
+
+// Make sure we don't warn on VLA with -Walloca.
+
+void f (void*);
+
+void h1 (unsigned n)
+{
+ int a [n];
+ f (a);
+}
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 36299a6..57b61f4 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -469,6 +469,7 @@ extern simple_ipa_opt_pass *make_pass_ipa_oacc (gcc::context *ctxt);
extern simple_ipa_opt_pass *make_pass_ipa_oacc_kernels (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_gen_hsail (gcc::context *ctxt);
extern gimple_opt_pass *make_pass_warn_nonnull_compare (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_walloca (gcc::context *ctxt);
/* IPA Passes */
extern simple_ipa_opt_pass *make_pass_ipa_lower_emutls (gcc::context *ctxt);
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-08-19 14:59 ` Aldy Hernandez
@ 2016-09-13 20:30 ` Jeff Law
2016-10-18 21:42 ` Aldy Hernandez
0 siblings, 1 reply; 26+ messages in thread
From: Jeff Law @ 2016-09-13 20:30 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 08/19/2016 08:58 AM, Aldy Hernandez wrote:
>> I'd just drop the /*strict_mode_p*/ comment in both places it appears in
>> your patch's change to passes.def. I think we've generally frowned on
>> those embedded comments, even though some have snuck in.
>
> I've seen a lot of embedded comments throughout GCC, especially in
> optional type arguments. ISTM it makes things clearer for these
> parameters. But hey, I don't care that much. Fixed.
Yea. They've crept in. Long term this kind of stuff should have an
enum or somesuch so that its obvious at both the call site and
implementation site what those arguments to.
>
> Actually when the cast is from a known positive range we don't get a
> VR_ANTI_RANGE, we get a proper VR_RANGE.
Ah, in that case there's several of these things that get trivially
cleaned up :-)
Though I bet with some work I might be able to create an ANTI_RANGE that
excludes all negative numbers. But I don't think it's going to be that
important in practice. So we just have to make sure we don't
abort/segfault.
>
> I've cleaned this code up a bit and merged some common conditionals. In
> the process, taking a subset of your advice, and cleaning up some things
> I've managed to handle 2 cases where I was previously XFAILing.
> So...less false positives. More coverage. Woo hoo!
Always goodness.
>
>>
>>> +{
>>> + gcc_assert (gimple_alloca_call_p (stmt));
>>> + tree len = gimple_call_arg (stmt, 0);
>>> + enum alloca_type w = ALLOCA_UNBOUNDED;
>>> + wide_int min, max;
>>> +
>>> + gcc_assert (!is_vla || warn_vla_limit > 0);
>>> + gcc_assert (is_vla || warn_alloca_limit > 0);
>>> +
>>> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
>>> + // type into account.
>>> + unsigned HOST_WIDE_INT max_size;
>>> + if (is_vla)
>>> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
>>> + else
>>> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
>>> +
>>> + // Check for the obviously bounded case.
>>> + if (TREE_CODE (len) == INTEGER_CST)
>>> + {
>>> + if (tree_to_uhwi (len) > max_size)
>>> + {
>>> + *assumed_limit = len;
>>> + return ALLOCA_BOUND_DEFINITELY_LARGE;
>>> + }
>>> + if (integer_zerop (len))
>>> + return ALLOCA_ARG_IS_ZERO;
>>> + w = ALLOCA_OK;
>>> + }
>>> + else if (TREE_CODE (len) != SSA_NAME)
>>> + return ALLOCA_UNBOUNDED;
>> Hmm, other than INTEGER_CST and SSA_NAME, is there any other nodes we
>> can get here? Perhaps we get DECLs and such, particularly when not
>> optimizing?!?
>
> Nope. We don't even run without optimization (because we need VRP/range
> info). I added a gcc_unreachable() to make sure and added an
> appropriate comment. It doesn't get triggered on tests or
> glibc/binutils builds.
Yea, maybe I was conflating your work at Martin's (the latter of which
can run without optimization).
>> So this is an interesting tidbit that answers my questions about how we
>> use alloca_call_type_by_arg. Essentially this is meant to catch the
>> merge point for flow control and give a conservative warning. That's
>> fine and good. But ISTM it's really a bit of a hack. What if we have
>> something like this:
>>
>> X Y Z
>> \ | /
>> \|/
>> A
>> / \
>> / \
>> B C
>> / \
>> / \
>> D E
>>
>>
>> Where the alloca call is in E and the incoming edges to A actually have
>> useful information about the argument to the alloca call.
>>
>> ISTM you need to be doing something with the dominator tree here to find
>> the merge point(s) where we might know something useful. And it's this
>> kind of test that makes me wonder about re-purposing some of the path
>> analysis code from tree-ssa-uninit.c. It may be the case that the path
>> Z->A->C->E is unfeasible, but left in the CFG because duplication to
>> expose the unfeasible path was unprofitable. If it turns out that the
>> only argument that causes problems comes from the edge Z->A, then we
>> wouldn't want to warn in this case.
>>
>> I don't see Andrew's work necessarily being able to solve this problem.
>
> In my limited testing I've seen that 95% of all cases (I'm pulling this
> number out of thin air ;-)) are relatively simple. Just looking at the
> definition of the SSA name in the alloca() call or the immediate
> predecessors yields most of the information we need. I haven't seen
> much more complicated things with an actual range.
Understood. But it's this kind of thing that creates the false
positives that drive people nuts :-)
As I said, I don't necessarily think Andrew's work will resolve this nor
do I think it ought to block this work. I mostly pointed it out as an
example of the kind of case we're not going to handle well today, but
perhaps could with the tree-ssa-uninit.c framework.
>
>
> 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-alloca.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.
>
> @@ -4666,6 +4667,70 @@ annotations.
>> +
> +
> +Unbounded uses, on the other hand, are uses of @code{alloca} with no
> +controlling predicate verifying its size. For example:
> +
> +@smallexample
> +stuff ();
> +p = alloca (n);
> +@end smallexample
Consider making this a full example.
> diff --git a/gcc/gimple-ssa-warn-alloca.c b/gcc/gimple-ssa-warn-alloca.c
> new file mode 100644
> index 0000000..53b0b30
> --- /dev/null
> +++ b/gcc/gimple-ssa-warn-alloca.c
> +// NOTE: When we get better range info, this entire function becomes
> +// irrelevant, as it should be possible to get range info for an SSA
> +// name at any point in the program.
> +//
> +// We have a few heuristics up our sleeve to determine if a call to
> +// alloca() is within bounds. Try them out and return the type of
> +// alloca call with its assumed limit (if applicable).
> +//
> +// Given a known argument (ARG) to alloca() and an EDGE (E)
> +// calculating said argument, verify that the last statement in the BB
> +// in E->SRC is a gate comparing ARG to an acceptable bound for
> +// alloca(). See examples below.
> +//
> +// If set, ARG_CASTED is the possible unsigned argument to which ARG
> +// was casted to. This is to handle cases where the controlling
> +// predicate is looking at a casted value, not the argument itself.
> +// arg_casted = (size_t) arg;
> +// if (arg_casted < N)
> +// goto bb3;
> +// else
> +// goto bb5;
> +//
> +// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
> +// in bytes we allow for arg.
> +
> +static struct alloca_type_and_limit
> +alloca_call_type_by_arg (tree arg, tree arg_casted, edge e, unsigned max_size)
> +{
> + basic_block bb = e->src;
> + gimple_stmt_iterator gsi = gsi_last_bb (bb);
> + gimple *last = gsi_stmt (gsi);
> + if (!last || gimple_code (last) != GIMPLE_COND)
> + return alloca_type_and_limit (ALLOCA_UNBOUNDED);
> +
> +
> + // Check for:
> + // if (ARG .COND. N)
> + // goto <bb 3>;
> + // else
> + // goto <bb 4>;
> + // <bb 3>:
> + // alloca(ARG);
> + // Similarly, but check for a comparison with an unknown LIMIT.
> + // if (LIMIT .COND. ARG)
> + // alloca(arg);
> + //
> + // Where LIMIT has a bound of unknown range.
> + //
> + // Note: All conditions of the form (ARG .COND. XXXX) where covered
> + // by the previous check above, so we only need to look for (LIMIT
> + // .COND. ARG) here.
> + tree limit = gimple_cond_lhs (last);
> + if ((gimple_cond_rhs (last) == arg
> + || gimple_cond_rhs (last) == arg_casted)
> + && TREE_CODE (limit) == SSA_NAME)
> + {
> + wide_int min, max;
> + value_range_type range_type = get_range_info (limit, &min, &max);
> +
> + if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
> + return alloca_type_and_limit (ALLOCA_BOUND_UNKNOWN);
> +
> + // ?? It looks like the above `if' is unnecessary, as we never
> + // get any VR_RANGE or VR_ANTI_RANGE here. If we had a range
> + // for LIMIT, I suppose we would have taken care of it in
> + // alloca_call_type(), or handled above where we handle (ARG .COND. N).
> + //
> + // If this ever triggers, figure out why and handle it, though
> + // it is likely to be just an ALLOCA_UNBOUNDED.
> + gcc_unreachable ();
So this seems like a case of "we don't expect this, though it might in
theory happen". I think degrading gracefully to ALLOCA_UNBOUNDED is a
better choice for this kind of situation than gcc_unreachable. If we're
generating a dump file, then perhaps saying something in the dump file
about the unhandled case would be useful.
> +
> +// Analyze the alloca call in STMT and return the alloca type with its
> +// corresponding limit (if applicable). IS_VLA is set if the alloca
> +// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
> +//
> +// If the alloca call may be too large because of a cast from a signed
> +// type to an unsigned type, set *INVALID_CASTED_TYPE to the
> +// problematic signed type.
> +
> +static struct alloca_type_and_limit
> +alloca_call_type (gimple *stmt, bool is_vla, tree *invalid_casted_type)
> +{
> + gcc_assert (gimple_alloca_call_p (stmt));
> + tree len = gimple_call_arg (stmt, 0);
> + tree len_casted = NULL;
> + wide_int min, max;
> + struct alloca_type_and_limit ret = alloca_type_and_limit (ALLOCA_UNBOUNDED);
> +
> + gcc_assert (!is_vla || warn_vla_limit > 0);
> + gcc_assert (is_vla || warn_alloca_limit > 0);
> +
> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
> + // type into account.
> + unsigned HOST_WIDE_INT max_size;
> + if (is_vla)
> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
> + else
> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
> +
> + // Check for the obviously bounded case.
> + if (TREE_CODE (len) == INTEGER_CST)
> + {
> + if (tree_to_uhwi (len) > max_size)
> + return alloca_type_and_limit (ALLOCA_BOUND_DEFINITELY_LARGE, len);
> + if (integer_zerop (len))
> + return alloca_type_and_limit (ALLOCA_ARG_IS_ZERO);
> + ret = alloca_type_and_limit (ALLOCA_OK);
> + }
> + else if (TREE_CODE (len) != SSA_NAME)
> + {
> + // We only run with optimization and we have yet to trigger
> + // anything but an SSA_NAME here. Fail here just in case we get
> + // a non SSA_NAME here in the future, though I doubt it will
> + // hold anything of value, since we need range information for
> + // anything to make sense.
> + gcc_unreachable ();
> + return alloca_type_and_limit (ALLOCA_UNBOUNDED);
> + }
In the gimple world, the arguments to an alloca should only be constants
and SSA_NAMES. My concern WRT _DECL nodes was only an issue if not
optimizing. So I think this else clause can just go away.
I think with the minor stuff noted above fixed, this is fine for the
trunk. I don't think it needs an additional review cycle. Sorry for
the long delay.
jeff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-09-13 20:30 ` Jeff Law
@ 2016-10-18 21:42 ` Aldy Hernandez
2016-10-19 7:08 ` Eric Botcazou
2016-10-19 10:45 ` Andreas Schwab
0 siblings, 2 replies; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-18 21:42 UTC (permalink / raw)
To: Jeff Law, gcc-patches
On 09/13/2016 04:28 PM, Jeff Law wrote:
> On 08/19/2016 08:58 AM, Aldy Hernandez wrote:
>
>>> I'd just drop the /*strict_mode_p*/ comment in both places it appears in
>>> your patch's change to passes.def. I think we've generally frowned on
>>> those embedded comments, even though some have snuck in.
>>
>> I've seen a lot of embedded comments throughout GCC, especially in
>> optional type arguments. ISTM it makes things clearer for these
>> parameters. But hey, I don't care that much. Fixed.
> Yea. They've crept in. Long term this kind of stuff should have an
> enum or somesuch so that its obvious at both the call site and
> implementation site what those arguments to.
>>
>> Actually when the cast is from a known positive range we don't get a
>> VR_ANTI_RANGE, we get a proper VR_RANGE.
> Ah, in that case there's several of these things that get trivially
> cleaned up :-)
>
> Though I bet with some work I might be able to create an ANTI_RANGE that
> excludes all negative numbers. But I don't think it's going to be that
> important in practice. So we just have to make sure we don't
> abort/segfault.
>
>>
>> I've cleaned this code up a bit and merged some common conditionals. In
>> the process, taking a subset of your advice, and cleaning up some things
>> I've managed to handle 2 cases where I was previously XFAILing.
>> So...less false positives. More coverage. Woo hoo!
> Always goodness.
>
>>
>>>
>>>> +{
>>>> + gcc_assert (gimple_alloca_call_p (stmt));
>>>> + tree len = gimple_call_arg (stmt, 0);
>>>> + enum alloca_type w = ALLOCA_UNBOUNDED;
>>>> + wide_int min, max;
>>>> +
>>>> + gcc_assert (!is_vla || warn_vla_limit > 0);
>>>> + gcc_assert (is_vla || warn_alloca_limit > 0);
>>>> +
>>>> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
>>>> + // type into account.
>>>> + unsigned HOST_WIDE_INT max_size;
>>>> + if (is_vla)
>>>> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
>>>> + else
>>>> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
>>>> +
>>>> + // Check for the obviously bounded case.
>>>> + if (TREE_CODE (len) == INTEGER_CST)
>>>> + {
>>>> + if (tree_to_uhwi (len) > max_size)
>>>> + {
>>>> + *assumed_limit = len;
>>>> + return ALLOCA_BOUND_DEFINITELY_LARGE;
>>>> + }
>>>> + if (integer_zerop (len))
>>>> + return ALLOCA_ARG_IS_ZERO;
>>>> + w = ALLOCA_OK;
>>>> + }
>>>> + else if (TREE_CODE (len) != SSA_NAME)
>>>> + return ALLOCA_UNBOUNDED;
>>> Hmm, other than INTEGER_CST and SSA_NAME, is there any other nodes we
>>> can get here? Perhaps we get DECLs and such, particularly when not
>>> optimizing?!?
>>
>> Nope. We don't even run without optimization (because we need VRP/range
>> info). I added a gcc_unreachable() to make sure and added an
>> appropriate comment. It doesn't get triggered on tests or
>> glibc/binutils builds.
> Yea, maybe I was conflating your work at Martin's (the latter of which
> can run without optimization).
>
>>> So this is an interesting tidbit that answers my questions about how we
>>> use alloca_call_type_by_arg. Essentially this is meant to catch the
>>> merge point for flow control and give a conservative warning. That's
>>> fine and good. But ISTM it's really a bit of a hack. What if we have
>>> something like this:
>>>
>>> X Y Z
>>> \ | /
>>> \|/
>>> A
>>> / \
>>> / \
>>> B C
>>> / \
>>> / \
>>> D E
>>>
>>>
>>> Where the alloca call is in E and the incoming edges to A actually have
>>> useful information about the argument to the alloca call.
>>>
>>> ISTM you need to be doing something with the dominator tree here to find
>>> the merge point(s) where we might know something useful. And it's this
>>> kind of test that makes me wonder about re-purposing some of the path
>>> analysis code from tree-ssa-uninit.c. It may be the case that the path
>>> Z->A->C->E is unfeasible, but left in the CFG because duplication to
>>> expose the unfeasible path was unprofitable. If it turns out that the
>>> only argument that causes problems comes from the edge Z->A, then we
>>> wouldn't want to warn in this case.
>>>
>>> I don't see Andrew's work necessarily being able to solve this
>>> problem.
>>
>> In my limited testing I've seen that 95% of all cases (I'm pulling this
>> number out of thin air ;-)) are relatively simple. Just looking at the
>> definition of the SSA name in the alloca() call or the immediate
>> predecessors yields most of the information we need. I haven't seen
>> much more complicated things with an actual range.
> Understood. But it's this kind of thing that creates the false
> positives that drive people nuts :-)
>
> As I said, I don't necessarily think Andrew's work will resolve this nor
> do I think it ought to block this work. I mostly pointed it out as an
> example of the kind of case we're not going to handle well today, but
> perhaps could with the tree-ssa-uninit.c framework.
>
>
>>
>>
>> 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-alloca.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.
>>
>> @@ -4666,6 +4667,70 @@ annotations.
>>> +
>> +
>> +Unbounded uses, on the other hand, are uses of @code{alloca} with no
>> +controlling predicate verifying its size. For example:
>> +
>> +@smallexample
>> +stuff ();
>> +p = alloca (n);
>> +@end smallexample
> Consider making this a full example.
>
>> diff --git a/gcc/gimple-ssa-warn-alloca.c b/gcc/gimple-ssa-warn-alloca.c
>> new file mode 100644
>> index 0000000..53b0b30
>> --- /dev/null
>> +++ b/gcc/gimple-ssa-warn-alloca.c
>
>> +// NOTE: When we get better range info, this entire function becomes
>> +// irrelevant, as it should be possible to get range info for an SSA
>> +// name at any point in the program.
>> +//
>> +// We have a few heuristics up our sleeve to determine if a call to
>> +// alloca() is within bounds. Try them out and return the type of
>> +// alloca call with its assumed limit (if applicable).
>> +//
>> +// Given a known argument (ARG) to alloca() and an EDGE (E)
>> +// calculating said argument, verify that the last statement in the BB
>> +// in E->SRC is a gate comparing ARG to an acceptable bound for
>> +// alloca(). See examples below.
>> +//
>> +// If set, ARG_CASTED is the possible unsigned argument to which ARG
>> +// was casted to. This is to handle cases where the controlling
>> +// predicate is looking at a casted value, not the argument itself.
>> +// arg_casted = (size_t) arg;
>> +// if (arg_casted < N)
>> +// goto bb3;
>> +// else
>> +// goto bb5;
>> +//
>> +// MAX_SIZE is WARN_ALLOCA= adjusted for VLAs. It is the maximum size
>> +// in bytes we allow for arg.
>> +
>> +static struct alloca_type_and_limit
>> +alloca_call_type_by_arg (tree arg, tree arg_casted, edge e, unsigned
>> max_size)
>> +{
>> + basic_block bb = e->src;
>> + gimple_stmt_iterator gsi = gsi_last_bb (bb);
>> + gimple *last = gsi_stmt (gsi);
>> + if (!last || gimple_code (last) != GIMPLE_COND)
>> + return alloca_type_and_limit (ALLOCA_UNBOUNDED);
>> +
>> +
>> + // Check for:
>> + // if (ARG .COND. N)
>> + // goto <bb 3>;
>> + // else
>> + // goto <bb 4>;
>> + // <bb 3>:
>> + // alloca(ARG);
>> + // Similarly, but check for a comparison with an unknown LIMIT.
>> + // if (LIMIT .COND. ARG)
>> + // alloca(arg);
>> + //
>> + // Where LIMIT has a bound of unknown range.
>> + //
>> + // Note: All conditions of the form (ARG .COND. XXXX) where covered
>> + // by the previous check above, so we only need to look for (LIMIT
>> + // .COND. ARG) here.
>> + tree limit = gimple_cond_lhs (last);
>> + if ((gimple_cond_rhs (last) == arg
>> + || gimple_cond_rhs (last) == arg_casted)
>> + && TREE_CODE (limit) == SSA_NAME)
>> + {
>> + wide_int min, max;
>> + value_range_type range_type = get_range_info (limit, &min, &max);
>> +
>> + if (range_type == VR_UNDEFINED || range_type == VR_VARYING)
>> + return alloca_type_and_limit (ALLOCA_BOUND_UNKNOWN);
>> +
>> + // ?? It looks like the above `if' is unnecessary, as we never
>> + // get any VR_RANGE or VR_ANTI_RANGE here. If we had a range
>> + // for LIMIT, I suppose we would have taken care of it in
>> + // alloca_call_type(), or handled above where we handle (ARG
>> .COND. N).
>> + //
>> + // If this ever triggers, figure out why and handle it, though
>> + // it is likely to be just an ALLOCA_UNBOUNDED.
>> + gcc_unreachable ();
> So this seems like a case of "we don't expect this, though it might in
> theory happen". I think degrading gracefully to ALLOCA_UNBOUNDED is a
> better choice for this kind of situation than gcc_unreachable. If we're
> generating a dump file, then perhaps saying something in the dump file
> about the unhandled case would be useful.
>
>
>
>> +
>> +// Analyze the alloca call in STMT and return the alloca type with its
>> +// corresponding limit (if applicable). IS_VLA is set if the alloca
>> +// call is really a BUILT_IN_ALLOCA_WITH_ALIGN, signifying a VLA.
>> +//
>> +// If the alloca call may be too large because of a cast from a signed
>> +// type to an unsigned type, set *INVALID_CASTED_TYPE to the
>> +// problematic signed type.
>> +
>> +static struct alloca_type_and_limit
>> +alloca_call_type (gimple *stmt, bool is_vla, tree *invalid_casted_type)
>> +{
>> + gcc_assert (gimple_alloca_call_p (stmt));
>> + tree len = gimple_call_arg (stmt, 0);
>> + tree len_casted = NULL;
>> + wide_int min, max;
>> + struct alloca_type_and_limit ret = alloca_type_and_limit
>> (ALLOCA_UNBOUNDED);
>> +
>> + gcc_assert (!is_vla || warn_vla_limit > 0);
>> + gcc_assert (is_vla || warn_alloca_limit > 0);
>> +
>> + // Adjust warn_alloca_max_size for VLAs, by taking the underlying
>> + // type into account.
>> + unsigned HOST_WIDE_INT max_size;
>> + if (is_vla)
>> + max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
>> + else
>> + max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
>> +
>> + // Check for the obviously bounded case.
>> + if (TREE_CODE (len) == INTEGER_CST)
>> + {
>> + if (tree_to_uhwi (len) > max_size)
>> + return alloca_type_and_limit (ALLOCA_BOUND_DEFINITELY_LARGE, len);
>> + if (integer_zerop (len))
>> + return alloca_type_and_limit (ALLOCA_ARG_IS_ZERO);
>> + ret = alloca_type_and_limit (ALLOCA_OK);
>> + }
>> + else if (TREE_CODE (len) != SSA_NAME)
>> + {
>> + // We only run with optimization and we have yet to trigger
>> + // anything but an SSA_NAME here. Fail here just in case we get
>> + // a non SSA_NAME here in the future, though I doubt it will
>> + // hold anything of value, since we need range information for
>> + // anything to make sense.
>> + gcc_unreachable ();
>> + return alloca_type_and_limit (ALLOCA_UNBOUNDED);
>> + }
> In the gimple world, the arguments to an alloca should only be constants
> and SSA_NAMES. My concern WRT _DECL nodes was only an issue if not
> optimizing. So I think this else clause can just go away.
>
> I think with the minor stuff noted above fixed, this is fine for the
> trunk. I don't think it needs an additional review cycle. Sorry for
> the long delay.
My apologies for the delay. I have finally committed this patch.
Before doing so, I merged with trunk and re-tested (x86-64 Linux) just
in case. There were no regressions.
Thanks for the review.
Aldy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-18 21:42 ` Aldy Hernandez
@ 2016-10-19 7:08 ` Eric Botcazou
2016-10-19 12:46 ` Aldy Hernandez
2016-10-19 10:45 ` Andreas Schwab
1 sibling, 1 reply; 26+ messages in thread
From: Eric Botcazou @ 2016-10-19 7:08 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches, Jeff Law
> My apologies for the delay. I have finally committed this patch.
> Before doing so, I merged with trunk and re-tested (x86-64 Linux) just
> in case. There were no regressions.
Can you rename the testcases? Git doesn't like the names:
*** These collisions happen when the name of two or more files
*** differ in casing only (Eg: "hello.txt" and "Hello.txt").
*** Please re-do your commit, chosing names that do not collide.
***
*** Commit: 09b6f37fdbc6b8955a13bce336f4679bb0da78d7
*** Subject: Manual merge of 'origin/fsf-gcc-head-branch' into gcc-head
(no-tn-check).
***
*** The matching files are:
***
*** gcc/testsuite/gcc.dg/Wvla-2.c
*** gcc/testsuite/gcc.dg/wvla-2.c
***
*** gcc/testsuite/gcc.dg/Wvla-1.c
*** gcc/testsuite/gcc.dg/wvla-1.c
***
*** gcc/testsuite/gcc.dg/Wvla-3.c
*** gcc/testsuite/gcc.dg/wvla-3.c)
--
Eric Botcazou
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-18 21:42 ` Aldy Hernandez
2016-10-19 7:08 ` Eric Botcazou
@ 2016-10-19 10:45 ` Andreas Schwab
2016-10-19 10:50 ` Christophe Lyon
2016-10-19 12:54 ` Aldy Hernandez
1 sibling, 2 replies; 26+ messages in thread
From: Andreas Schwab @ 2016-10-19 10:45 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Jeff Law, gcc-patches
FAIL: gcc.dg/Walloca-1.c (test for warnings, line 26)
FAIL: gcc.dg/Walloca-1.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-1.c:26:5: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
FAIL: gcc.dg/Walloca-2.c (test for warnings, line 34)
FAIL: gcc.dg/Walloca-2.c note (test for warnings, line 34)
FAIL: gcc.dg/Walloca-2.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:11:7: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
/daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:34:9: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 10:45 ` Andreas Schwab
@ 2016-10-19 10:50 ` Christophe Lyon
2016-10-19 12:54 ` Aldy Hernandez
1 sibling, 0 replies; 26+ messages in thread
From: Christophe Lyon @ 2016-10-19 10:50 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Aldy Hernandez, Jeff Law, gcc-patches
On 19 October 2016 at 12:45, Andreas Schwab <schwab@suse.de> wrote:
> FAIL: gcc.dg/Walloca-1.c (test for warnings, line 26)
> FAIL: gcc.dg/Walloca-1.c (test for excess errors)
> Excess errors:
> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-1.c:26:5: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
> FAIL: gcc.dg/Walloca-2.c (test for warnings, line 34)
> FAIL: gcc.dg/Walloca-2.c note (test for warnings, line 34)
> FAIL: gcc.dg/Walloca-2.c (test for excess errors)
> Excess errors:
> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:11:7: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:34:9: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
>
I'm seeing the same errors on arm* targets.
Christophe
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 7:08 ` Eric Botcazou
@ 2016-10-19 12:46 ` Aldy Hernandez
2016-10-19 13:24 ` Eric Botcazou
0 siblings, 1 reply; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-19 12:46 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Jeff Law
On 10/19/2016 03:08 AM, Eric Botcazou wrote:
>> My apologies for the delay. I have finally committed this patch.
>> Before doing so, I merged with trunk and re-tested (x86-64 Linux) just
>> in case. There were no regressions.
>
> Can you rename the testcases? Git doesn't like the names:
>
> *** These collisions happen when the name of two or more files
> *** differ in casing only (Eg: "hello.txt" and "Hello.txt").
> *** Please re-do your commit, chosing names that do not collide.
> ***
> *** Commit: 09b6f37fdbc6b8955a13bce336f4679bb0da78d7
> *** Subject: Manual merge of 'origin/fsf-gcc-head-branch' into gcc-head
> (no-tn-check).
> ***
> *** The matching files are:
> ***
> *** gcc/testsuite/gcc.dg/Wvla-2.c
> *** gcc/testsuite/gcc.dg/wvla-2.c
> ***
> *** gcc/testsuite/gcc.dg/Wvla-1.c
> *** gcc/testsuite/gcc.dg/wvla-1.c
> ***
> *** gcc/testsuite/gcc.dg/Wvla-3.c
> *** gcc/testsuite/gcc.dg/wvla-3.c)
>
It seems to me that there is a precedence for -W type tests to start
with a capital letter, as opposed to a lowercase letter:
$ cd gcc.dg
$ ls W[a-z]* |wc
280 280 5816
Do you think it would perhaps be reasonable to rename all of them to
uppercase? You could take -Wvla-[1-7].c and I can take the subsequent ones?
Aldy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 10:45 ` Andreas Schwab
2016-10-19 10:50 ` Christophe Lyon
@ 2016-10-19 12:54 ` Aldy Hernandez
2016-10-19 13:03 ` Andreas Schwab
1 sibling, 1 reply; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-19 12:54 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Jeff Law, gcc-patches
On 10/19/2016 06:45 AM, Andreas Schwab wrote:
> FAIL: gcc.dg/Walloca-1.c (test for warnings, line 26)
> FAIL: gcc.dg/Walloca-1.c (test for excess errors)
> Excess errors:
> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-1.c:26:5: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
> FAIL: gcc.dg/Walloca-2.c (test for warnings, line 34)
> FAIL: gcc.dg/Walloca-2.c note (test for warnings, line 34)
> FAIL: gcc.dg/Walloca-2.c (test for excess errors)
> Excess errors:
> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:11:7: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:34:9: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
>
> Andreas.
>
Is this for Arm as Christophe reported, or this on another target?
triplet?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 12:54 ` Aldy Hernandez
@ 2016-10-19 13:03 ` Andreas Schwab
2016-10-19 13:16 ` Eric Botcazou
0 siblings, 1 reply; 26+ messages in thread
From: Andreas Schwab @ 2016-10-19 13:03 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Jeff Law, gcc-patches
On Okt 19 2016, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 10/19/2016 06:45 AM, Andreas Schwab wrote:
>> FAIL: gcc.dg/Walloca-1.c (test for warnings, line 26)
>> FAIL: gcc.dg/Walloca-1.c (test for excess errors)
>> Excess errors:
>> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-1.c:26:5: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
>> FAIL: gcc.dg/Walloca-2.c (test for warnings, line 34)
>> FAIL: gcc.dg/Walloca-2.c note (test for warnings, line 34)
>> FAIL: gcc.dg/Walloca-2.c (test for excess errors)
>> Excess errors:
>> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:11:7: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
>> /daten/aranym/gcc/gcc-20161019/gcc/testsuite/gcc.dg/Walloca-2.c:34:9: warning: unbounded use of 'alloca' [-Walloca-larger-than=]
>>
>> Andreas.
>>
>
> Is this for Arm as Christophe reported, or this on another target?
m68k-suse-linux
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 13:03 ` Andreas Schwab
@ 2016-10-19 13:16 ` Eric Botcazou
2016-10-19 15:32 ` Aldy Hernandez
0 siblings, 1 reply; 26+ messages in thread
From: Eric Botcazou @ 2016-10-19 13:16 UTC (permalink / raw)
To: Andreas Schwab; +Cc: gcc-patches, Aldy Hernandez, Jeff Law
> m68k-suse-linux
visium-elf too.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 12:46 ` Aldy Hernandez
@ 2016-10-19 13:24 ` Eric Botcazou
2016-10-19 13:27 ` Aldy Hernandez
2016-10-19 13:54 ` Aldy Hernandez
0 siblings, 2 replies; 26+ messages in thread
From: Eric Botcazou @ 2016-10-19 13:24 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches, Jeff Law
> It seems to me that there is a precedence for -W type tests to start
> with a capital letter, as opposed to a lowercase letter:
>
> $ cd gcc.dg
> $ ls W[a-z]* |wc
> 280 280 5816
>
> Do you think it would perhaps be reasonable to rename all of them to
> uppercase?
Yes, I agree that this would be more consistent.
> You could take -Wvla-[1-7].c and I can take the subsequent ones?
Not sure if this wouldn't make us tread on each other's toe. ;-) I'd just
rename your 3 new testcases (-Wvla-larger-than-1.c, Wvla-larger-than-2.c and
Walloca-11.c probably) and promote the existing wvla-[1-7].c to uppercase.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 13:24 ` Eric Botcazou
@ 2016-10-19 13:27 ` Aldy Hernandez
2016-10-19 13:42 ` Eric Botcazou
2016-10-19 13:54 ` Aldy Hernandez
1 sibling, 1 reply; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-19 13:27 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Jeff Law
On 10/19/2016 09:24 AM, Eric Botcazou wrote:
>> It seems to me that there is a precedence for -W type tests to start
>> with a capital letter, as opposed to a lowercase letter:
>>
>> $ cd gcc.dg
>> $ ls W[a-z]* |wc
>> 280 280 5816
>>
>> Do you think it would perhaps be reasonable to rename all of them to
>> uppercase?
>
> Yes, I agree that this would be more consistent.
>
>> You could take -Wvla-[1-7].c and I can take the subsequent ones?
>
> Not sure if this wouldn't make us tread on each other's toe. ;-) I'd just
> rename your 3 new testcases (-Wvla-larger-than-1.c, Wvla-larger-than-2.c and
> Walloca-11.c probably) and promote the existing wvla-[1-7].c to uppercase.
>
It's unclear from your response whether you want to do this or want me
to do it. In the interest of being lazy, I'll let you do it :).
Aldy
p.s. If OTOH, you want me to do it, let me know.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 13:27 ` Aldy Hernandez
@ 2016-10-19 13:42 ` Eric Botcazou
0 siblings, 0 replies; 26+ messages in thread
From: Eric Botcazou @ 2016-10-19 13:42 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches, Jeff Law
> It's unclear from your response whether you want to do this or want me
> to do it. In the interest of being lazy, I'll let you do it :).
I guess this wasn't really decided in my mind either. ;-)
> p.s. If OTOH, you want me to do it, let me know.
Yes, please do, thanks in advance.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 13:24 ` Eric Botcazou
2016-10-19 13:27 ` Aldy Hernandez
@ 2016-10-19 13:54 ` Aldy Hernandez
2016-10-19 15:32 ` Eric Botcazou
1 sibling, 1 reply; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-19 13:54 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Jeff Law
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
On 10/19/2016 09:24 AM, Eric Botcazou wrote:
>> It seems to me that there is a precedence for -W type tests to start
>> with a capital letter, as opposed to a lowercase letter:
>>
>> $ cd gcc.dg
>> $ ls W[a-z]* |wc
>> 280 280 5816
>>
>> Do you think it would perhaps be reasonable to rename all of them to
>> uppercase?
>
> Yes, I agree that this would be more consistent.
>
>> You could take -Wvla-[1-7].c and I can take the subsequent ones?
>
> Not sure if this wouldn't make us tread on each other's toe. ;-) I'd just
> rename your 3 new testcases (-Wvla-larger-than-1.c, Wvla-larger-than-2.c and
> Walloca-11.c probably) and promote the existing wvla-[1-7].c to uppercase.
>
Done.
I have committed the attached patch as mostly obvious :).
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 10852 bytes --]
commit e066ee667cec9a90478deff7b3090587ae11236f
Author: aldyh <aldyh@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Wed Oct 19 13:52:43 2016 +0000
* gcc.dg/Wvla-1.c: Rename to...
* gcc.dg/Wvla-larger-than-1.c: ...this.
* gcc.dg/Wvla-2.c: Rename to...
* gcc.dg/Wvla-larger-than-2.c: ...this.
* gcc.dg/Wvla-3.c: Rename to...
* gcc.dg/Walloca-11.c.: ...this.
* gcc.dg/wvla-[1-7].c: Rename to:
* gcc.dg/Wvla-[1-7].c: ...this.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@241344 138bc75d-0d04-0410-961f-82ee72b054a4
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index a270700..d3d269d 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,14 @@
+2016-10-19 Aldy Hernandez <aldyh@redhat.com>
+
+ * gcc.dg/Wvla-1.c: Rename to...
+ * gcc.dg/Wvla-larger-than-1.c: ...this.
+ * gcc.dg/Wvla-2.c: Rename to...
+ * gcc.dg/Wvla-larger-than-2.c: ...this.
+ * gcc.dg/Wvla-3.c: Rename to...
+ * gcc.dg/Walloca-11.c.: ...this.
+ * gcc.dg/wvla-[1-7].c: Rename to:
+ * gcc.dg/Wvla-[1-7].c: ...this.
+
2016-10-19 Bin Cheng <bin.cheng@arm.com>
PR tree-optimization/78005
diff --git a/gcc/testsuite/gcc.dg/Walloca-11.c b/gcc/testsuite/gcc.dg/Walloca-11.c
new file mode 100644
index 0000000..5124476
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloca-11.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-Walloca -O2" } */
+
+// Make sure we don't warn on VLA with -Walloca.
+
+void f (void*);
+
+void h1 (unsigned n)
+{
+ int a [n];
+ f (a);
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-1.c b/gcc/testsuite/gcc.dg/Wvla-1.c
index 384c930..d2e3cb5 100644
--- a/gcc/testsuite/gcc.dg/Wvla-1.c
+++ b/gcc/testsuite/gcc.dg/Wvla-1.c
@@ -1,24 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-Wvla-larger-than=100 -O2" } */
+/* { dg-options "-std=c89 -Wvla" } */
-typedef __SIZE_TYPE__ size_t;
-
-extern void useit (char *);
-
-int num;
-
-void test_vlas (size_t num)
-{
- char str2[num]; /* { dg-warning "unbounded use" } */
- useit(str2);
-
- num = 98;
- for (int i=0; i < 1234; ++i) {
- char str[num]; // OK, VLA in a loop, but it is a
- // known size *AND* the compiler takes
- // care of cleaning up between
- // iterations with
- // __builtin_stack_restore.
- useit(str);
- }
-}
+extern void
+func (int i, int array[i]); /* { dg-warning "ISO C90 forbids variable length array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/Wvla-2.c b/gcc/testsuite/gcc.dg/Wvla-2.c
index 96814dc..92c67ed 100644
--- a/gcc/testsuite/gcc.dg/Wvla-2.c
+++ b/gcc/testsuite/gcc.dg/Wvla-2.c
@@ -1,70 +1,5 @@
/* { dg-do compile } */
-/* { dg-require-effective-target stdint_types } */
-/* { dg-options "-O2 -Wvla-larger-than=40" } */
+/* { dg-options "-std=c99 -Wvla" } */
-#include <stdint.h>
-
-void f0 (void *);
-void
-f1 (__SIZE_TYPE__ a)
-{
- if (a <= 10)
- {
- // 10 * 4 bytes = 40: OK!
- uint32_t x[a];
- f0 (x);
- }
-}
-
-void
-f2 (__SIZE_TYPE__ a)
-{
- if (a <= 11)
- {
- // 11 * 4 bytes = 44: Not OK.
- uint32_t x[a]; // { dg-warning "array may be too large" }
- // { dg-message "note:.*argument may be as large as 44" "note" { target *-*-* } 25 }
- f0 (x);
- }
-}
-
-void
-f3 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
-{
- if (a <= 5 && b <= 3)
- {
- // 5 * 3 * 4 bytes = 60: Not OK.
- uint32_t x[a][b]; // { dg-warning "array may be too large" }
- f0 (x);
- }
-}
-
-void
-f4 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
-{
- if (a <= 5 && b <= 2)
- {
- // 5 * 2 * 4 bytes = 40 bytes: OK!
- uint32_t x[a][b];
- f0 (x);
- }
-}
-
-void
-f5 (__SIZE_TYPE__ len)
-{
- // Test that a direct call to __builtin_alloca_with_align is not
- // confused with a VLA.
- void *p = __builtin_alloca_with_align (len, 8);
- f0 (p);
-}
-
-void
-f6 (unsigned stuff)
-{
- int n = 7000;
- do {
- char a[n]; // { dg-warning "variable-length array is too large" }
- f0 (a);
- } while (stuff--);
-}
+extern void
+func (int i, int array[i]); /* { dg-warning "ISO C90 forbids variable length array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/Wvla-3.c b/gcc/testsuite/gcc.dg/Wvla-3.c
index 5124476..45132fa 100644
--- a/gcc/testsuite/gcc.dg/Wvla-3.c
+++ b/gcc/testsuite/gcc.dg/Wvla-3.c
@@ -1,12 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-Walloca -O2" } */
+/* { dg-options "-pedantic-errors -std=c89 -Wvla" } */
-// Make sure we don't warn on VLA with -Walloca.
-
-void f (void*);
-
-void h1 (unsigned n)
-{
- int a [n];
- f (a);
-}
+extern void
+func (int i, int array[i]); /* { dg-error "ISO C90 forbids variable.* array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/Wvla-4.c b/gcc/testsuite/gcc.dg/Wvla-4.c
new file mode 100644
index 0000000..ae2e0b0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-4.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-pedantic-errors -std=c99 -Wvla" } */
+
+extern void
+func (int i, int array[i]); /* { dg-warning "ISO C90 forbids variable length array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/Wvla-5.c b/gcc/testsuite/gcc.dg/Wvla-5.c
new file mode 100644
index 0000000..919b8dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-5.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-pedantic-errors -std=c89 -Wno-vla" } */
+
+extern void
+func (int i, int array[i]);
diff --git a/gcc/testsuite/gcc.dg/Wvla-6.c b/gcc/testsuite/gcc.dg/Wvla-6.c
new file mode 100644
index 0000000..694a4cc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-6.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c89 -Wvla" } */
+
+extern void
+func (int i, int [i]); /* { dg-warning "ISO C90 forbids variable length array" } */
diff --git a/gcc/testsuite/gcc.dg/Wvla-7.c b/gcc/testsuite/gcc.dg/Wvla-7.c
new file mode 100644
index 0000000..4c264f0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-7.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-pedantic-errors -std=c89 -Wvla" } */
+
+extern void
+func (int i, int [i]); /* { dg-error "ISO C90 forbids variable" } */
diff --git a/gcc/testsuite/gcc.dg/Wvla-larger-than-1.c b/gcc/testsuite/gcc.dg/Wvla-larger-than-1.c
new file mode 100644
index 0000000..384c930
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-larger-than-1.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-Wvla-larger-than=100 -O2" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void useit (char *);
+
+int num;
+
+void test_vlas (size_t num)
+{
+ char str2[num]; /* { dg-warning "unbounded use" } */
+ useit(str2);
+
+ num = 98;
+ for (int i=0; i < 1234; ++i) {
+ char str[num]; // OK, VLA in a loop, but it is a
+ // known size *AND* the compiler takes
+ // care of cleaning up between
+ // iterations with
+ // __builtin_stack_restore.
+ useit(str);
+ }
+}
diff --git a/gcc/testsuite/gcc.dg/Wvla-larger-than-2.c b/gcc/testsuite/gcc.dg/Wvla-larger-than-2.c
new file mode 100644
index 0000000..96814dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-larger-than-2.c
@@ -0,0 +1,70 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target stdint_types } */
+/* { dg-options "-O2 -Wvla-larger-than=40" } */
+
+#include <stdint.h>
+
+void f0 (void *);
+void
+f1 (__SIZE_TYPE__ a)
+{
+ if (a <= 10)
+ {
+ // 10 * 4 bytes = 40: OK!
+ uint32_t x[a];
+ f0 (x);
+ }
+}
+
+void
+f2 (__SIZE_TYPE__ a)
+{
+ if (a <= 11)
+ {
+ // 11 * 4 bytes = 44: Not OK.
+ uint32_t x[a]; // { dg-warning "array may be too large" }
+ // { dg-message "note:.*argument may be as large as 44" "note" { target *-*-* } 25 }
+ f0 (x);
+ }
+}
+
+void
+f3 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
+{
+ if (a <= 5 && b <= 3)
+ {
+ // 5 * 3 * 4 bytes = 60: Not OK.
+ uint32_t x[a][b]; // { dg-warning "array may be too large" }
+ f0 (x);
+ }
+}
+
+void
+f4 (__SIZE_TYPE__ a, __SIZE_TYPE__ b)
+{
+ if (a <= 5 && b <= 2)
+ {
+ // 5 * 2 * 4 bytes = 40 bytes: OK!
+ uint32_t x[a][b];
+ f0 (x);
+ }
+}
+
+void
+f5 (__SIZE_TYPE__ len)
+{
+ // Test that a direct call to __builtin_alloca_with_align is not
+ // confused with a VLA.
+ void *p = __builtin_alloca_with_align (len, 8);
+ f0 (p);
+}
+
+void
+f6 (unsigned stuff)
+{
+ int n = 7000;
+ do {
+ char a[n]; // { dg-warning "variable-length array is too large" }
+ f0 (a);
+ } while (stuff--);
+}
diff --git a/gcc/testsuite/gcc.dg/wvla-1.c b/gcc/testsuite/gcc.dg/wvla-1.c
deleted file mode 100644
index d2e3cb5..0000000
--- a/gcc/testsuite/gcc.dg/wvla-1.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-std=c89 -Wvla" } */
-
-extern void
-func (int i, int array[i]); /* { dg-warning "ISO C90 forbids variable length array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/wvla-2.c b/gcc/testsuite/gcc.dg/wvla-2.c
deleted file mode 100644
index 92c67ed..0000000
--- a/gcc/testsuite/gcc.dg/wvla-2.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-std=c99 -Wvla" } */
-
-extern void
-func (int i, int array[i]); /* { dg-warning "ISO C90 forbids variable length array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/wvla-3.c b/gcc/testsuite/gcc.dg/wvla-3.c
deleted file mode 100644
index 45132fa..0000000
--- a/gcc/testsuite/gcc.dg/wvla-3.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-pedantic-errors -std=c89 -Wvla" } */
-
-extern void
-func (int i, int array[i]); /* { dg-error "ISO C90 forbids variable.* array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/wvla-4.c b/gcc/testsuite/gcc.dg/wvla-4.c
deleted file mode 100644
index ae2e0b0..0000000
--- a/gcc/testsuite/gcc.dg/wvla-4.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-pedantic-errors -std=c99 -Wvla" } */
-
-extern void
-func (int i, int array[i]); /* { dg-warning "ISO C90 forbids variable length array 'array'" } */
diff --git a/gcc/testsuite/gcc.dg/wvla-5.c b/gcc/testsuite/gcc.dg/wvla-5.c
deleted file mode 100644
index 919b8dc..0000000
--- a/gcc/testsuite/gcc.dg/wvla-5.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-pedantic-errors -std=c89 -Wno-vla" } */
-
-extern void
-func (int i, int array[i]);
diff --git a/gcc/testsuite/gcc.dg/wvla-6.c b/gcc/testsuite/gcc.dg/wvla-6.c
deleted file mode 100644
index 694a4cc..0000000
--- a/gcc/testsuite/gcc.dg/wvla-6.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-std=c89 -Wvla" } */
-
-extern void
-func (int i, int [i]); /* { dg-warning "ISO C90 forbids variable length array" } */
diff --git a/gcc/testsuite/gcc.dg/wvla-7.c b/gcc/testsuite/gcc.dg/wvla-7.c
deleted file mode 100644
index 4c264f0..0000000
--- a/gcc/testsuite/gcc.dg/wvla-7.c
+++ /dev/null
@@ -1,5 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-pedantic-errors -std=c89 -Wvla" } */
-
-extern void
-func (int i, int [i]); /* { dg-error "ISO C90 forbids variable" } */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 13:54 ` Aldy Hernandez
@ 2016-10-19 15:32 ` Eric Botcazou
0 siblings, 0 replies; 26+ messages in thread
From: Eric Botcazou @ 2016-10-19 15:32 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches, Jeff Law
> I have committed the attached patch as mostly obvious :).
Thanks!
--
Eric Botcazou
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 13:16 ` Eric Botcazou
@ 2016-10-19 15:32 ` Aldy Hernandez
2016-10-19 15:38 ` Andreas Schwab
2016-10-19 15:55 ` Jeff Law
0 siblings, 2 replies; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-19 15:32 UTC (permalink / raw)
To: Eric Botcazou, Andreas Schwab; +Cc: gcc-patches, Jeff Law, christophe.lyon
[-- Attachment #1: Type: text/plain, Size: 903 bytes --]
On 10/19/2016 09:16 AM, Eric Botcazou wrote:
>> m68k-suse-linux
>
> visium-elf too.
>
The attached patch fixes the failures on m68k-suse-linux, visium-elf,
and arm-eabi.
There were a few problems.
One problem is that on lp64 targets (where sizeof(size_t) !=
sizeof(int)), the warning is slightly different-- and rightly so. I
have updated the test to handle both warnings on the respective targets.
The other problem is that the following snippet is incorrectly warning
on 32-bit targets:
if (n > 0 && n < 2000)
p = __builtin_alloca (n);
Looking at the gimple it seems like another case of VRP failing to give
any range information whatsoever. I have xfailed it as another case
where Andrew's upcoming work should theoretically fix this. The test is
fine on 64-bit targets.
Can y'all double check it on your respective targets as I only have a
crude cross build?
Thanks.
Aldy
[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 2103 bytes --]
commit 32b629dcab7b78e8181146338c4b077f1d55a0bf
Author: Aldy Hernandez <aldyh@redhat.com>
Date: Wed Oct 19 11:24:44 2016 -0400
* gcc.dg/Walloca-1.c: Adjust test for !lp64 targets.
* gcc.dg/Walloca-2.c: Same.
diff --git a/gcc/testsuite/gcc.dg/Walloca-1.c b/gcc/testsuite/gcc.dg/Walloca-1.c
index 34a20c3..32e4ab8 100644
--- a/gcc/testsuite/gcc.dg/Walloca-1.c
+++ b/gcc/testsuite/gcc.dg/Walloca-1.c
@@ -23,7 +23,8 @@ void foo1 (size_t len, size_t len2, size_t len3)
char *s = alloca (123);
useit (s); // OK, constant argument to alloca
- s = alloca (num); // { dg-warning "large due to conversion" }
+ s = alloca (num); // { dg-warning "large due to conversion" "" { target lp64 } }
+ // { dg-warning "unbounded use of 'alloca'" "" { target { ! lp64 } } 26 }
useit (s);
s = alloca(90000); /* { dg-warning "is too large" } */
diff --git a/gcc/testsuite/gcc.dg/Walloca-2.c b/gcc/testsuite/gcc.dg/Walloca-2.c
index 284b34e..4695fda 100644
--- a/gcc/testsuite/gcc.dg/Walloca-2.c
+++ b/gcc/testsuite/gcc.dg/Walloca-2.c
@@ -8,7 +8,11 @@ g1 (int n)
{
void *p;
if (n > 0 && n < 2000)
- p = __builtin_alloca (n);
+ // FIXME: This is a bogus warning, and is currently happening on
+ // 32-bit targets because VRP is not giving us any range info for
+ // the argument to __builtin_alloca. This should be fixed by the
+ // upcoming range work.
+ p = __builtin_alloca (n); // { dg-bogus "unbounded use of 'alloca'" "" { xfail { ! lp64 } } }
else
p = __builtin_malloc (n);
f (p);
@@ -31,8 +35,9 @@ g3 (int n)
void *p;
if (n > 0 && n < 3000)
{
- p = __builtin_alloca (n); // { dg-warning "'alloca' may be too large" }
- // { dg-message "note:.*argument may be as large as 2999" "note" { target *-*-* } 34 }
+ p = __builtin_alloca (n); // { dg-warning "'alloca' may be too large" "" { target lp64} }
+ // { dg-message "note:.*argument may be as large as 2999" "note" { target lp64 } 38 }
+ // { dg-warning "unbounded use of 'alloca'" "" { target { ! lp64 } } 38 }
}
else
p = __builtin_malloc (n);
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 15:32 ` Aldy Hernandez
@ 2016-10-19 15:38 ` Andreas Schwab
2016-10-19 15:42 ` Eric Botcazou
2016-10-19 15:55 ` Jeff Law
1 sibling, 1 reply; 26+ messages in thread
From: Andreas Schwab @ 2016-10-19 15:38 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Eric Botcazou, gcc-patches, Jeff Law, christophe.lyon
On Okt 19 2016, Aldy Hernandez <aldyh@redhat.com> wrote:
> commit 32b629dcab7b78e8181146338c4b077f1d55a0bf
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date: Wed Oct 19 11:24:44 2016 -0400
>
> * gcc.dg/Walloca-1.c: Adjust test for !lp64 targets.
> * gcc.dg/Walloca-2.c: Same.
This fixes both tests on m68k.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 15:38 ` Andreas Schwab
@ 2016-10-19 15:42 ` Eric Botcazou
0 siblings, 0 replies; 26+ messages in thread
From: Eric Botcazou @ 2016-10-19 15:42 UTC (permalink / raw)
To: Andreas Schwab; +Cc: gcc-patches, Aldy Hernandez, Jeff Law, christophe.lyon
> This fixes both tests on m68k.
Likewise on Visium.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 15:32 ` Aldy Hernandez
2016-10-19 15:38 ` Andreas Schwab
@ 2016-10-19 15:55 ` Jeff Law
2016-10-19 16:22 ` Christophe Lyon
1 sibling, 1 reply; 26+ messages in thread
From: Jeff Law @ 2016-10-19 15:55 UTC (permalink / raw)
To: Aldy Hernandez, Eric Botcazou, Andreas Schwab
Cc: gcc-patches, christophe.lyon
On 10/19/2016 09:32 AM, Aldy Hernandez wrote:
> On 10/19/2016 09:16 AM, Eric Botcazou wrote:
>>> m68k-suse-linux
>>
>> visium-elf too.
>>
>
> The attached patch fixes the failures on m68k-suse-linux, visium-elf,
> and arm-eabi.
>
> There were a few problems.
>
> One problem is that on lp64 targets (where sizeof(size_t) !=
> sizeof(int)), the warning is slightly different-- and rightly so. I
> have updated the test to handle both warnings on the respective targets.
>
> The other problem is that the following snippet is incorrectly warning
> on 32-bit targets:
>
> if (n > 0 && n < 2000)
> p = __builtin_alloca (n);
>
> Looking at the gimple it seems like another case of VRP failing to give
> any range information whatsoever. I have xfailed it as another case
> where Andrew's upcoming work should theoretically fix this. The test is
> fine on 64-bit targets.
>
> Can y'all double check it on your respective targets as I only have a
> crude cross build?
OK for the trunk whenever you're ready.
jeff
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 15:55 ` Jeff Law
@ 2016-10-19 16:22 ` Christophe Lyon
2016-10-19 18:43 ` Aldy Hernandez
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Lyon @ 2016-10-19 16:22 UTC (permalink / raw)
To: Jeff Law; +Cc: Aldy Hernandez, Eric Botcazou, Andreas Schwab, gcc-patches
On 19 October 2016 at 17:55, Jeff Law <law@redhat.com> wrote:
> On 10/19/2016 09:32 AM, Aldy Hernandez wrote:
>>
>> On 10/19/2016 09:16 AM, Eric Botcazou wrote:
>>>>
>>>> m68k-suse-linux
>>>
>>>
>>> visium-elf too.
>>>
>>
>> The attached patch fixes the failures on m68k-suse-linux, visium-elf,
>> and arm-eabi.
>>
>> There were a few problems.
>>
>> One problem is that on lp64 targets (where sizeof(size_t) !=
>> sizeof(int)), the warning is slightly different-- and rightly so. I
>> have updated the test to handle both warnings on the respective targets.
>>
>> The other problem is that the following snippet is incorrectly warning
>> on 32-bit targets:
>>
>> if (n > 0 && n < 2000)
>> p = __builtin_alloca (n);
>>
>> Looking at the gimple it seems like another case of VRP failing to give
>> any range information whatsoever. I have xfailed it as another case
>> where Andrew's upcoming work should theoretically fix this. The test is
>> fine on 64-bit targets.
>>
>> Can y'all double check it on your respective targets as I only have a
>> crude cross build?
>
> OK for the trunk whenever you're ready.
>
> jeff
You are too fast for me :-)
I do not have the build trees for all the configurations ready for
manual testing...
So, I just merged the initial patch and the 2nd one, and started
a validation job to make sure the 2nd patch fixes all the regressions
observed earlier.
It will take a few hours.
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 16:22 ` Christophe Lyon
@ 2016-10-19 18:43 ` Aldy Hernandez
2016-10-19 20:16 ` Christophe Lyon
0 siblings, 1 reply; 26+ messages in thread
From: Aldy Hernandez @ 2016-10-19 18:43 UTC (permalink / raw)
To: Christophe Lyon, Jeff Law; +Cc: Eric Botcazou, Andreas Schwab, gcc-patches
>> OK for the trunk whenever you're ready.
>>
>> jeff
>
> You are too fast for me :-)
> I do not have the build trees for all the configurations ready for
> manual testing...
> So, I just merged the initial patch and the 2nd one, and started
> a validation job to make sure the 2nd patch fixes all the regressions
> observed earlier.
> It will take a few hours.
There shouldn't be any problems that didn't surface in my cross build,
since they are not execution tests, but compile tests.
I have committed the patch. If there any specific failures specific to
your target, let me know and I'll address those separately.
Thanks.
Aldy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: PING: new pass to warn on questionable uses of alloca() and VLAs
2016-10-19 18:43 ` Aldy Hernandez
@ 2016-10-19 20:16 ` Christophe Lyon
0 siblings, 0 replies; 26+ messages in thread
From: Christophe Lyon @ 2016-10-19 20:16 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Jeff Law, Eric Botcazou, Andreas Schwab, gcc-patches
On 19 October 2016 at 20:43, Aldy Hernandez <aldyh@redhat.com> wrote:
>
>>> OK for the trunk whenever you're ready.
>>>
>>> jeff
>>
>>
>> You are too fast for me :-)
>> I do not have the build trees for all the configurations ready for
>> manual testing...
>> So, I just merged the initial patch and the 2nd one, and started
>> a validation job to make sure the 2nd patch fixes all the regressions
>> observed earlier.
>> It will take a few hours.
>
>
> There shouldn't be any problems that didn't surface in my cross build, since
> they are not execution tests, but compile tests.
>
> I have committed the patch. If there any specific failures specific to your
> target, let me know and I'll address those separately.
>
Validation results of the 2 patches combined are OK.
Thanks
> Thanks.
> Aldy
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2016-10-19 20:16 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-27 9:01 PING: new pass to warn on questionable uses of alloca() and VLAs Aldy Hernandez
2016-07-27 9:26 ` Rainer Orth
2016-08-04 16:38 ` Jeff Law
[not found] ` <57B6D2F1.6050709@redhat.com>
2016-08-19 14:57 ` Aldy Hernandez
2016-08-19 14:59 ` Aldy Hernandez
2016-09-13 20:30 ` Jeff Law
2016-10-18 21:42 ` Aldy Hernandez
2016-10-19 7:08 ` Eric Botcazou
2016-10-19 12:46 ` Aldy Hernandez
2016-10-19 13:24 ` Eric Botcazou
2016-10-19 13:27 ` Aldy Hernandez
2016-10-19 13:42 ` Eric Botcazou
2016-10-19 13:54 ` Aldy Hernandez
2016-10-19 15:32 ` Eric Botcazou
2016-10-19 10:45 ` Andreas Schwab
2016-10-19 10:50 ` Christophe Lyon
2016-10-19 12:54 ` Aldy Hernandez
2016-10-19 13:03 ` Andreas Schwab
2016-10-19 13:16 ` Eric Botcazou
2016-10-19 15:32 ` Aldy Hernandez
2016-10-19 15:38 ` Andreas Schwab
2016-10-19 15:42 ` Eric Botcazou
2016-10-19 15:55 ` Jeff Law
2016-10-19 16:22 ` Christophe Lyon
2016-10-19 18:43 ` Aldy Hernandez
2016-10-19 20:16 ` Christophe Lyon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).