[PATCH] Fix PR79345, better uninit warnings for memory

[PATCH] Fix PR79345, better uninit warnings for memory

[Richard Biener]

The following addresses a regression in uninit warnings that happens
because clobber stmts preclude the very simple-minded support we have
for memory.  The patch fixes this by instead implementing uninit
warnings for memory properly, using the alias oracle walk_aliased_vdefs
helper.

The patch adds better limiting to that interface and fixes one
false positive in fixed-value.c.  Two other false positives are
fixed by the wide-int.h patch posted a few hours ago and a patch
to genemit from Jakub.

Bootstrap and regtest running on x86_64-unknown-linux-gnu with those
prerequesites included.

One issue with the patch is duplicate warnings as TREE_NO_WARNING
doesn't work very well on tcc_reference trees which are not
shared.  A followup could use some sort of hash table to mitigate
this a bit.  OTOH for maybe-uninit uses multiple locations may
be in need to be fixed to silence the warning.  Another thing is
that we walk the function in random (BB) order and thus the
alias oracle walk limiting may result in warnings popping up
and going away in less predictable order (and also be reported
in odd order, like not all must-uninits first).

Comments?  I realize this may introduce (a lot of?) false positives
quite late in the game, more aggressive limiting, like to 2
disambiguations, could solve the testcase in the PR and give up
most of the times while preserving the non-walking case of the
old code.

Richard.

2017-03-01  Richard Biener  <rguenther@suse.de>

	* tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit
	param and abort the walk, returning -1 if it is hit.
	(walk_aliased_vdefs): Take a limit param and pass it on.
	* tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param,
	defaulting to 0 and return a signed int.
	* tree-ssa-uninit.c (struct check_defs_data): New struct.
	(check_defs): New helper.
	(warn_uninitialized_vars): Use walk_aliased_vdefs to warn
	about uninitialized memory.

	* fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid
	bogus uninitialized warning.
	(fixed_convert_from_real): Likewise.

	* g++.dg/warn/Wuninitialized-7.C: New testcase.
	* c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized.
	* gcc.dg/uninit-pr19430-2.c: Add expected warning.

Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 245803)
+++ gcc/tree-ssa-alias.c	(working copy)
@@ -2897,13 +2897,15 @@ walk_non_aliased_vuses (ao_ref *ref, tre
    PHI argument (but only one walk continues on merge points), the
    return value is true if any of the walks was successful.
 
-   The function returns the number of statements walked.  */
+   The function returns the number of statements walked or -1 if
+   LIMIT stmts were walked and the walk was aborted at this point.
+   If LIMIT is zero the walk is not aborted.  */
 
-static unsigned int
+static int
 walk_aliased_vdefs_1 (ao_ref *ref, tree vdef,
 		      bool (*walker)(ao_ref *, tree, void *), void *data,
 		      bitmap *visited, unsigned int cnt,
-		      bool *function_entry_reached)
+		      bool *function_entry_reached, unsigned limit)
 {
   do
     {
@@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
 	  if (!*visited)
 	    *visited = BITMAP_ALLOC (NULL);
 	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
-	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
-					 walker, data, visited, 0,
-					 function_entry_reached);
+	    {
+	      int res = walk_aliased_vdefs_1 (ref,
+					      gimple_phi_arg_def (def_stmt, i),
+					      walker, data, visited, 0,
+					      function_entry_reached, limit);
+	      if (res == -1)
+		return -1;
+	      cnt += res;
+	    }
 	  return cnt;
 	}
 
       /* ???  Do we want to account this to TV_ALIAS_STMT_WALK?  */
       cnt++;
+      if (cnt == limit)
+	return -1;
       if ((!ref
 	   || stmt_may_clobber_ref_p_1 (def_stmt, ref))
 	  && (*walker) (ref, vdef, data))
@@ -2943,14 +2953,14 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
   while (1);
 }
 
