Warn when returning the address of a temporary (middle-end) v2
Marc Glisse
marc.glisse@inria.fr
Wed Jul 30 12:00:00 GMT 2014
On Tue, 29 Jul 2014, David Malcolm wrote:
> On Tue, 2014-07-29 at 19:36 +0200, Marc Glisse wrote:
>> On Sun, 27 Jul 2014, Richard Sandiford wrote:
>>
>>> Marc Glisse <marc.glisse@inria.fr> writes:
>>>> Hello,
>>>>
>>>> I followed the advice in this discussion:
>>>> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
>>>>
>>>> and here is a new patch. I made an effort to isolate a path in at least
>>>> one subcase so it doesn't look too strange that the warning is in this
>>>> file. Computing the dominance info just to tweak the warning message may
>>>> be a bit excessive.
>>>
>>> How about only calculating it once you've decided to issue a message?
>>>
>>>> + if (always_executed)
>>>> + msg = "function returns address of local variable";
>>>> + else
>>>> + msg = "function may return address of local variable";
>>>
>>> I think you need _(...) here, unless some magic makes that unnecessary now.
>>
>> Current version, which passed bootstrap+testsuite on x86_64-linux-gnu.
>> (the original discussion is at
>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )
>
> This is possibly a dumb question, but what happens for a static local,
> rather than an auto local? e.g.
>
> int *f (void)
> {
> static int i;
> return &i;
> }
>
> (e.g. should the test case cover this?)
New version with a static local variable in the testcase, and without
using build_compound_expr in the C front-end to avoid extra warnings.
Same ChangeLog, same testsuite results.
--
Marc Glisse
-------------- next part --------------
Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c (revision 213251)
+++ gcc/c/c-typeck.c (working copy)
@@ -9330,22 +9330,26 @@ c_finish_return (location_t loc, tree re
if (DECL_P (inner)
&& !DECL_EXTERNAL (inner)
&& !TREE_STATIC (inner)
&& DECL_CONTEXT (inner) == current_function_decl)
{
if (TREE_CODE (inner) == LABEL_DECL)
warning_at (loc, OPT_Wreturn_local_addr,
"function returns address of label");
else
- warning_at (loc, OPT_Wreturn_local_addr,
- "function returns address of local variable");
+ {
+ warning_at (loc, OPT_Wreturn_local_addr,
+ "function returns address of local variable");
+ tree zero = build_zero_cst (TREE_TYPE (res));
+ t = build2 (COMPOUND_EXPR, TREE_TYPE (res), t, zero);
+ }
}
break;
default:
break;
}
break;
}
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt (revision 213251)
+++ gcc/c-family/c.opt (working copy)
@@ -698,24 +698,20 @@ ObjC ObjC++ Var(warn_protocol) Init(1) W
Warn if inherited methods are unimplemented
Wredundant-decls
C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
Warn about multiple declarations of the same object
Wreorder
C++ ObjC++ Var(warn_reorder) Warning LangEnabledBy(C++ ObjC++,Wall)
Warn when the compiler reorders code
-Wreturn-local-addr
-C ObjC C++ ObjC++ Var(warn_return_local_addr) Init(1) Warning
-Warn about returning a pointer/reference to a local or temporary variable.
-
Wreturn-type
C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
Warn whenever a function's return type defaults to \"int\" (C), or about inconsistent return types (C++)
Wselector
ObjC ObjC++ Var(warn_selector) Warning
Warn if a selector has multiple methods
Wshadow-ivar
ObjC ObjC++ Var(warn_shadow_ivar) EnabledBy(Wshadow) Init(1) Warning
Index: gcc/common.opt
===================================================================
--- gcc/common.opt (revision 213251)
+++ gcc/common.opt (working copy)
@@ -600,20 +600,24 @@ Common Var(warn_packed) Warning
Warn when the packed attribute has no effect on struct layout
Wpadded
Common Var(warn_padded) Warning
Warn when padding is required to align structure members
Wpedantic
Common Var(pedantic) Warning
Issue warnings needed for strict compliance to the standard
+Wreturn-local-addr
+Common Var(warn_return_local_addr) Init(1) Warning
+Warn about returning a pointer/reference to a local or temporary variable.
+
Wshadow
Common Var(warn_shadow) Warning
Warn when one local variable shadows another
Wstack-protector
Common Var(warn_stack_protect) Warning
Warn when not issuing stack smashing protection for some reason
Wstack-usage=
Common Joined RejectNegative UInteger Var(warn_stack_usage) Init(-1) Warning
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c (revision 213251)
+++ gcc/cp/typeck.c (working copy)
@@ -49,21 +49,21 @@ static tree convert_for_assignment (tree
static tree cp_pointer_int_sum (enum tree_code, tree, tree, tsubst_flags_t);
static tree rationalize_conditional_expr (enum tree_code, tree,
tsubst_flags_t);
static int comp_ptr_ttypes_real (tree, tree, int);
static bool comp_except_types (tree, tree, bool);
static bool comp_array_types (const_tree, const_tree, bool);
static tree pointer_diff (tree, tree, tree, tsubst_flags_t);
static tree get_delta_difference (tree, tree, bool, bool, tsubst_flags_t);
static void casts_away_constness_r (tree *, tree *, tsubst_flags_t);
static bool casts_away_constness (tree, tree, tsubst_flags_t);
-static void maybe_warn_about_returning_address_of_local (tree);
+static bool maybe_warn_about_returning_address_of_local (tree);
static tree lookup_destructor (tree, tree, tree, tsubst_flags_t);
static void warn_args_num (location_t, tree, bool);
static int convert_arguments (tree, vec<tree, va_gc> **, tree, int,
tsubst_flags_t);
/* Do `exp = require_complete_type (exp);' to make sure exp
does not have an incomplete type. (That includes void types.)
Returns error_mark_node if the VALUE does not have
complete type when this function returns. */
@@ -8289,82 +8289,84 @@ convert_for_initialization (tree exp, tr
return rhs;
if (MAYBE_CLASS_TYPE_P (type))
return perform_implicit_conversion_flags (type, rhs, complain, flags);
return convert_for_assignment (type, rhs, errtype, fndecl, parmnum,
complain, flags);
}
/* If RETVAL is the address of, or a reference to, a local variable or
- temporary give an appropriate warning. */
+ temporary give an appropriate warning and return true. */
-static void
+static bool
maybe_warn_about_returning_address_of_local (tree retval)
{
tree valtype = TREE_TYPE (DECL_RESULT (current_function_decl));
tree whats_returned = retval;
for (;;)
{
if (TREE_CODE (whats_returned) == COMPOUND_EXPR)
whats_returned = TREE_OPERAND (whats_returned, 1);
else if (CONVERT_EXPR_P (whats_returned)
|| TREE_CODE (whats_returned) == NON_LVALUE_EXPR)
whats_returned = TREE_OPERAND (whats_returned, 0);
else
break;
}
if (TREE_CODE (whats_returned) != ADDR_EXPR)
- return;
+ return false;
whats_returned = TREE_OPERAND (whats_returned, 0);
while (TREE_CODE (whats_returned) == COMPONENT_REF
|| TREE_CODE (whats_returned) == ARRAY_REF)
whats_returned = TREE_OPERAND (whats_returned, 0);
if (TREE_CODE (valtype) == REFERENCE_TYPE)
{
if (TREE_CODE (whats_returned) == AGGR_INIT_EXPR
|| TREE_CODE (whats_returned) == TARGET_EXPR)
{
warning (OPT_Wreturn_local_addr, "returning reference to temporary");
- return;
+ return true;
}
if (VAR_P (whats_returned)
&& DECL_NAME (whats_returned)
&& TEMP_NAME_P (DECL_NAME (whats_returned)))
{
warning (OPT_Wreturn_local_addr, "reference to non-lvalue returned");
- return;
+ return true;
}
}
if (DECL_P (whats_returned)
&& DECL_NAME (whats_returned)
&& DECL_FUNCTION_SCOPE_P (whats_returned)
&& !is_capture_proxy (whats_returned)
&& !(TREE_STATIC (whats_returned)
|| TREE_PUBLIC (whats_returned)))
{
if (TREE_CODE (valtype) == REFERENCE_TYPE)
warning (OPT_Wreturn_local_addr, "reference to local variable %q+D returned",
whats_returned);
else if (TREE_CODE (whats_returned) == LABEL_DECL)
warning (OPT_Wreturn_local_addr, "address of label %q+D returned",
whats_returned);
else
warning (OPT_Wreturn_local_addr, "address of local variable %q+D "
"returned", whats_returned);
- return;
+ return true;
}
+
+ return false;
}
/* Check that returning RETVAL from the current function is valid.
Return an expression explicitly showing all conversions required to
change RETVAL into the function return type, and to assign it to
the DECL_RESULT for the function. Set *NO_WARNING to true if
code reaches end of non-void function warning shouldn't be issued
on this RETURN_EXPR. */
tree
@@ -8643,22 +8645,23 @@ check_return_expr (tree retval, bool *no
/* If the conversion failed, treat this just like `return;'. */
if (retval == error_mark_node)
return retval;
/* We can't initialize a register from a AGGR_INIT_EXPR. */
else if (! cfun->returns_struct
&& TREE_CODE (retval) == TARGET_EXPR
&& TREE_CODE (TREE_OPERAND (retval, 1)) == AGGR_INIT_EXPR)
retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
TREE_OPERAND (retval, 0));
- else
- maybe_warn_about_returning_address_of_local (retval);
+ else if (maybe_warn_about_returning_address_of_local (retval))
+ retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
+ build_zero_cst (TREE_TYPE (retval)));
}
/* Actually copy the value returned into the appropriate location. */
if (retval && retval != result)
retval = build2 (INIT_EXPR, TREE_TYPE (result), result, retval);
return retval;
}
Index: gcc/gimple-ssa-isolate-paths.c
===================================================================
--- gcc/gimple-ssa-isolate-paths.c (revision 213251)
+++ gcc/gimple-ssa-isolate-paths.c (working copy)
@@ -35,20 +35,22 @@ along with GCC; see the file COPYING3.
#include "tree-ssa.h"
#include "stringpool.h"
#include "tree-ssanames.h"
#include "gimple-ssa.h"
#include "tree-ssa-operands.h"
#include "tree-phinodes.h"
#include "ssa-iterators.h"
#include "cfgloop.h"
#include "tree-pass.h"
#include "tree-cfg.h"
+#include "diagnostic-core.h"
+#include "intl.h"
static bool cfg_altered;
/* Callback for walk_stmt_load_store_ops.
Return TRUE if OP will dereference the tree stored in DATA, FALSE
otherwise.
This routine only makes a superficial check for a dereference. Thus,
@@ -125,41 +127,44 @@ insert_trap_and_remove_trailing_statemen
/* BB when reached via incoming edge E will exhibit undefined behaviour
at STMT. Isolate and optimize the path which exhibits undefined
behaviour.
Isolation is simple. Duplicate BB and redirect E to BB'.
Optimization is simple as well. Replace STMT in BB' with an
unconditional trap and remove all outgoing edges from BB'.
+ If RET_ZERO, do not trap, only return NULL.
+
DUPLICATE is a pre-existing duplicate, use it as BB' if it exists.
Return BB'. */
basic_block
isolate_path (basic_block bb, basic_block duplicate,
- edge e, gimple stmt, tree op)
+ edge e, gimple stmt, tree op, bool ret_zero)
{
gimple_stmt_iterator si, si2;
edge_iterator ei;
edge e2;
/* First duplicate BB if we have not done so already and remove all
the duplicate's outgoing edges as duplicate is going to unconditionally
trap. Removing the outgoing edges is both an optimization and ensures
we don't need to do any PHI node updates. */
if (!duplicate)
{
duplicate = duplicate_block (bb, NULL, NULL);
- for (ei = ei_start (duplicate->succs); (e2 = ei_safe_edge (ei)); )
- remove_edge (e2);
+ if (!ret_zero)
+ for (ei = ei_start (duplicate->succs); (e2 = ei_safe_edge (ei)); )
+ remove_edge (e2);
}
/* Complete the isolation step by redirecting E to reach DUPLICATE. */
e2 = redirect_edge_and_branch (e, duplicate);
if (e2)
flush_pending_stmts (e2);
/* There may be more than one statement in DUPLICATE which exhibits
undefined behaviour. Ultimately we want the first such statement in
@@ -190,21 +195,31 @@ isolate_path (basic_block bb, basic_bloc
}
/* This would be an indicator that we never found STMT in BB, which should
never happen. */
gcc_assert (!gsi_end_p (si));
/* If we did not run to the end of DUPLICATE, then SI points to STMT and
SI2 points to the duplicate of STMT in DUPLICATE. Insert a trap
before SI2 and remove SI2 and all trailing statements. */
if (!gsi_end_p (si2))
- insert_trap_and_remove_trailing_statements (&si2, op);
+ {
+ if (ret_zero)
+ {
+ gimple ret = gsi_stmt (si2);
+ tree zero = build_zero_cst (TREE_TYPE (gimple_return_retval (ret)));
+ gimple_return_set_retval (ret, zero);
+ update_stmt (ret);
+ }
+ else
+ insert_trap_and_remove_trailing_statements (&si2, op);
+ }
return duplicate;
}
/* Look for PHI nodes which feed statements in the same block where
the value of the PHI node implies the statement is erroneous.
For example, a NULL PHI arg value which then feeds a pointer
dereference.
@@ -248,47 +263,80 @@ find_implicit_erroneous_behaviour (void)
arguments are NULL.
When we remove an edge, we want to reprocess the current
index, hence the ugly way we update I for each iteration. */
basic_block duplicate = NULL;
for (unsigned i = 0, next_i = 0;
i < gimple_phi_num_args (phi);
i = next_i)
{
tree op = gimple_phi_arg_def (phi, i);
+ edge e = gimple_phi_arg_edge (phi, i);
+ imm_use_iterator iter;
+ gimple use_stmt;
next_i = i + 1;
+ if (TREE_CODE (op) == ADDR_EXPR)
+ {
+ tree valbase = get_base_address (TREE_OPERAND (op, 0));
+ if ((TREE_CODE (valbase) == VAR_DECL
+ && !is_global_var (valbase))
+ || TREE_CODE (valbase) == PARM_DECL)
+ {
+ FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
+ {
+ if (gimple_code (use_stmt) != GIMPLE_RETURN
+ || gimple_return_retval (use_stmt) != lhs)
+ continue;
+
+ if (warning_at (gimple_location (use_stmt),
+ OPT_Wreturn_local_addr,
+ "function may return address "
+ "of local variable"))
+ inform (DECL_SOURCE_LOCATION(valbase),
+ "declared here");
+
+ if (gimple_bb (use_stmt) == bb)
+ {
+ duplicate = isolate_path (bb, duplicate, e,
+ use_stmt, lhs, true);
+
+ /* When we remove an incoming edge, we need to
+ reprocess the Ith element. */
+ next_i = i;
+ cfg_altered = true;
+ }
+ }
+ }
+ }
+
if (!integer_zerop (op))
continue;
- edge e = gimple_phi_arg_edge (phi, i);
- imm_use_iterator iter;
- gimple use_stmt;
-
/* We've got a NULL PHI argument. Now see if the
PHI's result is dereferenced within BB. */
FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
{
/* We only care about uses in BB. Catching cases in
in other blocks would require more complex path
isolation code. */
if (gimple_bb (use_stmt) != bb)
continue;
if (infer_nonnull_range (use_stmt, lhs,
flag_isolate_erroneous_paths_dereference,
flag_isolate_erroneous_paths_attribute))
{
- duplicate = isolate_path (bb, duplicate,
- e, use_stmt, lhs);
+ duplicate = isolate_path (bb, duplicate, e,
+ use_stmt, lhs, false);
/* When we remove an incoming edge, we need to
reprocess the Ith element. */
next_i = i;
cfg_altered = true;
}
}
}
}
}
@@ -340,23 +388,59 @@ find_explicit_erroneous_behaviour (void)
for (edge_iterator ei = ei_start (bb->succs);
(e = ei_safe_edge (ei)); )
remove_edge (e);
/* Ignore any more operands on this statement and
continue the statement iterator (which should
terminate its loop immediately. */
cfg_altered = true;
break;
}
+
+ /* Detect returning the address of a local variable. This only
+ becomes undefined behavior if the result is used, so we do not
+ insert a trap and only return NULL instead. */
+ if (gimple_code (stmt) == GIMPLE_RETURN)
+ {
+ tree val = gimple_return_retval (stmt);
+ if (val && TREE_CODE (val) == ADDR_EXPR)
+ {
+ tree valbase = get_base_address (TREE_OPERAND (val, 0));
+ if ((TREE_CODE (valbase) == VAR_DECL
+ && !is_global_var (valbase))
+ || TREE_CODE (valbase) == PARM_DECL)
+ {
+ /* We only need it for this particular case. */
+ calculate_dominance_info (CDI_POST_DOMINATORS);
+ const char* msg;
+ bool always_executed = dominated_by_p
+ (CDI_POST_DOMINATORS,
+ single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb);
+ if (always_executed)
+ msg = N_("function returns address of local variable");
+ else
+ msg = N_("function may return address of "
+ "local variable");
+
+ if (warning_at (gimple_location (stmt),
+ OPT_Wreturn_local_addr, msg))
+ inform (DECL_SOURCE_LOCATION(valbase), "declared here");
+ tree zero = build_zero_cst (TREE_TYPE (val));
+ gimple_return_set_retval (stmt, zero);
+ update_stmt (stmt);
+ }
+ }
+ }
}
}
}
+
/* Search the function for statements which, if executed, would cause
the program to fault such as a dereference of a NULL pointer.
Such a program can't be valid if such a statement was to execute
according to ISO standards.
We detect explicit NULL pointer dereferences as well as those implied
by a PHI argument having a NULL value which unconditionally flows into
a dereference in the same block as the PHI.
Index: gcc/testsuite/c-c++-common/addrtmp.c
===================================================================
--- gcc/testsuite/c-c++-common/addrtmp.c (revision 0)
+++ gcc/testsuite/c-c++-common/addrtmp.c (working copy)
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef struct A { int a,b; } A;
+int*g(int*x){return x;}
+int*f1(){
+ A x[2]={{1,2},{3,4}};
+ return g(&x[1].a); // { dg-warning "returns address of local variable" }
+}
+int*f2(int n){
+ A x[2]={{1,2},{3,4}};
+ return n?0:g(&x[1].a); // { dg-warning "may return address of local variable" }
+}
+A y[2]={{1,2},{3,4}};
+int*h(){
+ return g(&y[1].a);
+}
+int*j(int n){
+ A x[2]={{1,2},{3,4}};
+ int*p=g(&y[1].a);
+ if(n==1)p=g(&x[1].a);
+ if(n==2)p=g(&x[0].b);
+ return p; // { dg-warning "may return address of local variable" }
+}
+int*s()
+{
+ static int i;
+ return &i;
+}
Index: gcc/testsuite/c-c++-common/uninit-G.c
===================================================================
--- gcc/testsuite/c-c++-common/uninit-G.c (revision 213251)
+++ gcc/testsuite/c-c++-common/uninit-G.c (working copy)
@@ -1,9 +1,10 @@
/* Test we do not warn about initializing variable with address of self in the initialization. */
/* { dg-do compile } */
/* { dg-options "-O -Wuninitialized" } */
-void *f()
+void g(void*);
+void f()
{
void *i = &i;
- return i;
+ g(i);
}
More information about the Gcc-patches
mailing list