| Message ID | alpine.DEB.2.02.1404051508500.22861@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Sat, Apr 5, 2014 at 3:52 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > Hello, > > we have front-end warnings about returning the address of a local variable. > However, quite often in C++, people don't directly return the address of a > temporary, it goes through a few functions which hide that fact. After some > inlining, the fact that we are returning the address of a local variable can > become obvious to the compiler, to the point where I have used, for > debugging purposes, grep 'return &' on the optimized dump produced with -O3 > -fkeep-inline-functions (I then had to sort through the global/local > variables). > > fold_stmt looks like a good place for this, but it could go almost anywhere. > It has to happen after enough inlining / copy propagation to make it useful > though. > > One hard part is avoiding duplicate warnings. Replacing the address with 0 > is a convenient way to do that, so I did it both for my new warning and for > the existing C/C++ ones. The patch breaks > gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I > didn't touch that front-end because I don't know fortran, and the warning > message "Pointer at .1. in pointer assignment might outlive the pointer > target" doesn't seem very confident that the thing really is broken enough > to be replaced by 0. I only tested (bootstrap+regression) the default > languages, so ada/go may have a similar issue, to be handled if the approach > seems ok. (I personally wouldn't care about duplicate warnings, but I know > some people can't help complaining about it) > > This doesn't actually fix PR 60517, for that I was thinking of checking for > memory reads if the first stop of walk_aliased_vdefs is a clobber (could > also check __builtin_free), though I don't know in which pass to do that > yet. Note that this will break "working" programs where inlining causes those references to no longer be "returned" (though maybe they already break with the clobbers we insert). I remember that POOMA/PETE was full of this ... (and made it reliably work by flattening all call trees with such calls). Richard. > 2014-04-05 Marc Glisse <marc.glisse@inria.fr> > > PR c++/60517 > gcc/c/ > * c-typeck.c (c_finish_return): Return 0 instead of the address of > a local variable. > gcc/cp/ > * typeck.c (check_return_expr): Likewise. > (maybe_warn_about_returning_address_of_local): Tell the caller if > we warned. > gcc/ > * gimple-fold.c (fold_stmt_1): Warn if returning the address of a > local variable. > gcc/testsuite/ > * c-c++-common/addrtmp.c: New testcase. > * c-c++-common/uninit-G.c: Adjust. > > -- > Marc Glisse > Index: gcc/c/c-typeck.c > =================================================================== > --- gcc/c/c-typeck.c (revision 209157) > +++ gcc/c/c-typeck.c (working copy) > @@ -9254,23 +9254,25 @@ c_finish_return (location_t loc, tree re > inner = TREE_OPERAND (inner, 0); > > while (REFERENCE_CLASS_P (inner) > && TREE_CODE (inner) != INDIRECT_REF) > inner = TREE_OPERAND (inner, 0); > > if (DECL_P (inner) > && !DECL_EXTERNAL (inner) > && !TREE_STATIC (inner) > && DECL_CONTEXT (inner) == current_function_decl) > - 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"); > + t = build_zero_cst (TREE_TYPE (res)); > + } > break; > > default: > break; > } > > break; > } > > retval = build2 (MODIFY_EXPR, TREE_TYPE (res), res, t); > Index: gcc/cp/typeck.c > =================================================================== > --- gcc/cp/typeck.c (revision 209157) > +++ 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. */ > > @@ -8253,79 +8253,81 @@ 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 > 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 > @@ -8606,22 +8608,22 @@ 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 = 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-fold.c > =================================================================== > --- gcc/gimple-fold.c (revision 209157) > +++ gcc/gimple-fold.c (working copy) > @@ -45,20 +45,21 @@ along with GCC; see the file COPYING3. > #include "tree-into-ssa.h" > #include "tree-dfa.h" > #include "tree-ssa.h" > #include "tree-ssa-propagate.h" > #include "target.h" > #include "ipa-utils.h" > #include "gimple-pretty-print.h" > #include "tree-ssa-address.h" > #include "langhooks.h" > #include "gimplify-me.h" > +#include "diagnostic-core.h" > > /* Return true when DECL can be referenced from current unit. > FROM_DECL (if non-null) specify constructor of variable DECL was taken > from. > We can get declarations that are not possible to reference for various > reasons: > > 1) When analyzing C++ virtual tables. > C++ virtual tables do have known constructors even > when they are keyed to other compilation unit. > Those tables can contain pointers to methods and vars > @@ -1366,20 +1367,39 @@ fold_stmt_1 (gimple_stmt_iterator *gsi, > if (tem) > { > tem = build_fold_addr_expr_with_type (tem, TREE_TYPE > (val)); > gimple_debug_bind_set_value (stmt, tem); > changed = true; > } > } > } > break; > > + case 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)) > + { > + if (warning_at (gimple_location (stmt), > OPT_Wreturn_local_addr, > + "function returns address of local > variable")) > + inform (DECL_SOURCE_LOCATION(valbase), "declared here"); > + tree zero = build_zero_cst (TREE_TYPE (val)); > + gimple_return_set_retval (stmt, zero); > + changed = true; > + } > + } > + } > + break; > default:; > } > > stmt = gsi_stmt (*gsi); > > /* Fold *& on the lhs. */ > if (gimple_has_lhs (stmt)) > { > tree lhs = gimple_get_lhs (stmt); > if (lhs && REFERENCE_CLASS_P (lhs)) > 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,13 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +typedef struct A { int a,b; } A; > +int*g(int*x){return x;} > +int*f(){ > + A x[2]={{1,2},{3,4}}; > + return g(&x[1].a); // { dg-warning "address of local variable" } > +} > +A y[2]={{1,2},{3,4}}; > +int*h(){ > + return g(&y[1].a); > +} > > Property changes on: gcc/testsuite/c-c++-common/addrtmp.c > ___________________________________________________________________ > Added: svn:keywords > ## -0,0 +1 ## > +Author Date Id Revision URL > \ No newline at end of property > Added: svn:eol-style > ## -0,0 +1 ## > +native > \ No newline at end of property > Index: gcc/testsuite/c-c++-common/uninit-G.c > =================================================================== > --- gcc/testsuite/c-c++-common/uninit-G.c (revision 209157) > +++ 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); > } >
On Mon, 7 Apr 2014, Richard Biener wrote: >> One hard part is avoiding duplicate warnings. Replacing the address with 0 >> is a convenient way to do that, so I did it both for my new warning and for >> the existing C/C++ ones. The patch breaks >> gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I >> didn't touch that front-end because I don't know fortran, and the warning >> message "Pointer at .1. in pointer assignment might outlive the pointer >> target" doesn't seem very confident that the thing really is broken enough >> to be replaced by 0. I only tested (bootstrap+regression) the default >> languages, so ada/go may have a similar issue, to be handled if the approach >> seems ok. (I personally wouldn't care about duplicate warnings, but I know >> some people can't help complaining about it) >> >> This doesn't actually fix PR 60517, for that I was thinking of checking for >> memory reads if the first stop of walk_aliased_vdefs is a clobber (could >> also check __builtin_free), though I don't know in which pass to do that >> yet. > > Note that this will break "working" programs where inlining causes > those references to no longer be "returned" (though maybe they > already break with the clobbers we insert). I remember that POOMA/PETE > was full of this ... (and made it reliably work by flattening all call trees > with such calls). I mostly see programs that are written and tested using "debug mode" and that appear to be working, but break as soon as they are compiled in "release mode" because of dangling references. Breaking them more seems like a helpful thing to me ;-) (especially with a warning). But I would be happy not replacing the pointer and only emitting a warning, if people don't mind duplicate warnings or if we can find another way to avoid them. >> + if (warning_at (gimple_location (stmt), OPT_Wreturn_local_addr, >> + "function returns address of local variable")) That's a front-end option, I should either move it to gcc/common.opt or use a different option.
> One hard part is avoiding duplicate warnings. Replacing the address with 0 > is a convenient way to do that, so I did it both for my new warning and > for the existing C/C++ ones. The patch breaks > gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I > didn't touch that front-end because I don't know fortran, and the warning > message "Pointer at .1. in pointer assignment might outlive the pointer > target" doesn't seem very confident that the thing really is broken enough > to be replaced by 0. I only tested (bootstrap+regression) the default > languages, so ada/go may have a similar issue, to be handled if the > approach seems ok. Ada is designed to make such a thing impossible (although you can work around the design with kludges like 'Unrestricted_Access) so the compiler will stop.
On 04/05/14 07:52, Marc Glisse wrote: > Hello, > > we have front-end warnings about returning the address of a local > variable. However, quite often in C++, people don't directly return the > address of a temporary, it goes through a few functions which hide that > fact. After some inlining, the fact that we are returning the address of > a local variable can become obvious to the compiler, to the point where > I have used, for debugging purposes, grep 'return &' on the optimized > dump produced with -O3 -fkeep-inline-functions (I then had to sort > through the global/local variables). > > fold_stmt looks like a good place for this, but it could go almost > anywhere. It has to happen after enough inlining / copy propagation to > make it useful though. I was wondering if this would be better implemented as a propagation problem so that cases where some, but not all paths to the return statement have &local which reaches the return. ie ... if (foo) x = &local else x = &global return x; ISTM it ought to be a standard propagation problem and that the problematical ADDR_EXPRs are all going to be in PHI nodes. So I think you just search for problematical ADDR_EXPRs in PHI nodes as your seeds, then forward propagate through the CFG. Ultimately looking for any cases where those ADDR_EXPRs ultimately reach the return statement. Thoughts? jeff
On Mon, 7 Apr 2014, Jeff Law wrote: > On 04/05/14 07:52, Marc Glisse wrote: >> Hello, >> >> we have front-end warnings about returning the address of a local >> variable. However, quite often in C++, people don't directly return the >> address of a temporary, it goes through a few functions which hide that >> fact. After some inlining, the fact that we are returning the address of >> a local variable can become obvious to the compiler, to the point where >> I have used, for debugging purposes, grep 'return &' on the optimized >> dump produced with -O3 -fkeep-inline-functions (I then had to sort >> through the global/local variables). >> >> fold_stmt looks like a good place for this, but it could go almost >> anywhere. It has to happen after enough inlining / copy propagation to >> make it useful though. > I was wondering if this would be better implemented as a propagation problem > so that cases where some, but not all paths to the return statement have > &local which reaches the return. ie > > ... > if (foo) > x = &local > else > x = &global > > return x; > > ISTM it ought to be a standard propagation problem and that the problematical > ADDR_EXPRs are all going to be in PHI nodes. So I think you just search for > problematical ADDR_EXPRs in PHI nodes as your seeds, then forward propagate > through the CFG. Ultimately looking for any cases where those ADDR_EXPRs > ultimately reach the return statement. > > Thoughts? I would tend to start from the return statements (assuming the return type is a pointer), look at the defining statement, do things if it is an assignment of an addr_expr, and recurse if it is a PHI. But maybe my brain is cabled backwards ;-) It would be good to piggy back on a pass that already does something similar, if we go that way. Do you know a convenient one? I am also afraid we may get more false positives, but maybe not. Last, the simple version actually works well enough that it discovered at least one real bug in real code, and I am afraid that by refining it too much we'll delay and get nothing in the end (my time and my knowledge of the compiler are limited enough to make it a real possibility). But I admit that's not a good argument. Thanks for the comments,
On 04/07/14 12:51, Marc Glisse wrote: > > I would tend to start from the return statements (assuming the return > type is a pointer), look at the defining statement, do things if it is > an assignment of an addr_expr, and recurse if it is a PHI. But maybe my > brain is cabled backwards ;-) It works either way. > > I am also afraid we may get more false positives, but maybe not. The only false positives should come from paths which are unexecutable. One could argue that if we find any that we should warn, then isolate the path so that we get an immediate runtime trap rather than letting the address of the local escape through the return value. That in turn would argue for dumping it into gimple-isolate-erroneous-paths ;-) > > Last, the simple version actually works well enough that it discovered > at least one real bug in real code, and I am afraid that by refining it > too much we'll delay and get nothing in the end (my time and my > knowledge of the compiler are limited enough to make it a real > possibility). But I admit that's not a good argument. The difference is I see the enhanced version as being simple enough that we ought to just do it. jeff
On Mon, 7 Apr 2014, Jeff Law wrote: >> I am also afraid we may get more false positives, but maybe not. > The only false positives should come from paths which are unexecutable. One > could argue that if we find any that we should warn, then isolate the path so > that we get an immediate runtime trap rather than letting the address of the > local escape through the return value. > > That in turn would argue for dumping it into gimple-isolate-erroneous-paths > ;-) I wonder if trapping may be too strong? If a function sometimes returns a pointer to a local variable but the caller always ignores the return value in those cases, we can warn, but maybe we don't want to introduce a trap? Replacing the address with a null pointer seemed like a compromise, it will trap when you try to read it, but not if you ignore it. But if you think we can trap, ok.
On Mon, Apr 7, 2014 at 8:51 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Mon, 7 Apr 2014, Jeff Law wrote: > >> On 04/05/14 07:52, Marc Glisse wrote: >>> >>> Hello, >>> >>> we have front-end warnings about returning the address of a local >>> variable. However, quite often in C++, people don't directly return the >>> address of a temporary, it goes through a few functions which hide that >>> fact. After some inlining, the fact that we are returning the address of >>> a local variable can become obvious to the compiler, to the point where >>> I have used, for debugging purposes, grep 'return &' on the optimized >>> dump produced with -O3 -fkeep-inline-functions (I then had to sort >>> through the global/local variables). >>> >>> fold_stmt looks like a good place for this, but it could go almost >>> anywhere. It has to happen after enough inlining / copy propagation to >>> make it useful though. >> >> I was wondering if this would be better implemented as a propagation >> problem so that cases where some, but not all paths to the return statement >> have &local which reaches the return. ie >> >> ... >> if (foo) >> x = &local >> else >> x = &global >> >> return x; >> >> ISTM it ought to be a standard propagation problem and that the >> problematical ADDR_EXPRs are all going to be in PHI nodes. So I think you >> just search for problematical ADDR_EXPRs in PHI nodes as your seeds, then >> forward propagate through the CFG. Ultimately looking for any cases where >> those ADDR_EXPRs ultimately reach the return statement. >> >> Thoughts? > > > I would tend to start from the return statements (assuming the return type > is a pointer), look at the defining statement, do things if it is an > assignment of an addr_expr, and recurse if it is a PHI. But maybe my brain > is cabled backwards ;-) > > It would be good to piggy back on a pass that already does something > similar, if we go that way. Do you know a convenient one? > > I am also afraid we may get more false positives, but maybe not. If you don't mind false positives you can also do the check in points-to analysis, looking at the points-to result for the SSA name we return. Btw, the idea to add this to -fisolate-errorneous-paths sounds good to me, as well as returning NULL for them. Richard. > Last, the simple version actually works well enough that it discovered at > least one real bug in real code, and I am afraid that by refining it too > much we'll delay and get nothing in the end (my time and my knowledge of the > compiler are limited enough to make it a real possibility). But I admit > that's not a good argument. > > Thanks for the comments, > > -- > Marc Glisse
On Sat, 5 Apr 2014, Marc Glisse wrote: > One hard part is avoiding duplicate warnings. Replacing the address with 0 is > a convenient way to do that, so I did it both for my new warning and for the > existing C/C++ ones. The patch breaks gfortran.dg/warn_target_lifetime_2.f90 That's not safe the way you do it; you lose side effects from the original return value. Consider a testcase such as: extern void exit (int); extern void abort (void); int * f (void) { int a[10]; return &a[(exit (0), 0)]; } int main (void) { f (); abort (); } (which produces the existing warning, but which has behavior fully defined to exit with status 0).
Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 209157) +++ gcc/c/c-typeck.c (working copy) @@ -9254,23 +9254,25 @@ c_finish_return (location_t loc, tree re inner = TREE_OPERAND (inner, 0); while (REFERENCE_CLASS_P (inner) && TREE_CODE (inner) != INDIRECT_REF) inner = TREE_OPERAND (inner, 0); if (DECL_P (inner) && !DECL_EXTERNAL (inner) && !TREE_STATIC (inner) && DECL_CONTEXT (inner) == current_function_decl) - 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"); + t = build_zero_cst (TREE_TYPE (res)); + } break; default: break; } break; } retval = build2 (MODIFY_EXPR, TREE_TYPE (res), res, t); Index: gcc/cp/typeck.c =================================================================== --- gcc/cp/typeck.c (revision 209157) +++ 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. */ @@ -8253,79 +8253,81 @@ 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 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 @@ -8606,22 +8608,22 @@ 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 = 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-fold.c =================================================================== --- gcc/gimple-fold.c (revision 209157) +++ gcc/gimple-fold.c (working copy) @@ -45,20 +45,21 @@ along with GCC; see the file COPYING3. #include "tree-into-ssa.h" #include "tree-dfa.h" #include "tree-ssa.h" #include "tree-ssa-propagate.h" #include "target.h" #include "ipa-utils.h" #include "gimple-pretty-print.h" #include "tree-ssa-address.h" #include "langhooks.h" #include "gimplify-me.h" +#include "diagnostic-core.h" /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. We can get declarations that are not possible to reference for various reasons: 1) When analyzing C++ virtual tables. C++ virtual tables do have known constructors even when they are keyed to other compilation unit. Those tables can contain pointers to methods and vars @@ -1366,20 +1367,39 @@ fold_stmt_1 (gimple_stmt_iterator *gsi, if (tem) { tem = build_fold_addr_expr_with_type (tem, TREE_TYPE (val)); gimple_debug_bind_set_value (stmt, tem); changed = true; } } } break; + case 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)) + { + if (warning_at (gimple_location (stmt), OPT_Wreturn_local_addr, + "function returns address of local variable")) + inform (DECL_SOURCE_LOCATION(valbase), "declared here"); + tree zero = build_zero_cst (TREE_TYPE (val)); + gimple_return_set_retval (stmt, zero); + changed = true; + } + } + } + break; default:; } stmt = gsi_stmt (*gsi); /* Fold *& on the lhs. */ if (gimple_has_lhs (stmt)) { tree lhs = gimple_get_lhs (stmt); if (lhs && REFERENCE_CLASS_P (lhs)) 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,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +typedef struct A { int a,b; } A; +int*g(int*x){return x;} +int*f(){ + A x[2]={{1,2},{3,4}}; + return g(&x[1].a); // { dg-warning "address of local variable" } +} +A y[2]={{1,2},{3,4}}; +int*h(){ + return g(&y[1].a); +} Property changes on: gcc/testsuite/c-c++-common/addrtmp.c ___________________________________________________________________ Added: svn:keywords ## -0,0 +1 ## +Author Date Id Revision URL \ No newline at end of property Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Index: gcc/testsuite/c-c++-common/uninit-G.c =================================================================== --- gcc/testsuite/c-c++-common/uninit-G.c (revision 209157) +++ 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); }