-unsigned int
+int
 walk_aliased_vdefs (ao_ref *ref, tree vdef,
 		    bool (*walker)(ao_ref *, tree, void *), void *data,
 		    bitmap *visited,
-		    bool *function_entry_reached)
+		    bool *function_entry_reached, unsigned int limit)
 {
   bitmap local_visited = NULL;
-  unsigned int ret;
+  int ret;
 
   timevar_push (TV_ALIAS_STMT_WALK);
 
@@ -2959,7 +2969,7 @@ walk_aliased_vdefs (ao_ref *ref, tree vd
 
   ret = walk_aliased_vdefs_1 (ref, vdef, walker, data,
 			      visited ? visited : &local_visited, 0,
-			      function_entry_reached);
+			      function_entry_reached, limit);
   if (local_visited)
     BITMAP_FREE (local_visited);
 
Index: gcc/tree-ssa-alias.h
===================================================================
--- gcc/tree-ssa-alias.h	(revision 245803)
+++ gcc/tree-ssa-alias.h	(working copy)
@@ -131,10 +131,11 @@ extern void *walk_non_aliased_vuses (ao_
 				     void *(*)(ao_ref *, tree, void *, bool *),
 				     tree (*)(tree),
 				     void *);
-extern unsigned int walk_aliased_vdefs (ao_ref *, tree,
-					bool (*)(ao_ref *, tree, void *),
-					void *, bitmap *,
-					bool *function_entry_reached = NULL);
+extern int walk_aliased_vdefs (ao_ref *, tree,
+			       bool (*)(ao_ref *, tree, void *),
+			       void *, bitmap *,
+			       bool *function_entry_reached = NULL,
+			       unsigned int limit = 0);
 extern void dump_alias_info (FILE *);
 extern void debug_alias_info (void);
 extern void dump_points_to_solution (FILE *, struct pt_solution *);
Index: gcc/testsuite/g++.dg/warn/Wuninitialized-7.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wuninitialized-7.C	(nonexistent)
+++ gcc/testsuite/g++.dg/warn/Wuninitialized-7.C	(working copy)
@@ -0,0 +1,20 @@
+// { dg-do compile }
+// { dg-require-effective-target c++11 }
+// { dg-options "-O -Wuninitialized" }
+
+struct A {
+    A (int);
+};
+
+struct B: A {
+    const bool x = true;
+
+    B (): A (x ? 3 : 7) { } // { dg-warning "x. is used uninitialized" }
+};
+
+void f (void*);
+void g ()
+{
+  B b;
+  f (&b);
+}
Index: gcc/fixed-value.c
===================================================================
--- gcc/fixed-value.c	(revision 245803)
+++ gcc/fixed-value.c	(working copy)
@@ -130,8 +130,8 @@ fixed_from_string (FIXED_VALUE_TYPE *f,
   real_arithmetic (&fixed_value, MULT_EXPR, &real_value, &base_value);
   wide_int w = real_to_integer (&fixed_value, &fail,
 				GET_MODE_PRECISION (mode));
-  f->data.low = w.elt (0);
-  f->data.high = w.elt (1);
+  f->data.low = w.ulow ();
+  f->data.high = w.uhigh ();
 
   if (temp == FIXED_MAX_EPS && ALL_FRACT_MODE_P (f->mode))
     {
@@ -1049,8 +1049,8 @@ fixed_convert_from_real (FIXED_VALUE_TYP
 
   wide_int w = real_to_integer (&fixed_value, &fail,
 				GET_MODE_PRECISION (mode));
-  f->data.low = w.elt (0);
-  f->data.high = w.elt (1);
+  f->data.low = w.ulow ();
+  f->data.high = w.uhigh ();
   temp = check_real_for_fixed_mode (&real_value, mode);
   if (temp == FIXED_UNDERFLOW) /* Minimum.  */
     {
Index: gcc/testsuite/c-c++-common/ubsan/bounds-2.c
===================================================================
--- gcc/testsuite/c-c++-common/ubsan/bounds-2.c	(revision 245803)
+++ gcc/testsuite/c-c++-common/ubsan/bounds-2.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds" } */
+/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds -Wno-uninitialized" } */
 
 /* Test runtime errors.  */
 
Index: gcc/testsuite/gcc.dg/uninit-pr19430-2.c
===================================================================
--- gcc/testsuite/gcc.dg/uninit-pr19430-2.c	(revision 245803)
+++ gcc/testsuite/gcc.dg/uninit-pr19430-2.c	(working copy)
@@ -10,7 +10,7 @@ int foo (int b)
   p = &i;
   q = &j;
   if (b)
-    x = p;
+    x = p;  /* { dg-warning "i. may be used uninitialized" } */
   else
     x = q;
   return *x;
Index: gcc/tree-ssa-uninit.c
===================================================================
--- gcc/tree-ssa-uninit.c	(revision 245803)
+++ gcc/tree-ssa-uninit.c	(working copy)
@@ -191,11 +191,39 @@ warn_uninit (enum opt_code wc, tree t, t
     }
 }
 
+struct check_defs_data
+{
+  /* If we found any may-defs besides must-def clobbers.  */
+  bool found_may_defs;
+};
+
+/* Callback for walk_aliased_vdefs.  */
+
+static bool
+check_defs (ao_ref *ref, tree vdef, void *data_)
+{
+  check_defs_data *data = (check_defs_data *)data_;
+  gimple *def_stmt = SSA_NAME_DEF_STMT (vdef);
+  /* If this is a clobber then if it is not a kill walk past it.  */
+  if (gimple_clobber_p (def_stmt))
+    {
+      if (stmt_kills_ref_p (def_stmt, ref))
+	return true;
+      return false;
+    }
+  /* Found a may-def on this path.  */
+  data->found_may_defs = true;
+  return true;
+}
+
 static unsigned int
 warn_uninitialized_vars (bool warn_possibly_uninitialized)
 {
   gimple_stmt_iterator gsi;
   basic_block bb;
+  unsigned int vdef_cnt = 0;
+  unsigned int oracle_cnt = 0;
+  unsigned limit = 0;
 
   FOR_EACH_BB_FN (bb, cfun)
     {
@@ -236,39 +264,73 @@ warn_uninitialized_vars (bool warn_possi
 			     stmt, UNKNOWN_LOCATION);
 	    }
 
-	  /* For memory the only cheap thing we can do is see if we
-	     have a use of the default def of the virtual operand.
-	     ???  Not so cheap would be to use the alias oracle via
-	     walk_aliased_vdefs, if we don't find any aliasing vdef
-	     warn as is-used-uninitialized, if we don't find an aliasing
-	     vdef that kills our use (stmt_kills_ref_p), warn as
-	     may-be-used-uninitialized.  But this walk is quadratic and
-	     so must be limited which means we would miss warning
-	     opportunities.  */
-	  use = gimple_vuse (stmt);
-	  if (use
-	      && gimple_assign_single_p (stmt)
-	      && !gimple_vdef (stmt)
-	      && SSA_NAME_IS_DEFAULT_DEF (use))
+	  /* For limiting the alias walk below we count all
+	     vdefs in the function.  We then give the alias walk an
+	     overall budget (and simply not warn for further stmts).
+	     ???  The interface doesn't give us the chance to assign
+	     a maximum cost to each walk (and abort the walk if hit).  */
+	  if (gimple_vdef (stmt))
+	    vdef_cnt++;
+
+	  if (gimple_assign_load_p (stmt)
+	      && gimple_has_location (stmt))
 	    {
 	      tree rhs = gimple_assign_rhs1 (stmt);
-	      tree base = get_base_address (rhs);
+	      if (TREE_NO_WARNING (rhs))
+		continue;
+
+	      ao_ref ref;
+	      ao_ref_init (&ref, rhs);
 
 	      /* Do not warn if it can be initialized outside this function.  */
+	      tree base = ao_ref_base (&ref);
 	      if (!VAR_P (base)
 		  || DECL_HARD_REGISTER (base)
-		  || is_global_var (base))
+		  || is_global_var (base)
+		  || TREE_NO_WARNING (base))
+		continue;
+
+	      /* Limit the walking to a constant number of stmts after
+	         we overcommit quadratic behavior for small functions
+		 and O(n) behavior.  */
+	      if (oracle_cnt > 128 * 128
+		  && oracle_cnt > vdef_cnt * 2)
+		limit = 32;
+	      check_defs_data data;
+	      data.found_may_defs = false;
+	      use = gimple_vuse (stmt);
+	      int res = walk_aliased_vdefs (&ref, use,
+					    check_defs, &data, NULL,
+					    NULL, limit);
+	      if (res == -1)
+		{
+		  oracle_cnt += limit;
+		  continue;
+		}
+	      oracle_cnt += res;
+	      if (data.found_may_defs)
 		continue;
 
+	      /* We didn't find any may-defs so on all paths either
+	         reached function entry or a killing clobber.  */
+	      location_t location
+		= linemap_resolve_location (line_table, gimple_location (stmt),
+					    LRK_SPELLING_LOCATION, NULL);
 	      if (always_executed)
-		warn_uninit (OPT_Wuninitialized, use, gimple_assign_rhs1 (stmt),
-			     base, "%qE is used uninitialized in this function",
-			     stmt, UNKNOWN_LOCATION);
+		{
+		  if (warning_at (location, OPT_Wuninitialized,
+				  "%qE is used uninitialized in this function",
+				  rhs))
+		    /* ???  This is only effective for decls as in
+		       gcc.dg/uninit-B-O0.c.  Avoid doing this for
+		       maybe-uninit uses as it may hide important
+		       locations.  */
+		    TREE_NO_WARNING (rhs) = 1;
+		}
 	      else if (warn_possibly_uninitialized)
-		warn_uninit (OPT_Wmaybe_uninitialized, use,
-			     gimple_assign_rhs1 (stmt), base,
-			     "%qE may be used uninitialized in this function",
-			     stmt, UNKNOWN_LOCATION);
+		warning_at (location, OPT_Wmaybe_uninitialized,
+			    "%qE may be used uninitialized in this function",
+			    rhs);
 	    }
 	}
     }

[PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345)

[Jakub Jelinek]

On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote:
> The patch adds better limiting to that interface and fixes one
> false positive in fixed-value.c.  Two other false positives are
> fixed by the wide-int.h patch posted a few hours ago and a patch
> to genemit from Jakub.

Here is that patch.  Right now insn-emit.c for match_scratch
operands in expanders emits
  rtx operand7 ATTRIBUTE_UNUSED;
...
  rtx operands[8];
  operands[0] = operand0;
...
  // but no operands[7] = something here;
...
  // C code from *.md file (which typically doesn't touch operands[7])
...
  operand7 = operands[7];
  (void) operand7;
...
  // generated code that doesn't use operand7
This triggers -Wuninitialized warning with Richard's patch and is really UB,
even when we actually don't use it and so hopefully optimize away.
The following patch just removes all operand7 and operands[7] references
from the generated code (i.e. operands that are for match_scratch).

Usually match_scratch numbers come after match_operand/match_dup etc.
numbers, but as can be seen, there are few spots where that is not the case.
The patch adds verification of this requirement in genemit and then fixes
the issues it has diagnosed.

Bootstrapped/regtested on x86_64-linux and i686-linux, plus tested with
make s-emit in a cross to
{powerpc64,aarch64,armv7hl,sparc64,s390x,cris,sh,ia64,hppa,mips}-linux, ok
for trunk?

2017-03-01  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/79345
	* gensupport.h (struct pattern_stats): Add min_scratch_opno field.
	* gensupport.c (get_pattern_stats_1) <case MATCH_SCRATCH>: Update it.
	(get_pattern_stats): Initialize it.
	* genemit.c (gen_expand): Verify match_scratch numbers come after
	match_operand/match_dup numbers.
	* config/i386/i386.md (<s>mul<mode>3_highpart): Swap match_dup and
	match_scratch numbers.
	* config/i386/sse.md (avx2_gathersi<mode>, avx2_gatherdi<mode>):
	Likewise.
	* config/s390/s390.md (trunctdsd2): Likewise.

--- gcc/gensupport.h.jj	2017-01-01 12:45:38.000000000 +0100
+++ gcc/gensupport.h	2017-03-01 12:06:21.816440102 +0100
@@ -199,7 +199,8 @@ struct pattern_stats
   /* The largest match_dup, match_op_dup or match_par_dup number found.  */
   int max_dup_opno;
 
-  /* The largest match_scratch number found.  */
+  /* The smallest and largest match_scratch number found.  */
+  int min_scratch_opno;
   int max_scratch_opno;
 
   /* The number of times match_dup, match_op_dup or match_par_dup appears
--- gcc/gensupport.c.jj	2017-01-05 22:10:31.000000000 +0100
+++ gcc/gensupport.c	2017-03-01 12:21:24.830327207 +0100
@@ -3000,6 +3000,10 @@ get_pattern_stats_1 (struct pattern_stat
       break;
 
     case MATCH_SCRATCH:
+      if (stats->min_scratch_opno == -1)
+	stats->min_scratch_opno = XINT (x, 0);
+      else
+	stats->min_scratch_opno = MIN (stats->min_scratch_opno, XINT (x, 0));
       stats->max_scratch_opno = MAX (stats->max_scratch_opno, XINT (x, 0));
       break;
 
@@ -3032,6 +3036,7 @@ get_pattern_stats (struct pattern_stats
 
   stats->max_opno = -1;
   stats->max_dup_opno = -1;
+  stats->min_scratch_opno = -1;
   stats->max_scratch_opno = -1;
   stats->num_dups = 0;
 
--- gcc/genemit.c.jj	2017-01-01 12:45:35.000000000 +0100
+++ gcc/genemit.c	2017-03-01 12:16:28.391343302 +0100
@@ -448,6 +448,10 @@ gen_expand (md_rtx_info *info)
 
   /* Find out how many operands this function has.  */
   get_pattern_stats (&stats, XVEC (expand, 1));
+  if (stats.min_scratch_opno != -1
+      && stats.min_scratch_opno <= MAX (stats.max_opno, stats.max_dup_opno))
+    fatal_at (info->loc, "define_expand for %s needs to have match_scratch "
+			 "numbers above all other operands", XSTR (expand, 0));
 
   /* Output the function name and argument declarations.  */
   printf ("rtx\ngen_%s (", XSTR (expand, 0));
@@ -479,8 +483,6 @@ gen_expand (md_rtx_info *info)
      make a local variable.  */
   for (i = stats.num_generator_args; i <= stats.max_dup_opno; i++)
     printf ("  rtx operand%d;\n", i);
-  for (; i <= stats.max_scratch_opno; i++)
-    printf ("  rtx operand%d ATTRIBUTE_UNUSED;\n", i);
   printf ("  rtx_insn *_val = 0;\n");
   printf ("  start_sequence ();\n");
 
@@ -516,7 +518,7 @@ gen_expand (md_rtx_info *info)
 	 (unless we aren't going to use them at all).  */
       if (XVEC (expand, 1) != 0)
 	{
-	  for (i = 0; i < stats.num_operand_vars; i++)
+	  for (i = 0; i <= MAX (stats.max_opno, stats.max_dup_opno); i++)
 	    {
 	      printf ("    operand%d = operands[%d];\n", i, i);
 	      printf ("    (void) operand%d;\n", i);
--- gcc/config/i386/i386.md.jj	2017-02-22 18:15:48.000000000 +0100
+++ gcc/config/i386/i386.md	2017-03-01 12:23:22.882736837 +0100
@@ -7364,11 +7364,11 @@ (define_expand "<s>mul<mode>3_highpart"
 			   (match_operand:SWI48 1 "nonimmediate_operand"))
 			 (any_extend:<DWI>
 			   (match_operand:SWI48 2 "register_operand")))
-		       (match_dup 4))))
-	      (clobber (match_scratch:SWI48 3))
+		       (match_dup 3))))
+	      (clobber (match_scratch:SWI48 4))
 	      (clobber (reg:CC FLAGS_REG))])]
   ""
