This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]