Re: Warn when returning the address of a temporary (middle-end)

[Richard Biener <richard dot guenther at gmail dot com>]

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