-  "operands[4] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));")
+  "operands[3] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));")
 
 (define_insn "*<s>muldi3_highpart_1"
   [(set (match_operand:DI 0 "register_operand" "=d")
--- gcc/config/i386/sse.md.jj	2017-02-07 16:41:32.000000000 +0100
+++ gcc/config/i386/sse.md	2017-03-01 12:24:57.193471018 +0100
@@ -18873,7 +18873,7 @@ (define_expand "avx2_gathersi<mode>"
 		   (unspec:VEC_GATHER_MODE
 		     [(match_operand:VEC_GATHER_MODE 1 "register_operand")
 		      (mem:<ssescalarmode>
-			(match_par_dup 7
+			(match_par_dup 6
 			  [(match_operand 2 "vsib_address_operand")
 			   (match_operand:<VEC_GATHER_IDXSI>
 			      3 "register_operand")
@@ -18881,10 +18881,10 @@ (define_expand "avx2_gathersi<mode>"
 		      (mem:BLK (scratch))
 		      (match_operand:VEC_GATHER_MODE 4 "register_operand")]
 		     UNSPEC_GATHER))
-	      (clobber (match_scratch:VEC_GATHER_MODE 6))])]
+	      (clobber (match_scratch:VEC_GATHER_MODE 7))])]
   "TARGET_AVX2"
 {
-  operands[7]
+  operands[6]
     = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3],
 					operands[5]), UNSPEC_VSIBADDR);
 })
@@ -18934,7 +18934,7 @@ (define_expand "avx2_gatherdi<mode>"
 		   (unspec:VEC_GATHER_MODE
 		     [(match_operand:<VEC_GATHER_SRCDI> 1 "register_operand")
 		      (mem:<ssescalarmode>
-			(match_par_dup 7
+			(match_par_dup 6
 			  [(match_operand 2 "vsib_address_operand")
 			   (match_operand:<VEC_GATHER_IDXDI>
 			      3 "register_operand")
@@ -18942,10 +18942,10 @@ (define_expand "avx2_gatherdi<mode>"
 		      (mem:BLK (scratch))
 		      (match_operand:<VEC_GATHER_SRCDI> 4 "register_operand")]
 		     UNSPEC_GATHER))
-	      (clobber (match_scratch:VEC_GATHER_MODE 6))])]
+	      (clobber (match_scratch:VEC_GATHER_MODE 7))])]
   "TARGET_AVX2"
 {
-  operands[7]
+  operands[6]
     = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3],
 					operands[5]), UNSPEC_VSIBADDR);
 })
--- gcc/config/s390/s390.md.jj	2017-02-06 13:32:14.000000000 +0100
+++ gcc/config/s390/s390.md	2017-03-01 12:45:07.184184616 +0100
@@ -5074,15 +5074,15 @@ (define_insn "truncddsd2"
 
 (define_expand "trunctdsd2"
   [(parallel
-    [(set (match_dup 3)
+    [(set (match_dup 2)
 	  (float_truncate:DD (match_operand:TD 1 "register_operand" "")))
      (unspec:DI [(const_int DFP_RND_PREP_FOR_SHORT_PREC)] UNSPEC_ROUND)
-     (clobber (match_scratch:TD 2 ""))])
+     (clobber (match_scratch:TD 3 ""))])
    (set (match_operand:SD 0 "register_operand" "")
-	(float_truncate:SD (match_dup 3)))]
+	(float_truncate:SD (match_dup 2)))]
   "TARGET_HARD_DFP"
 {
-  operands[3] = gen_reg_rtx (DDmode);
+  operands[2] = gen_reg_rtx (DDmode);
 })
 
 ;
  

	Jakub

