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

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

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1997 bytes --]

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. I kept the same option as the front-ends, I don't know 
if we want a different one, or maybe a Wmaybe-... version. There will be 
cases where we get a duplicate warning from -Wtarget-lifetime in fortran, 
but none in the testsuite, and I would rather have 2 warnings than miss 
such broken code. The uninit-G testcase is about initialization, not 
returning, so I am changing that, even if it is unnecessary with the 
current version of the patch (only activated at -O2).

Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on 
x86_64-unknown-linux-gnu.

(by the way, contrib/compare_tests is confused when I use all languages, 
it prints "comm: file 1 is not in sorted order" and tons of spurious 
differences)

2014-06-23  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 (maybe_warn_about_returning_address_of_local): Return
 	whether it is returning the address of a local variable.
 	(check_return_expr): Return 0 instead of the address of a local
 	variable.
gcc/c-family/
 	* c.opt (-Wreturn-local-addr): Move to common.opt.
gcc/
 	* common.opt (-Wreturn-local-addr): Moved from c.opt.
 	* gimple-ssa-isolate-paths.c: Include diagnostic-core.h.
 	(isolate_path): New argument to avoid inserting a trap.
 	(find_implicit_erroneous_behaviour): Handle returning the address
 	of a local variable.
 	(find_explicit_erroneous_behaviour): Likewise.
 	(gimple_ssa_isolate_erroneous_paths): Calculate dominance info.
gcc/testsuite/
 	* c-c++-common/addrtmp.c: New file.
 	* c-c++-common/uninit-G.c: Adapt.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 18895 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 211874)
+++ gcc/c/c-typeck.c	(working copy)
@@ -9315,22 +9315,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 = build_compound_expr (loc, t, zero);
+		    }
 		}
 	      break;
 
 	    default:
 	      break;
 	    }
 
 	  break;
 	}
 
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 211874)
+++ gcc/c-family/c.opt	(working copy)
@@ -682,24 +682,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 211874)
+++ gcc/common.opt	(working copy)
@@ -596,20 +596,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 211874)
+++ 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.  */
 
