On Sat, Apr 5, 2014 at 3:52 PM, Marc Glisse <marc.gli...@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.gli...@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);
> }
>