Re: [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345)

[Richard Biener]

On Wed, 1 Mar 2017, Jakub Jelinek wrote:

> On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote:
> > The patch adds better limiting to that interface and fixes one
> > false positive in fixed-value.c.  Two other false positives are
> > fixed by the wide-int.h patch posted a few hours ago and a patch
> > to genemit from Jakub.
> 
> Here is that patch.  Right now insn-emit.c for match_scratch
> operands in expanders emits
>   rtx operand7 ATTRIBUTE_UNUSED;
> ...
>   rtx operands[8];
>   operands[0] = operand0;
> ...
>   // but no operands[7] = something here;
> ...
>   // C code from *.md file (which typically doesn't touch operands[7])
> ...
>   operand7 = operands[7];
>   (void) operand7;
> ...
>   // generated code that doesn't use operand7
> This triggers -Wuninitialized warning with Richard's patch and is really UB,
> even when we actually don't use it and so hopefully optimize away.
> The following patch just removes all operand7 and operands[7] references
> from the generated code (i.e. operands that are for match_scratch).
> 
> Usually match_scratch numbers come after match_operand/match_dup etc.
> numbers, but as can be seen, there are few spots where that is not the case.
> The patch adds verification of this requirement in genemit and then fixes
> the issues it has diagnosed.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, plus tested with
> make s-emit in a cross to
> {powerpc64,aarch64,armv7hl,sparc64,s390x,cris,sh,ia64,hppa,mips}-linux, ok
> for trunk?

This is ok.

Thanks for fixing this,
Richard.

> 2017-03-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/79345
> 	* gensupport.h (struct pattern_stats): Add min_scratch_opno field.
> 	* gensupport.c (get_pattern_stats_1) <case MATCH_SCRATCH>: Update it.
> 	(get_pattern_stats): Initialize it.
> 	* genemit.c (gen_expand): Verify match_scratch numbers come after
> 	match_operand/match_dup numbers.
> 	* config/i386/i386.md (<s>mul<mode>3_highpart): Swap match_dup and
> 	match_scratch numbers.
> 	* config/i386/sse.md (avx2_gathersi<mode>, avx2_gatherdi<mode>):
> 	Likewise.
> 	* config/s390/s390.md (trunctdsd2): Likewise.
> 
> --- gcc/gensupport.h.jj	2017-01-01 12:45:38.000000000 +0100
> +++ gcc/gensupport.h	2017-03-01 12:06:21.816440102 +0100
> @@ -199,7 +199,8 @@ struct pattern_stats
>    /* The largest match_dup, match_op_dup or match_par_dup number found.  */
>    int max_dup_opno;
>  
> -  /* The largest match_scratch number found.  */
> +  /* The smallest and largest match_scratch number found.  */
> +  int min_scratch_opno;
>    int max_scratch_opno;
>  
>    /* The number of times match_dup, match_op_dup or match_par_dup appears
> --- gcc/gensupport.c.jj	2017-01-05 22:10:31.000000000 +0100
> +++ gcc/gensupport.c	2017-03-01 12:21:24.830327207 +0100
> @@ -3000,6 +3000,10 @@ get_pattern_stats_1 (struct pattern_stat
>        break;
>  
>      case MATCH_SCRATCH:
> +      if (stats->min_scratch_opno == -1)
> +	stats->min_scratch_opno = XINT (x, 0);
> +      else
> +	stats->min_scratch_opno = MIN (stats->min_scratch_opno, XINT (x, 0));
>        stats->max_scratch_opno = MAX (stats->max_scratch_opno, XINT (x, 0));
>        break;
>  
> @@ -3032,6 +3036,7 @@ get_pattern_stats (struct pattern_stats
>  
>    stats->max_opno = -1;
>    stats->max_dup_opno = -1;
> +  stats->min_scratch_opno = -1;
>    stats->max_scratch_opno = -1;
>    stats->num_dups = 0;
>  
> --- gcc/genemit.c.jj	2017-01-01 12:45:35.000000000 +0100
> +++ gcc/genemit.c	2017-03-01 12:16:28.391343302 +0100
> @@ -448,6 +448,10 @@ gen_expand (md_rtx_info *info)
>  
>    /* Find out how many operands this function has.  */
>    get_pattern_stats (&stats, XVEC (expand, 1));
> +  if (stats.min_scratch_opno != -1
> +      && stats.min_scratch_opno <= MAX (stats.max_opno, stats.max_dup_opno))
> +    fatal_at (info->loc, "define_expand for %s needs to have match_scratch "
> +			 "numbers above all other operands", XSTR (expand, 0));
>  
>    /* Output the function name and argument declarations.  */
>    printf ("rtx\ngen_%s (", XSTR (expand, 0));
> @@ -479,8 +483,6 @@ gen_expand (md_rtx_info *info)
>       make a local variable.  */
>    for (i = stats.num_generator_args; i <= stats.max_dup_opno; i++)
>      printf ("  rtx operand%d;\n", i);
> -  for (; i <= stats.max_scratch_opno; i++)
> -    printf ("  rtx operand%d ATTRIBUTE_UNUSED;\n", i);
>    printf ("  rtx_insn *_val = 0;\n");
>    printf ("  start_sequence ();\n");
>  
> @@ -516,7 +518,7 @@ gen_expand (md_rtx_info *info)
>  	 (unless we aren't going to use them at all).  */
>        if (XVEC (expand, 1) != 0)
>  	{
> -	  for (i = 0; i < stats.num_operand_vars; i++)
> +	  for (i = 0; i <= MAX (stats.max_opno, stats.max_dup_opno); i++)
>  	    {
>  	      printf ("    operand%d = operands[%d];\n", i, i);
>  	      printf ("    (void) operand%d;\n", i);
> --- gcc/config/i386/i386.md.jj	2017-02-22 18:15:48.000000000 +0100
> +++ gcc/config/i386/i386.md	2017-03-01 12:23:22.882736837 +0100
> @@ -7364,11 +7364,11 @@ (define_expand "<s>mul<mode>3_highpart"
>  			   (match_operand:SWI48 1 "nonimmediate_operand"))
>  			 (any_extend:<DWI>
>  			   (match_operand:SWI48 2 "register_operand")))
> -		       (match_dup 4))))
> -	      (clobber (match_scratch:SWI48 3))
> +		       (match_dup 3))))
> +	      (clobber (match_scratch:SWI48 4))
>  	      (clobber (reg:CC FLAGS_REG))])]
>    ""
> -  "operands[4] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));")
> +  "operands[3] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));")
>  
>  (define_insn "*<s>muldi3_highpart_1"
>    [(set (match_operand:DI 0 "register_operand" "=d")
> --- gcc/config/i386/sse.md.jj	2017-02-07 16:41:32.000000000 +0100
> +++ gcc/config/i386/sse.md	2017-03-01 12:24:57.193471018 +0100
> @@ -18873,7 +18873,7 @@ (define_expand "avx2_gathersi<mode>"
>  		   (unspec:VEC_GATHER_MODE
>  		     [(match_operand:VEC_GATHER_MODE 1 "register_operand")
>  		      (mem:<ssescalarmode>
> -			(match_par_dup 7
> +			(match_par_dup 6
>  			  [(match_operand 2 "vsib_address_operand")
>  			   (match_operand:<VEC_GATHER_IDXSI>
>  			      3 "register_operand")
> @@ -18881,10 +18881,10 @@ (define_expand "avx2_gathersi<mode>"
>  		      (mem:BLK (scratch))
>  		      (match_operand:VEC_GATHER_MODE 4 "register_operand")]
>  		     UNSPEC_GATHER))
> -	      (clobber (match_scratch:VEC_GATHER_MODE 6))])]
> +	      (clobber (match_scratch:VEC_GATHER_MODE 7))])]
>    "TARGET_AVX2"
>  {
> -  operands[7]
> +  operands[6]
>      = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3],
>  					operands[5]), UNSPEC_VSIBADDR);
>  })
> @@ -18934,7 +18934,7 @@ (define_expand "avx2_gatherdi<mode>"
>  		   (unspec:VEC_GATHER_MODE
>  		     [(match_operand:<VEC_GATHER_SRCDI> 1 "register_operand")
>  		      (mem:<ssescalarmode>
> -			(match_par_dup 7
> +			(match_par_dup 6
>  			  [(match_operand 2 "vsib_address_operand")
>  			   (match_operand:<VEC_GATHER_IDXDI>
>  			      3 "register_operand")
> @@ -18942,10 +18942,10 @@ (define_expand "avx2_gatherdi<mode>"
>  		      (mem:BLK (scratch))
>  		      (match_operand:<VEC_GATHER_SRCDI> 4 "register_operand")]
>  		     UNSPEC_GATHER))
> -	      (clobber (match_scratch:VEC_GATHER_MODE 6))])]
> +	      (clobber (match_scratch:VEC_GATHER_MODE 7))])]
>    "TARGET_AVX2"
>  {
> -  operands[7]
> +  operands[6]
>      = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3],
>  					operands[5]), UNSPEC_VSIBADDR);
>  })
> --- gcc/config/s390/s390.md.jj	2017-02-06 13:32:14.000000000 +0100
> +++ gcc/config/s390/s390.md	2017-03-01 12:45:07.184184616 +0100
> @@ -5074,15 +5074,15 @@ (define_insn "truncddsd2"
>  
>  (define_expand "trunctdsd2"
>    [(parallel
> -    [(set (match_dup 3)
> +    [(set (match_dup 2)
>  	  (float_truncate:DD (match_operand:TD 1 "register_operand" "")))
>       (unspec:DI [(const_int DFP_RND_PREP_FOR_SHORT_PREC)] UNSPEC_ROUND)
> -     (clobber (match_scratch:TD 2 ""))])
> +     (clobber (match_scratch:TD 3 ""))])
>     (set (match_operand:SD 0 "register_operand" "")
> -	(float_truncate:SD (match_dup 3)))]
> +	(float_truncate:SD (match_dup 2)))]
>    "TARGET_HARD_DFP"
>  {
> -  operands[3] = gen_reg_rtx (DDmode);
> +  operands[2] = gen_reg_rtx (DDmode);
>  })
>  
>  ;
>   
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR79345, better uninit warnings for memory