@@ -8278,82 +8278,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);
 }
 \f
 /* 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
@@ -8634,22 +8636,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;
 }
 
 \f
Index: gcc/gimple-ssa-isolate-paths.c
===================================================================
--- gcc/gimple-ssa-isolate-paths.c	(revision 211874)
+++ gcc/gimple-ssa-isolate-paths.c	(working copy)
@@ -35,20 +35,21 @@ 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"
 
 
 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 +126,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 +194,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 +262,79 @@ 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))
+		    {
+		      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 +386,55 @@ 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))
+		    {
+		      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 = "function returns address of local variable";
+		      else
+			msg = "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.
 
@@ -371,20 +449,21 @@ find_explicit_erroneous_behaviour (void)
 
    A warning for both cases may be advisable as well.
 
    Other statically detectable violations of the ISO standard could be
    handled in a similar way, such as out-of-bounds array indexing.  */
 
 static unsigned int
 gimple_ssa_isolate_erroneous_paths (void)
 {
   initialize_original_copy_tables ();
+  calculate_dominance_info (CDI_POST_DOMINATORS);
 
   /* Search all the blocks for edges which, if traversed, will
      result in undefined behaviour.  */
   cfg_altered = false;
 
   /* First handle cases where traversal of a particular edge
      triggers undefined behaviour.  These cases require creating
      duplicate blocks and thus new SSA_NAMEs.
 
      We want that process complete prior to the phase where we start
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,24 @@
+/* { 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" }
+}
Index: gcc/testsuite/c-c++-common/uninit-G.c
===================================================================
--- gcc/testsuite/c-c++-common/uninit-G.c	(revision 211874)
+++ 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);
 }

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

[Marc Glisse]

( https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )

On Sun, 22 Jun 2014, Marc Glisse wrote:

> 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. I kept the same option as the front-ends, I don't know if we want 
> a different one, or maybe a Wmaybe-... version. There will be cases where we 
> get a duplicate warning from -Wtarget-lifetime in fortran, but none in the 
> testsuite, and I would rather have 2 warnings than miss such broken code. The 
> uninit-G testcase is about initialization, not returning, so I am changing 
> that, even if it is unnecessary with the current version of the patch (only 
> activated at -O2).
>
> Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on 
> x86_64-unknown-linux-gnu.
>
> (by the way, contrib/compare_tests is confused when I use all languages, it 
> prints "comm: file 1 is not in sorted order" and tons of spurious 
> differences)
>
> 2014-06-23  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 (maybe_warn_about_returning_address_of_local): Return
> 	whether it is returning the address of a local variable.
> 	(check_return_expr): Return 0 instead of the address of a local
> 	variable.
> gcc/c-family/
> 	* c.opt (-Wreturn-local-addr): Move to common.opt.
> gcc/
> 	* common.opt (-Wreturn-local-addr): Moved from c.opt.
> 	* gimple-ssa-isolate-paths.c: Include diagnostic-core.h.
> 	(isolate_path): New argument to avoid inserting a trap.
> 	(find_implicit_erroneous_behaviour): Handle returning the address
> 	of a local variable.
> 	(find_explicit_erroneous_behaviour): Likewise.
> 	(gimple_ssa_isolate_erroneous_paths): Calculate dominance info.
> gcc/testsuite/
> 	* c-c++-common/addrtmp.c: New file.
> 	* c-c++-common/uninit-G.c: Adapt.

After looking at PR 61597, I updated the 2 conditions to:

+		  if ((TREE_CODE (valbase) == VAR_DECL
+		       && !is_global_var (valbase))
+		      || TREE_CODE (valbase) == PARM_DECL)

a PARM_DECL is a local variable and returning its address is wrong, isn't 
it?

(also passes bootstrap+testsuite)

-- 
Marc Glisse

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

[Jeff Law]

On 06/29/14 03:22, Marc Glisse wrote:
>
> After looking at PR 61597, I updated the 2 conditions to:
>
> +          if ((TREE_CODE (valbase) == VAR_DECL
> +               && !is_global_var (valbase))
> +              || TREE_CODE (valbase) == PARM_DECL)
>
> a PARM_DECL is a local variable and returning its address is wrong,
> isn't it?
Right.  It can live in either a caller or callee allocated slot.

When I first glanced at the patch my thought was "why is this in the 
path isolation pass?"  But I see you want to modify the returned value 
to be NULL.   I'll have to look at why you want to do that, but at least 
I understand why it's utilizing the path isolation code.

jeff

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

[Marc Glisse]

On Mon, 30 Jun 2014, Jeff Law wrote:

> On 06/29/14 03:22, Marc Glisse wrote:
>> 
>> After looking at PR 61597, I updated the 2 conditions to:
>> 
>> +          if ((TREE_CODE (valbase) == VAR_DECL
>> +               && !is_global_var (valbase))
>> +              || TREE_CODE (valbase) == PARM_DECL)
>> 
>> a PARM_DECL is a local variable and returning its address is wrong,
>> isn't it?
> Right.  It can live in either a caller or callee allocated slot.

The "caller" case scares me a bit. Is it really wrong to return the 
address in that case? The slot still exists after returning if the caller 
is responsible for it.

> When I first glanced at the patch my thought was "why is this in the path 
> isolation pass?"

A very pragmatic reason is that you and Richard asked me to after v1 of 
the patch... It doesn't matter that much to me where this goes.

> But I see you want to modify the returned value to be NULL. 
> I'll have to look at why you want to do that, but at least I understand why 
> it's utilizing the path isolation code.

Originally I mostly wanted to avoid warning several times. Now that the 
warning is in a pass that runs only once, it isn't that necessary (it 
remains necessary to do something in the front-end to avoid warning again 
in the middle-end). It is still an optimization (it probably helps remove 
dead code), though I could replace 0 with an undefined variable.

-- 
Marc Glisse

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

[Alan Modra]

On Mon, Jun 30, 2014 at 11:37:50PM +0200, Marc Glisse wrote:
> On Mon, 30 Jun 2014, Jeff Law wrote:
> 
> >On 06/29/14 03:22, Marc Glisse wrote:
> >>
> >>After looking at PR 61597, I updated the 2 conditions to:
> >>
> >>+          if ((TREE_CODE (valbase) == VAR_DECL
> >>+               && !is_global_var (valbase))
> >>+              || TREE_CODE (valbase) == PARM_DECL)
> >>
> >>a PARM_DECL is a local variable and returning its address is wrong,
> >>isn't it?
> >Right.  It can live in either a caller or callee allocated slot.
> 
> The "caller" case scares me a bit. Is it really wrong to return the
> address in that case? The slot still exists after returning if the
> caller is responsible for it.

At least on powerpc64, which uses a caller allocated parameter save
area, returning the address of something in the parameter save area
merits a warning.  The ABIs explicitly state that the parameter save
area is not preserved over function calls.

Also note that anything left in a caller allocated parameter save area
will potentially be trashed by arguments written for the next call.

-- 
Alan Modra
Australia Development Lab, IBM

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

[Marc Glisse]

On Wed, 2 Jul 2014, Alan Modra wrote:

> On Mon, Jun 30, 2014 at 11:37:50PM +0200, Marc Glisse wrote:
>> On Mon, 30 Jun 2014, Jeff Law wrote:
>>
>>> On 06/29/14 03:22, Marc Glisse wrote:
>>>>
>>>> After looking at PR 61597, I updated the 2 conditions to:
>>>>
>>>> +          if ((TREE_CODE (valbase) == VAR_DECL
>>>> +               && !is_global_var (valbase))
>>>> +              || TREE_CODE (valbase) == PARM_DECL)
>>>>
>>>> a PARM_DECL is a local variable and returning its address is wrong,
>>>> isn't it?
>>> Right.  It can live in either a caller or callee allocated slot.
>>
>> The "caller" case scares me a bit. Is it really wrong to return the
>> address in that case? The slot still exists after returning if the
>> caller is responsible for it.
>
> At least on powerpc64, which uses a caller allocated parameter save
> area, returning the address of something in the parameter save area
> merits a warning.

Note that the patch not only warns, it also returns NULL instead of the 
pointer (at least the current version does), so it really needs to be 
certain.

> The ABIs explicitly state that the parameter save
> area is not preserved over function calls.

Good, that makes me much more confident, thanks.

> Also note that anything left in a caller allocated parameter save area
> will potentially be trashed by arguments written for the next call.

-- 
Marc Glisse

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

[Jeff Law]

On 07/02/14 06:19, Alan Modra wrote:
> On Mon, Jun 30, 2014 at 11:37:50PM +0200, Marc Glisse wrote:
>> On Mon, 30 Jun 2014, Jeff Law wrote:
>>
>>> On 06/29/14 03:22, Marc Glisse wrote:
>>>>
>>>> After looking at PR 61597, I updated the 2 conditions to:
>>>>
>>>> +          if ((TREE_CODE (valbase) == VAR_DECL
>>>> +               && !is_global_var (valbase))
>>>> +              || TREE_CODE (valbase) == PARM_DECL)
>>>>
>>>> a PARM_DECL is a local variable and returning its address is wrong,
>>>> isn't it?
>>> Right.  It can live in either a caller or callee allocated slot.
>>
>> The "caller" case scares me a bit. Is it really wrong to return the
>> address in that case? The slot still exists after returning if the
>> caller is responsible for it.
>
> At least on powerpc64, which uses a caller allocated parameter save
> area, returning the address of something in the parameter save area
> merits a warning.  The ABIs explicitly state that the parameter save
> area is not preserved over function calls.
>
> Also note that anything left in a caller allocated parameter save area
> will potentially be trashed by arguments written for the next call.
Similarly for the PA ABIs.  If something returned the address of one of 
those slots, we'd definitely want a warning.

I'd be amazed if that wasn't the case for every ABI with a caller 
allocated parameter save-back area.

jeff

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

[Jeff Law]

On 06/30/14 15:37, Marc Glisse wrote:
> On Mon, 30 Jun 2014, Jeff Law wrote:
>
>> On 06/29/14 03:22, Marc Glisse wrote:
>>>
>>> After looking at PR 61597, I updated the 2 conditions to:
>>>
>>> +          if ((TREE_CODE (valbase) == VAR_DECL
>>> +               && !is_global_var (valbase))
>>> +              || TREE_CODE (valbase) == PARM_DECL)
>>>
>>> a PARM_DECL is a local variable and returning its address is wrong,
>>> isn't it?
>> Right.  It can live in either a caller or callee allocated slot.
>
> The "caller" case scares me a bit. Is it really wrong to return the
> address in that case? The slot still exists after returning if the
> caller is responsible for it.
The slot exists, but its contents are undefined.  The caller never even 
knows if the callee ever flushed an object back to those slots or if it 
did flush back, which objects were flushed and in what state.

There was a time when I was pondering using those slots for saving 
callee saved registers on the PA so that leafs that needed a few stack 
slots wouldn't need to allococate  a new frame, instead it'd just use 
those convenient 4 words.  But this turned out to just not be important 
and it would totally hose the unwinding mechanisms that were in use on 
the PA at the time.

Jeff

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

[Jeff Law]

On 06/22/14 12:20, Marc Glisse wrote:
> 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. I kept the same option as the front-ends, I don't
> know if we want a different one, or maybe a Wmaybe-... version. There
> will be cases where we get a duplicate warning from -Wtarget-lifetime in
> fortran, but none in the testsuite, and I would rather have 2 warnings
> than miss such broken code. The uninit-G testcase is about
> initialization, not returning, so I am changing that, even if it is
> unnecessary with the current version of the patch (only activated at -O2).
>
> Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on
> x86_64-unknown-linux-gnu.
>
> (by the way, contrib/compare_tests is confused when I use all languages,
> it prints "comm: file 1 is not in sorted order" and tons of spurious
> differences)
>
> 2014-06-23  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 (maybe_warn_about_returning_address_of_local): Return
>      whether it is returning the address of a local variable.
>      (check_return_expr): Return 0 instead of the address of a local
>      variable.
> gcc/c-family/
>      * c.opt (-Wreturn-local-addr): Move to common.opt.
> gcc/
>      * common.opt (-Wreturn-local-addr): Moved from c.opt.
>      * gimple-ssa-isolate-paths.c: Include diagnostic-core.h.
>      (isolate_path): New argument to avoid inserting a trap.
>      (find_implicit_erroneous_behaviour): Handle returning the address
>      of a local variable.
>      (find_explicit_erroneous_behaviour): Likewise.
>      (gimple_ssa_isolate_erroneous_paths): Calculate dominance info.
> gcc/testsuite/
>      * c-c++-common/addrtmp.c: New file.
>      * c-c++-common/uninit-G.c: Adapt.
I note you don't catch return &localvar in the isolation code -- it 
looks like you just catch those which potentially flow from PHIs.

I realize you're primarily catching that in the front-ends, but can't we 
have cases which aren't caught by the front end, but after optimizations 
we're able to propagate &somelocal into the return statement?

It generally looks good and I'm ready to approve if the answer to the 
above question is "can't happen".  If it can happen, then we ought to 
handle it in the isolation code as well (ought to be relatively easy).

Jeff


>

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

[Marc Glisse]

On Wed, 16 Jul 2014, Jeff Law wrote:

> On 06/22/14 12:20, Marc Glisse wrote:
>> 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. I kept the same option as the front-ends, I don't
>> know if we want a different one, or maybe a Wmaybe-... version. There
>> will be cases where we get a duplicate warning from -Wtarget-lifetime in
>> fortran, but none in the testsuite, and I would rather have 2 warnings
>> than miss such broken code. The uninit-G testcase is about
>> initialization, not returning, so I am changing that, even if it is
>> unnecessary with the current version of the patch (only activated at -O2).
>> 
>> Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on
>> x86_64-unknown-linux-gnu.
>> 
>> (by the way, contrib/compare_tests is confused when I use all languages,
>> it prints "comm: file 1 is not in sorted order" and tons of spurious
>> differences)
>> 
>> 2014-06-23  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 (maybe_warn_about_returning_address_of_local): Return
>>      whether it is returning the address of a local variable.
>>      (check_return_expr): Return 0 instead of the address of a local
>>      variable.
>> gcc/c-family/
>>      * c.opt (-Wreturn-local-addr): Move to common.opt.
>> gcc/
>>      * common.opt (-Wreturn-local-addr): Moved from c.opt.
>>      * gimple-ssa-isolate-paths.c: Include diagnostic-core.h.
>>      (isolate_path): New argument to avoid inserting a trap.
>>      (find_implicit_erroneous_behaviour): Handle returning the address
>>      of a local variable.
>>      (find_explicit_erroneous_behaviour): Likewise.
>>      (gimple_ssa_isolate_erroneous_paths): Calculate dominance info.
>> gcc/testsuite/
>>      * c-c++-common/addrtmp.c: New file.
>>      * c-c++-common/uninit-G.c: Adapt.
> I note you don't catch return &localvar in the isolation code -- it looks 
> like you just catch those which potentially flow from PHIs.

I thought I was handling it in the find_explicit_erroneous_behaviour part 
of the patch (as opposed to the implicit part which deals with PHIs). 
Function f1 in the testcase addrtmp.c has no PHI. Am I missing something?

> I realize you're primarily catching that in the front-ends, but can't we have 
> cases which aren't caught by the front end, but after optimizations we're 
> able to propagate &somelocal into the return statement?

We can, and it was my original motivation. I only added PHI handling when 
you asked for it.

> It generally looks good and I'm ready to approve if the answer to the above 
> question is "can't happen".  If it can happen, then we ought to handle it in 
> the isolation code as well (ought to be relatively easy).

Just to be clear, the approval would include the PARM_DECL tweak in
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02327.html
?

Thanks,

-- 
Marc Glisse

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

[Richard Sandiford]

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.

Thanks,
Richard

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

[Marc Glisse]

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?

Good idea! I'll post the updated patch after testing.

>> +		      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.

I just tried to see how the magic happens when someone calls error_at, and 
it goes through diagnostic_set_info, which contains:

diagnostic_set_info_translated (diagnostic, _(gmsgid), args, location, kind);

So I think the _(...) is already taken care of. But I don't know that code 
at all and I could easily have looked at it wrong.

Thanks for the help,

-- 
Marc Glisse

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

[Andreas Schwab]

Marc Glisse <marc.glisse@inria.fr> writes:

> On Sun, 27 Jul 2014, Richard Sandiford wrote:
>
>> Marc Glisse <marc.glisse@inria.fr> writes:
>>> +		      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.
>
> I just tried to see how the magic happens when someone calls error_at, and
> it goes through diagnostic_set_info, which contains:
>
> diagnostic_set_info_translated (diagnostic, _(gmsgid), args, location, kind);
>
> So I think the _(...) is already taken care of. But I don't know that code
> at all and I could easily have looked at it wrong.

If the msgid is not a direct argument of the diagnostic function you
need to mark the string with N_(...).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Marc Glisse]

On Sun, 27 Jul 2014, Andreas Schwab wrote:

> Marc Glisse <marc.glisse@inria.fr> writes:
>
>> On Sun, 27 Jul 2014, Richard Sandiford wrote:
>>
>>> Marc Glisse <marc.glisse@inria.fr> writes:
>>>> +		      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.
>>
>> I just tried to see how the magic happens when someone calls error_at, and
>> it goes through diagnostic_set_info, which contains:
>>
>> diagnostic_set_info_translated (diagnostic, _(gmsgid), args, location, kind);
>>
>> So I think the _(...) is already taken care of. But I don't know that code
>> at all and I could easily have looked at it wrong.
>
> If the msgid is not a direct argument of the diagnostic function you
> need to mark the string with N_(...).

Ah, ok, thanks.
Actually, shouldn't it be G_ instead? That's what ABOUT-GCC-NLS seems to 
suggest since 2005, though I don't expect it makes any difference for 
simple strings.

-- 
Marc Glisse

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

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1825 bytes --]

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 )

2014-07-30  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 (maybe_warn_about_returning_address_of_local): Return
 	whether it is returning the address of a local variable.
 	(check_return_expr): Return 0 instead of the address of a local
 	variable.
gcc/c-family/
 	* c.opt (-Wreturn-local-addr): Move to common.opt.
gcc/
 	* common.opt (-Wreturn-local-addr): Moved from c.opt.
 	* gimple-ssa-isolate-paths.c: Include diagnostic-core.h and intl.h.
 	(isolate_path): New argument to avoid inserting a trap.
 	(find_implicit_erroneous_behaviour): Handle returning the address
 	of a local variable.
 	(find_explicit_erroneous_behaviour): Likewise.
gcc/testsuite/
 	* c-c++-common/addrtmp.c: New file.
 	* c-c++-common/uninit-G.c: Adapt.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 18321 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 213210)
+++ 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 = build_compound_expr (loc, t, zero);
+		    }
 		}
 	      break;
 
 	    default:
 	      break;
 	    }
 
 	  break;
 	}
 
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 213210)
+++ 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 213210)
+++ 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 213210)
+++ 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);
 }
 \f
 /* 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;
 }
 
 \f
Index: gcc/gimple-ssa-isolate-paths.c
===================================================================
--- gcc/gimple-ssa-isolate-paths.c	(revision 213210)
+++ 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,24 @@
+/* { 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" }
+}
Index: gcc/testsuite/c-c++-common/uninit-G.c
===================================================================
--- gcc/testsuite/c-c++-common/uninit-G.c	(revision 213210)
+++ 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);
 }

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

[David Malcolm]

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?)

Thanks
Dave

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

[Marek Polacek]

On Tue, Jul 29, 2014 at 02:58:03PM -0400, David Malcolm wrote:
> 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;
> }

This is fine.  The variable i has a static storage duration, so is allocated
when the program begins and is deallocated when the program ends.  There's only
one instance of the variable i. 

	Marek

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

[Marc Glisse]

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

is_global_var returns true and we don't warn.

> (e.g. should the test case cover this?)

I can add it if you want. I already test a global variable (function h) 
and I expect this is likely already covered elsewhere, but it would make 
the testcase more complete, so I'll do that.


I just noticed an issue with my patch: since I am using a compound 
expression in the front-ends to preserve side-effects, I am now sometimes 
getting this extra warning:

warning: left-hand operand of comma expression has no effect [-Wunused-value]

(I am surprised the testsuite didn't tell me about it)

I will probably have to use the compound expr only if there are side 
effects.

-- 
Marc Glisse

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

[David Malcolm]

On Tue, 2014-07-29 at 21:13 +0200, Marek Polacek wrote:
> On Tue, Jul 29, 2014 at 02:58:03PM -0400, David Malcolm wrote:
> > 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;
> > }
> 
> This is fine.  The variable i has a static storage duration, so is allocated
> when the program begins and is deallocated when the program ends.  There's only
> one instance of the variable i. 
Sorry, by "what happens" I meant, "is a warning emitted?", and I see
Marc has answered that.

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

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1516 bytes --]

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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 18386 bytes --]

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);
 }
 \f
 /* 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;
 }
 
 \f
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);
 }

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

[Jeff Law]

On 07/22/14 03:03, Marc Glisse wrote:
>> I note you don't catch return &localvar in the isolation code -- it
>> looks like you just catch those which potentially flow from PHIs.
>
> I thought I was handling it in the find_explicit_erroneous_behaviour
> part of the patch (as opposed to the implicit part which deals with
> PHIs). Function f1 in the testcase addrtmp.c has no PHI. Am I missing
> something?
I must have missed that whole block of code.  So, no, I don't think you 
missed anything.  My bad.  I'm going to choose to blame it on using a 
laptop screen on the road rather than my usual monitor at home.




>
>> I realize you're primarily catching that in the front-ends, but can't
>> we have cases which aren't caught by the front end, but after
>> optimizations we're able to propagate &somelocal into the return
>> statement?
>
> We can, and it was my original motivation. I only added PHI handling
> when you asked for it.
Note my comment/question makes no sense now that we've settled that you 
do have the right code in find_explicit_erroneous_behavior :-)


>
>> It generally looks good and I'm ready to approve if the answer to the
>> above question is "can't happen".  If it can happen, then we ought to
>> handle it in the isolation code as well (ought to be relatively easy).
>
> Just to be clear, the approval would include the PARM_DECL tweak in
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02327.html
Yes.

jeff

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

[Jeff Law]

On 07/30/14 05:54, Marc Glisse wrote:
> 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.
Approved.

Jeff