[Jakub Jelinek]

On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote:
> One issue with the patch is duplicate warnings as TREE_NO_WARNING
> doesn't work very well on tcc_reference trees which are not
> shared.  A followup could use some sort of hash table to mitigate

Can't we use TREE_NO_WARNING on get_base_address instead?  That is
shared...

> @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
>  	  if (!*visited)
>  	    *visited = BITMAP_ALLOC (NULL);
>  	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
> -	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
> -					 walker, data, visited, 0,
> -					 function_entry_reached);
> +	    {
> +	      int res = walk_aliased_vdefs_1 (ref,
> +					      gimple_phi_arg_def (def_stmt, i),
> +					      walker, data, visited, 0,
> +					      function_entry_reached, limit);
> +	      if (res == -1)
> +		return -1;
> +	      cnt += res;
> +	    }

This doesn't look right.  The recursive call starts with cnt of 0 again,
so if say you have limit 20 and cnt 10 when you are about to recurse and
that walk does 15 oracle comparisons, it won't return -1, but 15 and
you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps
another 100000 times.
Can't you just pass cnt instead of 0 and then use
	      if (res == -1)
		return -1;
	      cnt = res;
?  Or you'd need to play gaves with decrementing the limit for the recursive
call.
> +	  /* For limiting the alias walk below we count all
> +	     vdefs in the function.  We then give the alias walk an
> +	     overall budget (and simply not warn for further stmts).
> +	     ???  The interface doesn't give us the chance to assign
> +	     a maximum cost to each walk (and abort the walk if hit).  */

I don't understand the last two lines, I thought you've added the ability
to the interface above and you actually use it.

Otherwise it LGTM, yes, we will end up with new warnings, but unless the
alias oracle is wrong, there shouldn't be too many false positives, right?

	Jakub

Re: [PATCH] Fix PR79345, better uninit warnings for memory

[Richard Biener]

On Thu, 2 Mar 2017, Jakub Jelinek wrote:

> On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote:
> > One issue with the patch is duplicate warnings as TREE_NO_WARNING
> > doesn't work very well on tcc_reference trees which are not
> > shared.  A followup could use some sort of hash table to mitigate
> 
> Can't we use TREE_NO_WARNING on get_base_address instead?  That is
> shared...

At least for VAR_DECL based refs, yes (the only ones we currently
analyze).  For MEM_REF bases it won't work.  It would of 
course only warn once per aggregate and
for a maybe-uninit warning it might be the false positive
(the patch only sets TREE_NO_WARNING on the always-executed case).

For the user maybe we should change the order we walk BBs to RPO
and thus warn at the first maybe-uninit point so we can say
"further uninit warnings suppressed for X"?

> > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
> >  	  if (!*visited)
> >  	    *visited = BITMAP_ALLOC (NULL);
> >  	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
> > -	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
> > -					 walker, data, visited, 0,
> > -					 function_entry_reached);
> > +	    {
> > +	      int res = walk_aliased_vdefs_1 (ref,
> > +					      gimple_phi_arg_def (def_stmt, i),
> > +					      walker, data, visited, 0,
> > +					      function_entry_reached, limit);
> > +	      if (res == -1)
> > +		return -1;
> > +	      cnt += res;
> > +	    }
> 
> This doesn't look right.  The recursive call starts with cnt of 0 again,
> so if say you have limit 20 and cnt 10 when you are about to recurse and
> that walk does 15 oracle comparisons, it won't return -1, but 15 and
> you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps
> another 100000 times.
> Can't you just pass cnt instead of 0 and then use
> 	      if (res == -1)
> 		return -1;
> 	      cnt = res;
> ?  Or you'd need to play gaves with decrementing the limit for the recursive
> call.

Ah, indeed.  Will fix (passing cnt).

> > +	  /* For limiting the alias walk below we count all
> > +	     vdefs in the function.  We then give the alias walk an
> > +	     overall budget (and simply not warn for further stmts).
> > +	     ???  The interface doesn't give us the chance to assign
> > +	     a maximum cost to each walk (and abort the walk if hit).  */
> 
> I don't understand the last two lines, I thought you've added the ability
> to the interface above and you actually use it.

Will adjust the comment.

> Otherwise it LGTM, yes, we will end up with new warnings, but unless the
> alias oracle is wrong, there shouldn't be too many false positives, right?

Yes, we are not warning for the case where there is at least one path
with an initialization, like

void foo(int b)
{
  S s;
  if (b)
    s.a = 1;
  return s.a;
}

will not warn (trivial to add but not necessary for the regression fix).

We also do not warn for pointer-based accesses (so unfortunately no
fix for PR2972).  Also trivial to add, but see PR79814 for more
bootstrap fallout.

Thanks,
Richard.

Re: [PATCH] Fix PR79345, better uninit warnings for memory

[Richard Biener]

On Thu, 2 Mar 2017, Richard Biener wrote:

> On Thu, 2 Mar 2017, Jakub Jelinek wrote:
> 
> > On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote:
> > > One issue with the patch is duplicate warnings as TREE_NO_WARNING
> > > doesn't work very well on tcc_reference trees which are not
> > > shared.  A followup could use some sort of hash table to mitigate
> > 
> > Can't we use TREE_NO_WARNING on get_base_address instead?  That is
> > shared...
> 
> At least for VAR_DECL based refs, yes (the only ones we currently
> analyze).  For MEM_REF bases it won't work.  It would of 
> course only warn once per aggregate and
> for a maybe-uninit warning it might be the false positive
> (the patch only sets TREE_NO_WARNING on the always-executed case).
> 
> For the user maybe we should change the order we walk BBs to RPO
> and thus warn at the first maybe-uninit point so we can say
> "further uninit warnings suppressed for X"?
> 
> > > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
> > >  	  if (!*visited)
> > >  	    *visited = BITMAP_ALLOC (NULL);
> > >  	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
> > > -	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
> > > -					 walker, data, visited, 0,
> > > -					 function_entry_reached);
> > > +	    {
> > > +	      int res = walk_aliased_vdefs_1 (ref,
> > > +					      gimple_phi_arg_def (def_stmt, i),
> > > +					      walker, data, visited, 0,
> > > +					      function_entry_reached, limit);
> > > +	      if (res == -1)
> > > +		return -1;
> > > +	      cnt += res;
> > > +	    }
> > 
> > This doesn't look right.  The recursive call starts with cnt of 0 again,
> > so if say you have limit 20 and cnt 10 when you are about to recurse and
> > that walk does 15 oracle comparisons, it won't return -1, but 15 and
> > you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps
> > another 100000 times.
> > Can't you just pass cnt instead of 0 and then use
> > 	      if (res == -1)
> > 		return -1;
> > 	      cnt = res;
> > ?  Or you'd need to play gaves with decrementing the limit for the recursive
> > call.
> 
> Ah, indeed.  Will fix (passing cnt).
> 
> > > +	  /* For limiting the alias walk below we count all
> > > +	     vdefs in the function.  We then give the alias walk an
> > > +	     overall budget (and simply not warn for further stmts).
> > > +	     ???  The interface doesn't give us the chance to assign
> > > +	     a maximum cost to each walk (and abort the walk if hit).  */
> > 
> > I don't understand the last two lines, I thought you've added the ability
> > to the interface above and you actually use it.
> 
> Will adjust the comment.
> 
> > Otherwise it LGTM, yes, we will end up with new warnings, but unless the
> > alias oracle is wrong, there shouldn't be too many false positives, right?
> 
> Yes, we are not warning for the case where there is at least one path
> with an initialization, like
> 
> void foo(int b)
> {
>   S s;
>   if (b)
>     s.a = 1;
>   return s.a;
> }
> 
> will not warn (trivial to add but not necessary for the regression fix).
> 
> We also do not warn for pointer-based accesses (so unfortunately no
> fix for PR2972).  Also trivial to add, but see PR79814 for more
> bootstrap fallout.

On IRC we decided to wait&see for the TREE_NO_WARNING issue.  So the
following is what I committed.

Bootstrapped / tested on x86_64-unknown-linux-gnu.

Richard.

2017-03-02  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/79345
	PR c++/42000
	* tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit
	param and abort the walk, returning -1 if it is hit.
	(walk_aliased_vdefs): Take a limit param and pass it on.
	* tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param,
	defaulting to 0 and return a signed int.
	* tree-ssa-uninit.c (struct check_defs_data): New struct.
	(check_defs): New helper.
	(warn_uninitialized_vars): Use walk_aliased_vdefs to warn
	about uninitialized memory.

	* fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid
	bogus uninitialized warning.
	(fixed_convert_from_real): Likewise.

	* g++.dg/warn/Wuninitialized-7.C: New testcase.
	* c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized.
	* gcc.dg/uninit-pr19430-2.c: Add expected warning.

Index: gcc/tree-ssa-alias.h
===================================================================
--- gcc/tree-ssa-alias.h	(revision 245803)
+++ gcc/tree-ssa-alias.h	(working copy)
@@ -131,10 +131,11 @@ extern void *walk_non_aliased_vuses (ao_
 				     void *(*)(ao_ref *, tree, void *, bool *),
 				     tree (*)(tree),
 				     void *);
-extern unsigned int walk_aliased_vdefs (ao_ref *, tree,
-					bool (*)(ao_ref *, tree, void *),
-					void *, bitmap *,
-					bool *function_entry_reached = NULL);
+extern int walk_aliased_vdefs (ao_ref *, tree,
+			       bool (*)(ao_ref *, tree, void *),
+			       void *, bitmap *,
+			       bool *function_entry_reached = NULL,
+			       unsigned int limit = 0);
 extern void dump_alias_info (FILE *);
 extern void debug_alias_info (void);
 extern void dump_points_to_solution (FILE *, struct pt_solution *);
Index: gcc/testsuite/g++.dg/warn/Wuninitialized-7.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wuninitialized-7.C	(nonexistent)
+++ gcc/testsuite/g++.dg/warn/Wuninitialized-7.C	(working copy)
@@ -0,0 +1,20 @@
+// { dg-do compile }
+// { dg-require-effective-target c++11 }
+// { dg-options "-O -Wuninitialized" }
+
+struct A {
+    A (int);
+};
+
+struct B: A {
+    const bool x = true;
+
+    B (): A (x ? 3 : 7) { } // { dg-warning "x. is used uninitialized" }
+};
+
+void f (void*);
+void g ()
+{
+  B b;
+  f (&b);
+}
Index: gcc/fixed-value.c
===================================================================
--- gcc/fixed-value.c	(revision 245803)
+++ gcc/fixed-value.c	(working copy)
@@ -130,8 +130,8 @@ fixed_from_string (FIXED_VALUE_TYPE *f,
   real_arithmetic (&fixed_value, MULT_EXPR, &real_value, &base_value);
   wide_int w = real_to_integer (&fixed_value, &fail,
 				GET_MODE_PRECISION (mode));
-  f->data.low = w.elt (0);
-  f->data.high = w.elt (1);
+  f->data.low = w.ulow ();
+  f->data.high = w.uhigh ();
 
   if (temp == FIXED_MAX_EPS && ALL_FRACT_MODE_P (f->mode))
     {
@@ -1049,8 +1049,8 @@ fixed_convert_from_real (FIXED_VALUE_TYP
 
   wide_int w = real_to_integer (&fixed_value, &fail,
 				GET_MODE_PRECISION (mode));
-  f->data.low = w.elt (0);
-  f->data.high = w.elt (1);
+  f->data.low = w.ulow ();
+  f->data.high = w.uhigh ();
   temp = check_real_for_fixed_mode (&real_value, mode);
   if (temp == FIXED_UNDERFLOW) /* Minimum.  */
     {
Index: gcc/testsuite/c-c++-common/ubsan/bounds-2.c
===================================================================
--- gcc/testsuite/c-c++-common/ubsan/bounds-2.c	(revision 245803)
+++ gcc/testsuite/c-c++-common/ubsan/bounds-2.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds" } */
+/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds -Wno-uninitialized" } */
 
 /* Test runtime errors.  */
 
Index: gcc/testsuite/gcc.dg/uninit-pr19430-2.c
===================================================================
--- gcc/testsuite/gcc.dg/uninit-pr19430-2.c	(revision 245803)
+++ gcc/testsuite/gcc.dg/uninit-pr19430-2.c	(working copy)
@@ -10,7 +10,7 @@ int foo (int b)
   p = &i;
   q = &j;
   if (b)
-    x = p;
+    x = p;  /* { dg-warning "i. may be used uninitialized" } */
   else
     x = q;
   return *x;
Index: gcc/tree-ssa-uninit.c
===================================================================
--- gcc/tree-ssa-uninit.c	(revision 245833)
+++ gcc/tree-ssa-uninit.c	(working copy)
@@ -191,11 +191,39 @@ warn_uninit (enum opt_code wc, tree t, t
     }
 }
 
+struct check_defs_data
+{
+  /* If we found any may-defs besides must-def clobbers.  */
+  bool found_may_defs;
+};
+
+/* Callback for walk_aliased_vdefs.  */
+
+static bool
+check_defs (ao_ref *ref, tree vdef, void *data_)
+{
+  check_defs_data *data = (check_defs_data *)data_;
+  gimple *def_stmt = SSA_NAME_DEF_STMT (vdef);
+  /* If this is a clobber then if it is not a kill walk past it.  */
+  if (gimple_clobber_p (def_stmt))
+    {
+      if (stmt_kills_ref_p (def_stmt, ref))
+	return true;
+      return false;
+    }
+  /* Found a may-def on this path.  */
+  data->found_may_defs = true;
+  return true;
+}
+
 static unsigned int
 warn_uninitialized_vars (bool warn_possibly_uninitialized)
 {
   gimple_stmt_iterator gsi;
   basic_block bb;
+  unsigned int vdef_cnt = 0;
+  unsigned int oracle_cnt = 0;
+  unsigned limit = 0;
 
   FOR_EACH_BB_FN (bb, cfun)
     {
@@ -236,39 +264,70 @@ warn_uninitialized_vars (bool warn_possi
 			     stmt, UNKNOWN_LOCATION);
 	    }
 
-	  /* For memory the only cheap thing we can do is see if we
-	     have a use of the default def of the virtual operand.
-	     ???  Not so cheap would be to use the alias oracle via
-	     walk_aliased_vdefs, if we don't find any aliasing vdef
-	     warn as is-used-uninitialized, if we don't find an aliasing
-	     vdef that kills our use (stmt_kills_ref_p), warn as
-	     may-be-used-uninitialized.  But this walk is quadratic and
-	     so must be limited which means we would miss warning
-	     opportunities.  */
-	  use = gimple_vuse (stmt);
-	  if (use
-	      && gimple_assign_single_p (stmt)
-	      && !gimple_vdef (stmt)
-	      && SSA_NAME_IS_DEFAULT_DEF (use))
+	  /* For limiting the alias walk below we count all
+	     vdefs in the function.  */
+	  if (gimple_vdef (stmt))
+	    vdef_cnt++;
+
+	  if (gimple_assign_load_p (stmt)
+	      && gimple_has_location (stmt))
 	    {
 	      tree rhs = gimple_assign_rhs1 (stmt);
-	      tree base = get_base_address (rhs);
+	      if (TREE_NO_WARNING (rhs))
+		continue;
+
+	      ao_ref ref;
+	      ao_ref_init (&ref, rhs);
 
 	      /* Do not warn if it can be initialized outside this function.  */
+	      tree base = ao_ref_base (&ref);
 	      if (!VAR_P (base)
 		  || DECL_HARD_REGISTER (base)
-		  || is_global_var (base))
+		  || is_global_var (base)
+		  || TREE_NO_WARNING (base))
+		continue;
+
+	      /* Limit the walking to a constant number of stmts after
+	         we overcommit quadratic behavior for small functions
+		 and O(n) behavior.  */
+	      if (oracle_cnt > 128 * 128
+		  && oracle_cnt > vdef_cnt * 2)
+		limit = 32;
+	      check_defs_data data;
+	      data.found_may_defs = false;
+	      use = gimple_vuse (stmt);
+	      int res = walk_aliased_vdefs (&ref, use,
+					    check_defs, &data, NULL,
+					    NULL, limit);
+	      if (res == -1)
+		{
+		  oracle_cnt += limit;
+		  continue;
+		}
+	      oracle_cnt += res;
+	      if (data.found_may_defs)
 		continue;
 
+	      /* We didn't find any may-defs so on all paths either
+	         reached function entry or a killing clobber.  */
+	      location_t location
+		= linemap_resolve_location (line_table, gimple_location (stmt),
+					    LRK_SPELLING_LOCATION, NULL);
 	      if (always_executed)
-		warn_uninit (OPT_Wuninitialized, use, gimple_assign_rhs1 (stmt),
-			     base, "%qE is used uninitialized in this function",
-			     stmt, UNKNOWN_LOCATION);
+		{
+		  if (warning_at (location, OPT_Wuninitialized,
+				  "%qE is used uninitialized in this function",
+				  rhs))
+		    /* ???  This is only effective for decls as in
+		       gcc.dg/uninit-B-O0.c.  Avoid doing this for
+		       maybe-uninit uses as it may hide important
+		       locations.  */
+		    TREE_NO_WARNING (rhs) = 1;
+		}
 	      else if (warn_possibly_uninitialized)
-		warn_uninit (OPT_Wmaybe_uninitialized, use,
-			     gimple_assign_rhs1 (stmt), base,
-			     "%qE may be used uninitialized in this function",
-			     stmt, UNKNOWN_LOCATION);
+		warning_at (location, OPT_Wmaybe_uninitialized,
+			    "%qE may be used uninitialized in this function",
+			    rhs);
 	    }
 	}
     }
Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 245833)
+++ gcc/tree-ssa-alias.c	(working copy)
@@ -2897,13 +2897,15 @@ walk_non_aliased_vuses (ao_ref *ref, tre
    PHI argument (but only one walk continues on merge points), the
    return value is true if any of the walks was successful.
 
-   The function returns the number of statements walked.  */
+   The function returns the number of statements walked or -1 if
+   LIMIT stmts were walked and the walk was aborted at this point.
+   If LIMIT is zero the walk is not aborted.  */
 
-static unsigned int
+static int
 walk_aliased_vdefs_1 (ao_ref *ref, tree vdef,
 		      bool (*walker)(ao_ref *, tree, void *), void *data,
 		      bitmap *visited, unsigned int cnt,
-		      bool *function_entry_reached)
+		      bool *function_entry_reached, unsigned limit)
 {
   do
     {
@@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
 	  if (!*visited)
 	    *visited = BITMAP_ALLOC (NULL);
 	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
-	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
-					 walker, data, visited, 0,
-					 function_entry_reached);
+	    {
+	      int res = walk_aliased_vdefs_1 (ref,
+					      gimple_phi_arg_def (def_stmt, i),
+					      walker, data, visited, cnt,
+					      function_entry_reached, limit);
+	      if (res == -1)
+		return -1;
+	      cnt = res;
+	    }
 	  return cnt;
 	}
 
       /* ???  Do we want to account this to TV_ALIAS_STMT_WALK?  */
       cnt++;
+      if (cnt == limit)
+	return -1;
       if ((!ref
 	   || stmt_may_clobber_ref_p_1 (def_stmt, ref))
 	  && (*walker) (ref, vdef, data))
@@ -2943,14 +2953,14 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
   while (1);
 }
 
-unsigned int
+int
 walk_aliased_vdefs (ao_ref *ref, tree vdef,
 		    bool (*walker)(ao_ref *, tree, void *), void *data,
 		    bitmap *visited,
-		    bool *function_entry_reached)
+		    bool *function_entry_reached, unsigned int limit)
 {
   bitmap local_visited = NULL;
-  unsigned int ret;
+  int ret;
 
   timevar_push (TV_ALIAS_STMT_WALK);
 
@@ -2959,7 +2969,7 @@ walk_aliased_vdefs (ao_ref *ref, tree vd
 
   ret = walk_aliased_vdefs_1 (ref, vdef, walker, data,
 			      visited ? visited : &local_visited, 0,
-			      function_entry_reached);
+			      function_entry_reached, limit);
   if (local_visited)
     BITMAP_FREE (local_visited);
 

Re: [PATCH] Fix PR79345, better uninit warnings for memory

[Jeff Law]

On 03/01/2017 07:03 AM, Richard Biener wrote:
>
> The following addresses a regression in uninit warnings that happens
> because clobber stmts preclude the very simple-minded support we have
> for memory.  The patch fixes this by instead implementing uninit
> warnings for memory properly, using the alias oracle walk_aliased_vdefs
> helper.
>
> The patch adds better limiting to that interface and fixes one
> false positive in fixed-value.c.  Two other false positives are
> fixed by the wide-int.h patch posted a few hours ago and a patch
> to genemit from Jakub.
>
> Bootstrap and regtest running on x86_64-unknown-linux-gnu with those
> prerequesites included.
>
> One issue with the patch is duplicate warnings as TREE_NO_WARNING
> doesn't work very well on tcc_reference trees which are not
> shared.  A followup could use some sort of hash table to mitigate
> this a bit.  OTOH for maybe-uninit uses multiple locations may
> be in need to be fixed to silence the warning.  Another thing is
> that we walk the function in random (BB) order and thus the
> alias oracle walk limiting may result in warnings popping up
> and going away in less predictable order (and also be reported
> in odd order, like not all must-uninits first).
>
> Comments?  I realize this may introduce (a lot of?) false positives
> quite late in the game, more aggressive limiting, like to 2
> disambiguations, could solve the testcase in the PR and give up
> most of the times while preserving the non-walking case of the
> old code.
I'm all for it.  Even if it triggers some false positives, improved 
detection of uninit memory at compile time is a big win in my mind.

jeff

[BUILDROBOT] Maybe uninitialized warnings in mips targets (was: [PATCH] Fix PR79345, better uninit warnings for memory)

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 4777 bytes --]

Hi Richard, Catherine, Matthew

On Thu, 2017-03-02 14:40:46 +0100, Richard Biener <rguenther@suse.de> wrote:
[...]
> On IRC we decided to wait&see for the TREE_NO_WARNING issue.  So the
> following is what I committed.
> 
> Bootstrapped / tested on x86_64-unknown-linux-gnu.
[...]
> 2017-03-02  Richard Biener  <rguenther@suse.de>
> 
> 	PR tree-optimization/79345
> 	PR c++/42000
> 	* tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit
> 	param and abort the walk, returning -1 if it is hit.
> 	(walk_aliased_vdefs): Take a limit param and pass it on.
> 	* tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param,
> 	defaulting to 0 and return a signed int.
> 	* tree-ssa-uninit.c (struct check_defs_data): New struct.
> 	(check_defs): New helper.
> 	(warn_uninitialized_vars): Use walk_aliased_vdefs to warn
> 	about uninitialized memory.
> 
> 	* fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid
> 	bogus uninitialized warning.
> 	(fixed_convert_from_real): Likewise.
> 
> 	* g++.dg/warn/Wuninitialized-7.C: New testcase.
> 	* c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized.
> 	* gcc.dg/uninit-pr19430-2.c: Add expected warning.

When building with config-list.mk, it seems to break for all of the
listed MIPS targets, but not on any other architecture:

[...]
g++ -fno-PIE -c   -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE   -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common  -DHAVE_CONFIG_H -I. -I. -I/scratch/4/jbglaw/configlist/repos/gcc/gcc -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/. -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../include -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libcpp/include -I/opt/cfarm/mpc/include  -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libdecnumber -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libbacktrace   -o mips.o -MT mips.o -MMD -MP -MF ./.deps/mips.TPo /scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c
In file included from /scratch/4/jbglaw/configlist/repos/gcc/gcc/hash-table.h:236:0,
                 from /scratch/4/jbglaw/configlist/repos/gcc/gcc/coretypes.h:369,
                 from /scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c:26:
/scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h: In function ‘mips_multi_member* mips_multi_add()’:
/scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   *slot = obj;
   ^
/scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h: In function ‘void mips_process_sync_loop(rtx_insn*, rtx_def**)’:
/scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   *slot = obj;
   ^
/scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   *slot = obj;
   ^
/scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   *slot = obj;
   ^
/scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c: In function ‘bool mips_expand_vec_perm_const(rtx_def**)’:
/scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c:21362:10: error: ‘orig_perm’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   memcpy (d.perm, orig_perm, MAX_VECT_LEN);
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [mips.o] Error 1


See eg. all the most recent builds:
mips64el-st-linux-gnu:	http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699675
mips64orion-elf:	http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699678
mipsisa32r2-linux-gnu:	http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699702
mipsisa32-elfoabi:	http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699701
mipsisa64sb1-elf:	http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699724
mipsisa64sr71k-elf:	http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699726
mips-netbsd:		http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699750

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:             God put me on earth to accomplish a certain number of
the second  :            things. Right now I am so far behind I will never die.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [BUILDROBOT] Maybe uninitialized warnings in mips targets

[Jeff Law]

On 03/18/2017 12:20 PM, Jan-Benedict Glaw wrote:
> Hi Richard, Catherine, Matthew
>
> On Thu, 2017-03-02 14:40:46 +0100, Richard Biener <rguenther@suse.de> wrote:
> [...]
>> On IRC we decided to wait&see for the TREE_NO_WARNING issue.  So the
>> following is what I committed.
>>
>> Bootstrapped / tested on x86_64-unknown-linux-gnu.
> [...]
>> 2017-03-02  Richard Biener  <rguenther@suse.de>
>>
>> 	PR tree-optimization/79345
>> 	PR c++/42000
>> 	* tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit
>> 	param and abort the walk, returning -1 if it is hit.
>> 	(walk_aliased_vdefs): Take a limit param and pass it on.
>> 	* tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param,
>> 	defaulting to 0 and return a signed int.
>> 	* tree-ssa-uninit.c (struct check_defs_data): New struct.
>> 	(check_defs): New helper.
>> 	(warn_uninitialized_vars): Use walk_aliased_vdefs to warn
>> 	about uninitialized memory.
>>
>> 	* fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid
>> 	bogus uninitialized warning.
>> 	(fixed_convert_from_real): Likewise.
>>
>> 	* g++.dg/warn/Wuninitialized-7.C: New testcase.
>> 	* c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized.
>> 	* gcc.dg/uninit-pr19430-2.c: Add expected warning.
>
> When building with config-list.mk, it seems to break for all of the
> listed MIPS targets, but not on any other architecture:
Yes.  The improved uninitialized memory warnings are catching some 
things that were previously missed.

I've got fixes for all the MIPS thingies in a local tree, but haven't 
submitted them (yet).

jeff

Re: [BUILDROBOT] Maybe uninitialized warnings in mips targets

[Jeff Law]

On 03/18/2017 12:20 PM, Jan-Benedict Glaw wrote:
> Hi Richard, Catherine, Matthew
>
> On Thu, 2017-03-02 14:40:46 +0100, Richard Biener <rguenther@suse.de> wrote:
> [...]
>> On IRC we decided to wait&see for the TREE_NO_WARNING issue.  So the
>> following is what I committed.
>>
>> Bootstrapped / tested on x86_64-unknown-linux-gnu.
> [...]
>> 2017-03-02  Richard Biener  <rguenther@suse.de>
>>
>> 	PR tree-optimization/79345
>> 	PR c++/42000
>> 	* tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit
>> 	param and abort the walk, returning -1 if it is hit.
>> 	(walk_aliased_vdefs): Take a limit param and pass it on.
>> 	* tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param,
>> 	defaulting to 0 and return a signed int.
>> 	* tree-ssa-uninit.c (struct check_defs_data): New struct.
>> 	(check_defs): New helper.
>> 	(warn_uninitialized_vars): Use walk_aliased_vdefs to warn
>> 	about uninitialized memory.
>>
>> 	* fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid
>> 	bogus uninitialized warning.
>> 	(fixed_convert_from_real): Likewise.
>>
>> 	* g++.dg/warn/Wuninitialized-7.C: New testcase.
>> 	* c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized.
>> 	* gcc.dg/uninit-pr19430-2.c: Add expected warning.
>
> When building with config-list.mk, it seems to break for all of the
> listed MIPS targets, but not on any other architecture:
I finally got around to installing the fixes to the MIPS backend for 
this problem.

Jeff