[PATCH] improve note location and refactor warn_uninit

[PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

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

Richard, some time ago you mentioned you'd had trouble getting
-Wuninitialized to print the note pointing to the uninitialized
variable.  I said I'd look into it, and so I did.  The attached
patch simplifies the warn_uninit() function to get rid of some
redundant cruft: besides a few pointless null tests and
a duplicate argument it also does away with ancient code that's
responsible for the problem you saw.  It turns out that tests
for the problem are already in the test suite but have been
xfailed since the day they were added, so the patch simply
removes the xfails.  I'll go ahead and commit this cleanup
as obvious later this week unless you have suggestions for
further changes.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-17506.diff --]
[-- Type: text/x-patch, Size: 12500 bytes --]

Refactor warn_uninit() code, improve note location.

Related:
PR tree-optimization/17506 - warning about uninitialized variable points to wrong location
PR testsuite/37182 - Revision 139286 caused gcc.dg/pr17506.c and gcc.dg/uninit-15.c

gcc/ChangeLog:

	PR tree-optimization/17506
	PR testsuite/37182
	* tree-ssa-uninit.c (warn_uninit): Refactor and simplify.
	(warn_uninit_phi_uses): Remove argument from calls to warn_uninit.
	(warn_uninitialized_vars): Same.  Reduce visibility of locals.
	(warn_uninitialized_phi): Same.

gcc/testsuite/ChangeLog:

	PR tree-optimization/17506
	PR testsuite/37182
	* gcc.dg/diagnostic-tree-expr-ranges-2.c: Add expected output.
	* gcc.dg/uninit-15-O0.c: Remove xfail.
	* gcc.dg/uninit-15.c: Same.

diff --git a/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c b/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c
index 302e233a04a..1cbcad5ac7d 100644
--- a/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c
+++ b/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c
@@ -3,21 +3,25 @@
 
 int test_uninit_1 (void)
 {
-  int result;
-  return result;  /* { dg-warning "uninitialized" } */
-/* { dg-begin-multiline-output "" }
-   return result;
-          ^~~~~~
+  int result_1;     /* { dg-message "declared here" } */
+  return result_1;  /* { dg-warning "uninitialized" } */
+  /* { dg-begin-multiline-output "" }
+   return result_1;
+          ^~~~~~~~
+   int result_1;
+       ^~~~~~~~
    { dg-end-multiline-output "" } */
 }
 
 int test_uninit_2 (void)
 {
-  int result;
-  result += 3; /* { dg-warning "uninitialized" } */
-/* { dg-begin-multiline-output "" }
-   result += 3;
-   ~~~~~~~^~~~
+  int result_2;     /* { dg-message "declared here" } */
+  result_2 += 3;    /* { dg-warning "uninitialized" } */
+  /* { dg-begin-multiline-output "" }
+   result_2 += 3;
+   ~~~~~~~~~^~~~
+   int result_2;
+       ^~~~~~~~
    { dg-end-multiline-output "" } */
-  return result;
+  return result_2;
 }
diff --git a/gcc/testsuite/gcc.dg/uninit-15-O0.c b/gcc/testsuite/gcc.dg/uninit-15-O0.c
index 36d96348617..1ddddee1388 100644
--- a/gcc/testsuite/gcc.dg/uninit-15-O0.c
+++ b/gcc/testsuite/gcc.dg/uninit-15-O0.c
@@ -14,7 +14,7 @@ void baz();
 
 void bar()
 {
-    int j;           /* { dg-message "was declared here" {} { xfail *-*-* } } */
+    int j;              /* { dg-message "declared here" } */
     for (; foo(j); ++j) /* { dg-warning "is used uninitialized" } */
         baz();
 }
diff --git a/gcc/testsuite/gcc.dg/uninit-15.c b/gcc/testsuite/gcc.dg/uninit-15.c
index 85cb116f349..7352662f627 100644
--- a/gcc/testsuite/gcc.dg/uninit-15.c
+++ b/gcc/testsuite/gcc.dg/uninit-15.c
@@ -20,7 +20,7 @@ void baz (void);
 void
 bar (void)
 {
-  int j; /* { dg-message "note: 'j' was declared here" "" { xfail *-*-* } } */
+  int j;                /* { dg-message "note: 'j' was declared here" } */
   for (; foo (j); ++j)  /* { dg-warning "'j' is used uninitialized" } */
     baz ();
 }
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index ad2cf48819b..3f9eab74e68 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -131,20 +131,19 @@ uninit_undefined_value_p (tree t)
    again for plain uninitialized variables, since optimization may have
    changed conditionally uninitialized to unconditionally uninitialized.  */
 
-/* Emit a warning for EXPR based on variable VAR at the point in the
-   program T, an SSA_NAME, is used being uninitialized.  The exact
-   warning text is in MSGID and DATA is the gimple stmt with info about
-   the location in source code.  When DATA is a GIMPLE_PHI, PHIARG_IDX
-   gives which argument of the phi node to take the location from.  WC
-   is the warning code.  */
+/* Emit warning OPT for variable VAR at the point in the program where
+   the SSA_NAME T is being used uninitialized.  The warning text is in
+   MSGID and STMT is the statement that does the uninitialized read.
+   PHI_ARG_LOC is the location of the PHI argument if T and VAR are one,
+   or UNKNOWN_LOCATION otherwise.  */
 
 static void
-warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
-	     const char *gmsgid, void *data, location_t phiarg_loc)
+warn_uninit (opt_code opt, tree t, tree var, const char *gmsgid,
+	     gimple *context, location_t phi_arg_loc = UNKNOWN_LOCATION)
 {
-  gimple *context = (gimple *) data;
-  location_t location, cfun_loc;
-  expanded_location xloc, floc;
+  /* Bail if the value isn't provably uninitialized.  */
+  if (!has_undefined_value_p (t))
+    return;
 
   /* Ignore COMPLEX_EXPR as initializing only a part of a complex
      turns in a COMPLEX_EXPR with the not initialized part being
@@ -152,65 +151,63 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
   if (is_gimple_assign (context)
       && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
     return;
-  if (!has_undefined_value_p (t))
-    return;
-
   /* Anonymous SSA_NAMEs shouldn't be uninitialized, but ssa_undefined_value_p
-     can return true if the def stmt of anonymous SSA_NAME is COMPLEX_EXPR
+     can return true if the def stmt of an anonymous SSA_NAME is COMPLEX_EXPR
      created for conversion from scalar to complex.  Use the underlying var of
      the COMPLEX_EXPRs real part in that case.  See PR71581.  */
-  if (expr == NULL_TREE
-      && var == NULL_TREE
-      && SSA_NAME_VAR (t) == NULL_TREE
-      && is_gimple_assign (SSA_NAME_DEF_STMT (t))
-      && gimple_assign_rhs_code (SSA_NAME_DEF_STMT (t)) == COMPLEX_EXPR)
-    {
-      tree v = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (t));
-      if (TREE_CODE (v) == SSA_NAME
-	  && has_undefined_value_p (v)
-	  && zerop (gimple_assign_rhs2 (SSA_NAME_DEF_STMT (t))))
+  if (!var && !SSA_NAME_VAR (t))
+    {
+      gimple *def_stmt = SSA_NAME_DEF_STMT (t);
+      if (is_gimple_assign (def_stmt)
+	  && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
 	{
-	  expr = SSA_NAME_VAR (v);
-	  var = expr;
+	  tree v = gimple_assign_rhs1 (def_stmt);
+	  if (TREE_CODE (v) == SSA_NAME
+	      && has_undefined_value_p (v)
+	      && zerop (gimple_assign_rhs2 (def_stmt)))
+	    var = SSA_NAME_VAR (v);
 	}
     }
 
-  if (expr == NULL_TREE)
+  if (var == NULL_TREE)
     return;
 
-  /* TREE_NO_WARNING either means we already warned, or the front end
-     wishes to suppress the warning.  */
-  if ((context
-       && (warning_suppressed_p (context, OPT_Wuninitialized)
-	   || (gimple_assign_single_p (context)
-	       && get_no_uninit_warning (gimple_assign_rhs1 (context)))))
-      || get_no_uninit_warning (expr))
+  /* Avoid warning if we've already done so or if the warning has been
+     suppressed.  */
+  if (((warning_suppressed_p (context, OPT_Wuninitialized)
+	|| (gimple_assign_single_p (context)
+	    && get_no_uninit_warning (gimple_assign_rhs1 (context)))))
+      || get_no_uninit_warning (var))
     return;
 
-  if (context != NULL && gimple_has_location (context))
-    location = gimple_location (context);
-  else if (phiarg_loc != UNKNOWN_LOCATION)
-    location = phiarg_loc;
-  else
-    location = DECL_SOURCE_LOCATION (var);
+  /* Use either the location of the read statement or that of the PHI
+     argument, or that of the uninitialized variable, in that order,
+     whichever is valid.  */
+  location_t location = gimple_location (context);
+  if (location == UNKNOWN_LOCATION)
+    {
+      if (phi_arg_loc == UNKNOWN_LOCATION)
+	location = DECL_SOURCE_LOCATION (var);
+      else
+	location = phi_arg_loc;
+    }
   location = linemap_resolve_location (line_table, location,
 				       LRK_SPELLING_LOCATION, NULL);
-  cfun_loc = DECL_SOURCE_LOCATION (cfun->decl);
-  xloc = expand_location (location);
-  floc = expand_location (cfun_loc);
+
   auto_diagnostic_group d;
-  if (warning_at (location, wc, gmsgid, expr))
-    {
-      suppress_warning (expr, wc);
+  if (!warning_at (location, opt, gmsgid, var))
+    return;
 
-      if (location == DECL_SOURCE_LOCATION (var))
-	return;
-      if (xloc.file != floc.file
-	  || linemap_location_before_p (line_table, location, cfun_loc)
-	  || linemap_location_before_p (line_table, cfun->function_end_locus,
-					location))
-	inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", var);
-    }
+  /* Avoid subsequent warnings for reads of the same variable again.  */
+  suppress_warning (var, opt);
+
+  /* Issue a note pointing to the read variable unless the warning
+     is at the same location.  */
+  location_t var_loc = DECL_SOURCE_LOCATION (var);
+  if (location == var_loc)
+    return;
+
+  inform (var_loc, "%qD was declared here", var);
 }
 
 struct check_defs_data
@@ -845,13 +842,14 @@ warn_uninit_phi_uses (basic_block bb)
 	}
       if (use_stmt)
 	warn_uninit (OPT_Wuninitialized, def, SSA_NAME_VAR (def),
-		     SSA_NAME_VAR (def),
-		     "%qD is used uninitialized", use_stmt,
-		     UNKNOWN_LOCATION);
+		     "%qD is used uninitialized", use_stmt);
     }
 }
 
-static unsigned int
+/* Issue warnings about reads of uninitialized variables.  WMAYBE_UNINIT
+   is true to issue -Wmaybe-uninitialized, otherwise -Wuninitialized.  */
+
+static void
 warn_uninitialized_vars (bool wmaybe_uninit)
 {
   /* Counters and limits controlling the the depth of the warning.  */
@@ -871,15 +869,13 @@ warn_uninitialized_vars (bool wmaybe_uninit)
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  gimple *stmt = gsi_stmt (gsi);
-	  use_operand_p use_p;
-	  ssa_op_iter op_iter;
-	  tree use;
-
 	  if (is_gimple_debug (stmt))
 	    continue;
 
 	  /* We only do data flow with SSA_NAMEs, so that's all we
 	     can warn about.  */
+	  use_operand_p use_p;
+	  ssa_op_iter op_iter;
 	  FOR_EACH_SSA_USE_OPERAND (use_p, stmt, op_iter, SSA_OP_USE)
 	    {
 	      /* BIT_INSERT_EXPR first operand should not be considered
@@ -890,17 +886,13 @@ warn_uninitialized_vars (bool wmaybe_uninit)
 		      && use_p->use == gimple_assign_rhs1_ptr (ass))
 		    continue;
 		}
-	      use = USE_FROM_PTR (use_p);
+	      tree use = USE_FROM_PTR (use_p);
 	      if (wlims.always_executed)
 		warn_uninit (OPT_Wuninitialized, use, SSA_NAME_VAR (use),
-			     SSA_NAME_VAR (use),
-			     "%qD is used uninitialized", stmt,
-			     UNKNOWN_LOCATION);
+			     "%qD is used uninitialized", stmt);
 	      else if (wmaybe_uninit)
 		warn_uninit (OPT_Wmaybe_uninitialized, use, SSA_NAME_VAR (use),
-			     SSA_NAME_VAR (use),
-			     "%qD may be used uninitialized",
-			     stmt, UNKNOWN_LOCATION);
+			     "%qD may be used uninitialized", stmt);
 	    }
 
 	  /* For limiting the alias walk below we count all
@@ -930,8 +922,6 @@ warn_uninitialized_vars (bool wmaybe_uninit)
 	    }
 	}
     }
-
-  return 0;
 }
 
 /* Checks if the operand OPND of PHI is defined by
@@ -3193,18 +3183,11 @@ static void
 warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
 			hash_set<gphi *> *added_to_worklist)
 {
-  unsigned uninit_opnds;
-  gimple *uninit_use_stmt = 0;
-  tree uninit_op;
-  int phiarg_index;
-  location_t loc;
-
   /* Don't look at virtual operands.  */
   if (virtual_operand_p (gimple_phi_result (phi)))
     return;
 
-  uninit_opnds = compute_uninit_opnds_pos (phi);
-
+  unsigned uninit_opnds = compute_uninit_opnds_pos (phi);
   if (MASK_EMPTY (uninit_opnds))
     return;
 
@@ -3215,25 +3198,23 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
     }
 
   /* Now check if we have any use of the value without proper guard.  */
-  uninit_use_stmt = find_uninit_use (phi, uninit_opnds,
-				     worklist, added_to_worklist);
+  gimple *uninit_use_stmt = find_uninit_use (phi, uninit_opnds,
+					     worklist, added_to_worklist);
 
   /* All uses are properly guarded.  */
   if (!uninit_use_stmt)
     return;
 
-  phiarg_index = MASK_FIRST_SET_BIT (uninit_opnds);
-  uninit_op = gimple_phi_arg_def (phi, phiarg_index);
+  int phiarg_index = MASK_FIRST_SET_BIT (uninit_opnds);
+  tree uninit_op = gimple_phi_arg_def (phi, phiarg_index);
   if (SSA_NAME_VAR (uninit_op) == NULL_TREE)
     return;
-  if (gimple_phi_arg_has_location (phi, phiarg_index))
-    loc = gimple_phi_arg_location (phi, phiarg_index);
-  else
-    loc = UNKNOWN_LOCATION;
-  warn_uninit (OPT_Wmaybe_uninitialized, uninit_op, SSA_NAME_VAR (uninit_op),
+
+  location_t phi_arg_loc = gimple_phi_arg_location (phi, phiarg_index);
+  warn_uninit (OPT_Wmaybe_uninitialized, uninit_op,
 	       SSA_NAME_VAR (uninit_op),
 	       "%qD may be used uninitialized in this function",
-	       uninit_use_stmt, loc);
+	       uninit_use_stmt, phi_arg_loc);
 }
 
 static bool

Re: [PATCH] improve note location and refactor warn_uninit

[Jeff Law]

On 8/25/2021 2:03 PM, Martin Sebor via Gcc-patches wrote:
> Richard, some time ago you mentioned you'd had trouble getting
> -Wuninitialized to print the note pointing to the uninitialized
> variable.  I said I'd look into it, and so I did.  The attached
> patch simplifies the warn_uninit() function to get rid of some
> redundant cruft: besides a few pointless null tests and
> a duplicate argument it also does away with ancient code that's
> responsible for the problem you saw.  It turns out that tests
> for the problem are already in the test suite but have been
> xfailed since the day they were added, so the patch simply
> removes the xfails.  I'll go ahead and commit this cleanup
> as obvious later this week unless you have suggestions for
> further changes.
>
> Tested on x86_64-linux.
I'm going to object to installing this "as obvious".  I don't see how 
anyone could come to the conclusion this is obvious.

Please wait for review.

jeff

Re: [PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

On 8/25/21 5:00 PM, Jeff Law wrote:
> 
> 
> On 8/25/2021 2:03 PM, Martin Sebor via Gcc-patches wrote:
>> Richard, some time ago you mentioned you'd had trouble getting
>> -Wuninitialized to print the note pointing to the uninitialized
>> variable.  I said I'd look into it, and so I did.  The attached
>> patch simplifies the warn_uninit() function to get rid of some
>> redundant cruft: besides a few pointless null tests and
>> a duplicate argument it also does away with ancient code that's
>> responsible for the problem you saw.  It turns out that tests
>> for the problem are already in the test suite but have been
>> xfailed since the day they were added, so the patch simply
>> removes the xfails.  I'll go ahead and commit this cleanup
>> as obvious later this week unless you have suggestions for
>> further changes.
>>
>> Tested on x86_64-linux.
> I'm going to object to installing this "as obvious".  I don't see how 
> anyone could come to the conclusion this is obvious.

As I explained, the patch just a) removes unnecessary and/or
unused code, b) moves declarations closer to their point of
initialization, and c) gets rid of the linemap_location_before_p()
tests that prevent the note that Richard was looking for from being
printed.  It has no effect besides that and it seems rather trivial
to me.  But I don't want to reopen the whole "obviousness debate"
again.

> 
> Please wait for review.

Fine.

Martin

Re: [PATCH] improve note location and refactor warn_uninit

[Richard Biener]

On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>
> Richard, some time ago you mentioned you'd had trouble getting
> -Wuninitialized to print the note pointing to the uninitialized
> variable.  I said I'd look into it, and so I did.  The attached
> patch simplifies the warn_uninit() function to get rid of some
> redundant cruft: besides a few pointless null tests and
> a duplicate argument it also does away with ancient code that's
> responsible for the problem you saw.  It turns out that tests
> for the problem are already in the test suite but have been
> xfailed since the day they were added, so the patch simply
> removes the xfails.  I'll go ahead and commit this cleanup
> as obvious later this week unless you have suggestions for
> further changes.

I do see lots of useful refactoring.

-  if (context != NULL && gimple_has_location (context))
-    location = gimple_location (context);
-  else if (phiarg_loc != UNKNOWN_LOCATION)
-    location = phiarg_loc;
-  else
-    location = DECL_SOURCE_LOCATION (var);
+  /* Use either the location of the read statement or that of the PHI
+     argument, or that of the uninitialized variable, in that order,
+     whichever is valid.  */
+  location_t location = gimple_location (context);
+  if (location == UNKNOWN_LOCATION)
+    {
+      if (phi_arg_loc == UNKNOWN_LOCATION)
+       location = DECL_SOURCE_LOCATION (var);
+      else
+       location = phi_arg_loc;
+    }

the comment is an improvement but I think the original flow is
easier to follow ;)

-      if (xloc.file != floc.file
-         || linemap_location_before_p (line_table, location, cfun_loc)
-         || linemap_location_before_p (line_table, cfun->function_end_locus,
-                                       location))
-       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", var);
...
+  inform (var_loc, "%qD was declared here", var);

and this is the actual change that "fixes" the missing diagnostics?  Can
you explain what the reason for the original sing-and-dance was?  It
looks like it tries to print the 'was declared here' only for locations
within the current function (but then it probably intended to do that
on the DECL_SOURCE_LOCATION (var), not on whatever we are
choosing now)?

That said, I'd prefer if you can commit the refactoring independently
of this core change and can try to dig why this was added and what
it was supposed to do.

Thanks,
Richard.

> Tested on x86_64-linux.
>
> Martin

Re: [PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

On 8/26/21 1:00 AM, Richard Biener wrote:
> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> Richard, some time ago you mentioned you'd had trouble getting
>> -Wuninitialized to print the note pointing to the uninitialized
>> variable.  I said I'd look into it, and so I did.  The attached
>> patch simplifies the warn_uninit() function to get rid of some
>> redundant cruft: besides a few pointless null tests and
>> a duplicate argument it also does away with ancient code that's
>> responsible for the problem you saw.  It turns out that tests
>> for the problem are already in the test suite but have been
>> xfailed since the day they were added, so the patch simply
>> removes the xfails.  I'll go ahead and commit this cleanup
>> as obvious later this week unless you have suggestions for
>> further changes.
> 
> I do see lots of useful refactoring.
> 
> -  if (context != NULL && gimple_has_location (context))
> -    location = gimple_location (context);
> -  else if (phiarg_loc != UNKNOWN_LOCATION)
> -    location = phiarg_loc;
> -  else
> -    location = DECL_SOURCE_LOCATION (var);
> +  /* Use either the location of the read statement or that of the PHI
> +     argument, or that of the uninitialized variable, in that order,
> +     whichever is valid.  */
> +  location_t location = gimple_location (context);
> +  if (location == UNKNOWN_LOCATION)
> +    {
> +      if (phi_arg_loc == UNKNOWN_LOCATION)
> +       location = DECL_SOURCE_LOCATION (var);
> +      else
> +       location = phi_arg_loc;
> +    }
> 
> the comment is an improvement but I think the original flow is
> easier to follow ;)

It doesn't really simplify things so I can revert it.

> 
> -      if (xloc.file != floc.file
> -         || linemap_location_before_p (line_table, location, cfun_loc)
> -         || linemap_location_before_p (line_table, cfun->function_end_locus,
> -                                       location))
> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", var);
> ...
> +  inform (var_loc, "%qD was declared here", var);
> 
> and this is the actual change that "fixes" the missing diagnostics?  Can
> you explain what the reason for the original sing-and-dance was?  It
> looks like it tries to print the 'was declared here' only for locations
> within the current function (but then it probably intended to do that
> on the DECL_SOURCE_LOCATION (var), not on whatever we are
> choosing now)?

The author(s) of the logic wanted to print the note only for variables
declared in functions other than the one the uninitialized read is in.
The idea first appears in pr17506, comment #25 (and attachment 12131).
At that time GCC would print no note at all and pr17506 was about
inlining making it difficult to find the uninitialized variable.
The originally reported problem can still be reproduced on Godbolt
(with the original very large translation unit):
   https://godbolt.org/z/8WW43rxnd

I've reduced it to a small test case:
   https://godbolt.org/z/rnPEfPqPf

It looks like they didn't think it would also be a problem if
the variable was defined and read in the same function, even
if the read was far away from the declaration.

> 
> That said, I'd prefer if you can commit the refactoring independently
> of this core change and can try to dig why this was added and what
> it was supposed to do.

Sure, let me do that.  Please let me know if the fix for the note
is okay to commit as is on its own.

Martin

> 
> Thanks,
> Richard.
> 
>> Tested on x86_64-linux.
>>
>> Martin

Re: [PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

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

On 8/26/21 10:38 AM, Martin Sebor wrote:
> On 8/26/21 1:00 AM, Richard Biener wrote:
>> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> Richard, some time ago you mentioned you'd had trouble getting
>>> -Wuninitialized to print the note pointing to the uninitialized
>>> variable.  I said I'd look into it, and so I did.  The attached
>>> patch simplifies the warn_uninit() function to get rid of some
>>> redundant cruft: besides a few pointless null tests and
>>> a duplicate argument it also does away with ancient code that's
>>> responsible for the problem you saw.  It turns out that tests
>>> for the problem are already in the test suite but have been
>>> xfailed since the day they were added, so the patch simply
>>> removes the xfails.  I'll go ahead and commit this cleanup
>>> as obvious later this week unless you have suggestions for
>>> further changes.
>>
>> I do see lots of useful refactoring.
>>
>> -  if (context != NULL && gimple_has_location (context))
>> -    location = gimple_location (context);
>> -  else if (phiarg_loc != UNKNOWN_LOCATION)
>> -    location = phiarg_loc;
>> -  else
>> -    location = DECL_SOURCE_LOCATION (var);
>> +  /* Use either the location of the read statement or that of the PHI
>> +     argument, or that of the uninitialized variable, in that order,
>> +     whichever is valid.  */
>> +  location_t location = gimple_location (context);
>> +  if (location == UNKNOWN_LOCATION)
>> +    {
>> +      if (phi_arg_loc == UNKNOWN_LOCATION)
>> +       location = DECL_SOURCE_LOCATION (var);
>> +      else
>> +       location = phi_arg_loc;
>> +    }
>>
>> the comment is an improvement but I think the original flow is
>> easier to follow ;)
> 
> It doesn't really simplify things so I can revert it.

I've done that and pushed r12-3165, and...

>> -      if (xloc.file != floc.file
>> -         || linemap_location_before_p (line_table, location, cfun_loc)
>> -         || linemap_location_before_p (line_table, 
>> cfun->function_end_locus,
>> -                                       location))
>> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", 
>> var);
>> ...
>> +  inform (var_loc, "%qD was declared here", var);
>>
>> and this is the actual change that "fixes" the missing diagnostics?  Can
>> you explain what the reason for the original sing-and-dance was?  It
>> looks like it tries to print the 'was declared here' only for locations
>> within the current function (but then it probably intended to do that
>> on the DECL_SOURCE_LOCATION (var), not on whatever we are
>> choosing now)?
> 
> The author(s) of the logic wanted to print the note only for variables
> declared in functions other than the one the uninitialized read is in.
> The idea first appears in pr17506, comment #25 (and attachment 12131).
> At that time GCC would print no note at all and pr17506 was about
> inlining making it difficult to find the uninitialized variable.
> The originally reported problem can still be reproduced on Godbolt
> (with the original very large translation unit):
>    https://godbolt.org/z/8WW43rxnd
> 
> I've reduced it to a small test case:
>    https://godbolt.org/z/rnPEfPqPf
> 
> It looks like they didn't think it would also be a problem if
> the variable was defined and read in the same function, even
> if the read was far away from the declaration.
> 
>>
>> That said, I'd prefer if you can commit the refactoring independently
>> of this core change and can try to dig why this was added and what
>> it was supposed to do.
> 
> Sure, let me do that.  Please let me know if the fix for the note
> is okay to commit as is on its own.

...the removal of the test guarding the note is attached.

> 
> Martin
> 
>>
>> Thanks,
>> Richard.
>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
> 


[-- Attachment #2: gcc-17506.diff --]
[-- Type: text/x-patch, Size: 3536 bytes --]

Improve note location.

Related:
PR tree-optimization/17506 - warning about uninitialized variable points to wrong location
PR testsuite/37182 - Revision 139286 caused gcc.dg/pr17506.c and gcc.dg/uninit-15.c

gcc/ChangeLog:

	PR tree-optimization/17506
	PR testsuite/37182
	* tree-ssa-uninit.c (warn_uninit): Remove conditional guarding note.

gcc/testsuite/ChangeLog:

	PR tree-optimization/17506
	PR testsuite/37182
	* gcc.dg/diagnostic-tree-expr-ranges-2.c: Add expected output.
	* gcc.dg/uninit-15-O0.c: Remove xfail.
	* gcc.dg/uninit-15.c: Same.

diff --git a/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c b/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c
index 302e233a04a..1cbcad5ac7d 100644
--- a/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c
+++ b/gcc/testsuite/gcc.dg/diagnostic-tree-expr-ranges-2.c
@@ -3,21 +3,25 @@
 
 int test_uninit_1 (void)
 {
-  int result;
-  return result;  /* { dg-warning "uninitialized" } */
-/* { dg-begin-multiline-output "" }
-   return result;
-          ^~~~~~
+  int result_1;     /* { dg-message "declared here" } */
+  return result_1;  /* { dg-warning "uninitialized" } */
+  /* { dg-begin-multiline-output "" }
+   return result_1;
+          ^~~~~~~~
+   int result_1;
+       ^~~~~~~~
    { dg-end-multiline-output "" } */
 }
 
 int test_uninit_2 (void)
 {
-  int result;
-  result += 3; /* { dg-warning "uninitialized" } */
-/* { dg-begin-multiline-output "" }
-   result += 3;
-   ~~~~~~~^~~~
+  int result_2;     /* { dg-message "declared here" } */
+  result_2 += 3;    /* { dg-warning "uninitialized" } */
+  /* { dg-begin-multiline-output "" }
+   result_2 += 3;
+   ~~~~~~~~~^~~~
+   int result_2;
+       ^~~~~~~~
    { dg-end-multiline-output "" } */
-  return result;
+  return result_2;
 }
diff --git a/gcc/testsuite/gcc.dg/uninit-15-O0.c b/gcc/testsuite/gcc.dg/uninit-15-O0.c
index 36d96348617..1ddddee1388 100644
--- a/gcc/testsuite/gcc.dg/uninit-15-O0.c
+++ b/gcc/testsuite/gcc.dg/uninit-15-O0.c
@@ -14,7 +14,7 @@ void baz();
 
 void bar()
 {
-    int j;           /* { dg-message "was declared here" {} { xfail *-*-* } } */
+    int j;              /* { dg-message "declared here" } */
     for (; foo(j); ++j) /* { dg-warning "is used uninitialized" } */
         baz();
 }
diff --git a/gcc/testsuite/gcc.dg/uninit-15.c b/gcc/testsuite/gcc.dg/uninit-15.c
index 85cb116f349..7352662f627 100644
--- a/gcc/testsuite/gcc.dg/uninit-15.c
+++ b/gcc/testsuite/gcc.dg/uninit-15.c
@@ -20,7 +20,7 @@ void baz (void);
 void
 bar (void)
 {
-  int j; /* { dg-message "note: 'j' was declared here" "" { xfail *-*-* } } */
+  int j;                /* { dg-message "note: 'j' was declared here" } */
   for (; foo (j); ++j)  /* { dg-warning "'j' is used uninitialized" } */
     baz ();
 }
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 394dbf40c9c..cb6d1145202 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -206,14 +206,7 @@ warn_uninit (opt_code opt, tree t, tree var, const char *gmsgid,
   if (location == var_loc)
     return;
 
-  location_t cfun_loc = DECL_SOURCE_LOCATION (cfun->decl);
-  expanded_location xloc = expand_location (location);
-  expanded_location floc = expand_location (cfun_loc);
-  if (xloc.file != floc.file
-      || linemap_location_before_p (line_table, location, cfun_loc)
-      || linemap_location_before_p (line_table, cfun->function_end_locus,
-					location))
-    inform (var_loc, "%qD was declared here", var);
+  inform (var_loc, "%qD was declared here", var);
 }
 
 struct check_defs_data

Re: [PATCH] improve note location and refactor warn_uninit

[Richard Biener]

On Thu, Aug 26, 2021 at 9:30 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 8/26/21 10:38 AM, Martin Sebor wrote:
> > On 8/26/21 1:00 AM, Richard Biener wrote:
> >> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
> >>>
> >>> Richard, some time ago you mentioned you'd had trouble getting
> >>> -Wuninitialized to print the note pointing to the uninitialized
> >>> variable.  I said I'd look into it, and so I did.  The attached
> >>> patch simplifies the warn_uninit() function to get rid of some
> >>> redundant cruft: besides a few pointless null tests and
> >>> a duplicate argument it also does away with ancient code that's
> >>> responsible for the problem you saw.  It turns out that tests
> >>> for the problem are already in the test suite but have been
> >>> xfailed since the day they were added, so the patch simply
> >>> removes the xfails.  I'll go ahead and commit this cleanup
> >>> as obvious later this week unless you have suggestions for
> >>> further changes.
> >>
> >> I do see lots of useful refactoring.
> >>
> >> -  if (context != NULL && gimple_has_location (context))
> >> -    location = gimple_location (context);
> >> -  else if (phiarg_loc != UNKNOWN_LOCATION)
> >> -    location = phiarg_loc;
> >> -  else
> >> -    location = DECL_SOURCE_LOCATION (var);
> >> +  /* Use either the location of the read statement or that of the PHI
> >> +     argument, or that of the uninitialized variable, in that order,
> >> +     whichever is valid.  */
> >> +  location_t location = gimple_location (context);
> >> +  if (location == UNKNOWN_LOCATION)
> >> +    {
> >> +      if (phi_arg_loc == UNKNOWN_LOCATION)
> >> +       location = DECL_SOURCE_LOCATION (var);
> >> +      else
> >> +       location = phi_arg_loc;
> >> +    }
> >>
> >> the comment is an improvement but I think the original flow is
> >> easier to follow ;)
> >
> > It doesn't really simplify things so I can revert it.
>
> I've done that and pushed r12-3165, and...
>
> >> -      if (xloc.file != floc.file
> >> -         || linemap_location_before_p (line_table, location, cfun_loc)
> >> -         || linemap_location_before_p (line_table,
> >> cfun->function_end_locus,
> >> -                                       location))
> >> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here",
> >> var);
> >> ...
> >> +  inform (var_loc, "%qD was declared here", var);
> >>
> >> and this is the actual change that "fixes" the missing diagnostics?  Can
> >> you explain what the reason for the original sing-and-dance was?  It
> >> looks like it tries to print the 'was declared here' only for locations
> >> within the current function (but then it probably intended to do that
> >> on the DECL_SOURCE_LOCATION (var), not on whatever we are
> >> choosing now)?
> >
> > The author(s) of the logic wanted to print the note only for variables
> > declared in functions other than the one the uninitialized read is in.
> > The idea first appears in pr17506, comment #25 (and attachment 12131).
> > At that time GCC would print no note at all and pr17506 was about
> > inlining making it difficult to find the uninitialized variable.
> > The originally reported problem can still be reproduced on Godbolt
> > (with the original very large translation unit):
> >    https://godbolt.org/z/8WW43rxnd
> >
> > I've reduced it to a small test case:
> >    https://godbolt.org/z/rnPEfPqPf
> >
> > It looks like they didn't think it would also be a problem if
> > the variable was defined and read in the same function, even
> > if the read was far away from the declaration.

Ah, OK.  I think that makes sense in principle, the test just looked
really weird - for the 'decl' it would be most natural to use sth
like decl_function_context (DECL_ORIGIN (var)) to determine
the function the variable was declared in and for the location of
the uninitialized use you'd similarly use

  tree ctx = BLOCK_ORIGIN (LOCATION_BLOCK (location));
  while (TREE_CODE ((ctx = BLOCK_SUPERCONTEXT (ctx))) != FUNCTION_DECL)
     ;

and then you can compare both functions.  This of course requires
a good location of the uninitialized use.

So I'm not sure whether we want to increase verboseness for say

int foo ()
{
   int i;
   i = i + 1;
   return i;
}

by telling the user 'i' was declared the line before the uninitialized use.

But I do think the code deserves a comment, so do you mind adding
one to say what it is about?

Thanks,
Richard.

> >>
> >> That said, I'd prefer if you can commit the refactoring independently
> >> of this core change and can try to dig why this was added and what
> >> it was supposed to do.
> >
> > Sure, let me do that.  Please let me know if the fix for the note
> > is okay to commit as is on its own.
>
> ...the removal of the test guarding the note is attached.
>
> >
> > Martin
> >
> >>
> >> Thanks,
> >> Richard.
> >>
> >>> Tested on x86_64-linux.
> >>>
> >>> Martin
> >
>

Re: [PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

On 8/27/21 12:23 AM, Richard Biener wrote:
> On Thu, Aug 26, 2021 at 9:30 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 8/26/21 10:38 AM, Martin Sebor wrote:
>>> On 8/26/21 1:00 AM, Richard Biener wrote:
>>>> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>
>>>>> Richard, some time ago you mentioned you'd had trouble getting
>>>>> -Wuninitialized to print the note pointing to the uninitialized
>>>>> variable.  I said I'd look into it, and so I did.  The attached
>>>>> patch simplifies the warn_uninit() function to get rid of some
>>>>> redundant cruft: besides a few pointless null tests and
>>>>> a duplicate argument it also does away with ancient code that's
>>>>> responsible for the problem you saw.  It turns out that tests
>>>>> for the problem are already in the test suite but have been
>>>>> xfailed since the day they were added, so the patch simply
>>>>> removes the xfails.  I'll go ahead and commit this cleanup
>>>>> as obvious later this week unless you have suggestions for
>>>>> further changes.
>>>>
>>>> I do see lots of useful refactoring.
>>>>
>>>> -  if (context != NULL && gimple_has_location (context))
>>>> -    location = gimple_location (context);
>>>> -  else if (phiarg_loc != UNKNOWN_LOCATION)
>>>> -    location = phiarg_loc;
>>>> -  else
>>>> -    location = DECL_SOURCE_LOCATION (var);
>>>> +  /* Use either the location of the read statement or that of the PHI
>>>> +     argument, or that of the uninitialized variable, in that order,
>>>> +     whichever is valid.  */
>>>> +  location_t location = gimple_location (context);
>>>> +  if (location == UNKNOWN_LOCATION)
>>>> +    {
>>>> +      if (phi_arg_loc == UNKNOWN_LOCATION)
>>>> +       location = DECL_SOURCE_LOCATION (var);
>>>> +      else
>>>> +       location = phi_arg_loc;
>>>> +    }
>>>>
>>>> the comment is an improvement but I think the original flow is
>>>> easier to follow ;)
>>>
>>> It doesn't really simplify things so I can revert it.
>>
>> I've done that and pushed r12-3165, and...
>>
>>>> -      if (xloc.file != floc.file
>>>> -         || linemap_location_before_p (line_table, location, cfun_loc)
>>>> -         || linemap_location_before_p (line_table,
>>>> cfun->function_end_locus,
>>>> -                                       location))
>>>> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here",
>>>> var);
>>>> ...
>>>> +  inform (var_loc, "%qD was declared here", var);
>>>>
>>>> and this is the actual change that "fixes" the missing diagnostics?  Can
>>>> you explain what the reason for the original sing-and-dance was?  It
>>>> looks like it tries to print the 'was declared here' only for locations
>>>> within the current function (but then it probably intended to do that
>>>> on the DECL_SOURCE_LOCATION (var), not on whatever we are
>>>> choosing now)?
>>>
>>> The author(s) of the logic wanted to print the note only for variables
>>> declared in functions other than the one the uninitialized read is in.
>>> The idea first appears in pr17506, comment #25 (and attachment 12131).
>>> At that time GCC would print no note at all and pr17506 was about
>>> inlining making it difficult to find the uninitialized variable.
>>> The originally reported problem can still be reproduced on Godbolt
>>> (with the original very large translation unit):
>>>     https://godbolt.org/z/8WW43rxnd
>>>
>>> I've reduced it to a small test case:
>>>     https://godbolt.org/z/rnPEfPqPf
>>>
>>> It looks like they didn't think it would also be a problem if
>>> the variable was defined and read in the same function, even
>>> if the read was far away from the declaration.
> 
> Ah, OK.  I think that makes sense in principle, the test just looked
> really weird - for the 'decl' it would be most natural to use sth
> like decl_function_context (DECL_ORIGIN (var)) to determine
> the function the variable was declared in and for the location of
> the uninitialized use you'd similarly use
> 
>    tree ctx = BLOCK_ORIGIN (LOCATION_BLOCK (location));
>    while (TREE_CODE ((ctx = BLOCK_SUPERCONTEXT (ctx))) != FUNCTION_DECL)
>       ;
> 
> and then you can compare both functions.  This of course requires
> a good location of the uninitialized use.
> 
> So I'm not sure whether we want to increase verboseness for say
> 
> int foo ()
> {
>     int i;
>     i = i + 1;
>     return i;
> }
> 
> by telling the user 'i' was declared the line before the uninitialized use.

In this contrived case the note may not be important but it doesn't
hurt.  It's the more realistic cases of large functions with problem
statements far away from the objects they operate on, often indirectly,
via pointers, where providing the additional context is helpful.

This is also why most other middle end warnings (all those I worked
on) as well as other instances of -Wmaybe-uninitialized issue these
(and other) notes to provide enough detail to understand the problem.
The one we're discussion is an outlier.  For example this code:

void f (const int*, ...);

void g (int i)
{
   int a[4], *p = a + i;
   f (p, p[4]);
}

results in two warnings, each with its own note(s):

z.c: In function ‘g’:
z.c:6:3: warning: ‘a’ is used uninitialized [-Wuninitialized]
     6 |   f (p, p[4]);
       |   ^~~~~~~~~~~
z.c:5:7: note: ‘a’ declared here
     5 |   int a[4], *p = a + i;
       |       ^
z.c:6:3: warning: array subscript 4 is outside array bounds of ‘int[4]’ 
[-Warray-bounds]
     6 |   f (p, p[4]);
       |   ^~~~~~~~~~~
z.c:5:7: note: at offset 16 into object ‘a’ of size 16
     5 |   int a[4], *p = a + i;
       |       ^

Sure, it might seem like overkill for such a contrived example, but
again, it's the real-world code with functions dozens or hundreds
of lines long that I'm trying to help.

> 
> But I do think the code deserves a comment, so do you mind adding
> one to say what it is about?

If you agree with the trend above in general, is the patch okay to
commit as is?

Martin

> 
> Thanks,
> Richard.
> 
>>>>
>>>> That said, I'd prefer if you can commit the refactoring independently
>>>> of this core change and can try to dig why this was added and what
>>>> it was supposed to do.
>>>
>>> Sure, let me do that.  Please let me know if the fix for the note
>>> is okay to commit as is on its own.
>>
>> ...the removal of the test guarding the note is attached.
>>
>>>
>>> Martin
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Tested on x86_64-linux.
>>>>>
>>>>> Martin
>>>
>>

Re: [PATCH] improve note location and refactor warn_uninit

[Richard Biener]

On August 27, 2021 5:20:57 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
>On 8/27/21 12:23 AM, Richard Biener wrote:
>> On Thu, Aug 26, 2021 at 9:30 PM Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 8/26/21 10:38 AM, Martin Sebor wrote:
>>>> On 8/26/21 1:00 AM, Richard Biener wrote:
>>>>> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>>
>>>>>> Richard, some time ago you mentioned you'd had trouble getting
>>>>>> -Wuninitialized to print the note pointing to the uninitialized
>>>>>> variable.  I said I'd look into it, and so I did.  The attached
>>>>>> patch simplifies the warn_uninit() function to get rid of some
>>>>>> redundant cruft: besides a few pointless null tests and
>>>>>> a duplicate argument it also does away with ancient code that's
>>>>>> responsible for the problem you saw.  It turns out that tests
>>>>>> for the problem are already in the test suite but have been
>>>>>> xfailed since the day they were added, so the patch simply
>>>>>> removes the xfails.  I'll go ahead and commit this cleanup
>>>>>> as obvious later this week unless you have suggestions for
>>>>>> further changes.
>>>>>
>>>>> I do see lots of useful refactoring.
>>>>>
>>>>> -  if (context != NULL && gimple_has_location (context))
>>>>> -    location = gimple_location (context);
>>>>> -  else if (phiarg_loc != UNKNOWN_LOCATION)
>>>>> -    location = phiarg_loc;
>>>>> -  else
>>>>> -    location = DECL_SOURCE_LOCATION (var);
>>>>> +  /* Use either the location of the read statement or that of the PHI
>>>>> +     argument, or that of the uninitialized variable, in that order,
>>>>> +     whichever is valid.  */
>>>>> +  location_t location = gimple_location (context);
>>>>> +  if (location == UNKNOWN_LOCATION)
>>>>> +    {
>>>>> +      if (phi_arg_loc == UNKNOWN_LOCATION)
>>>>> +       location = DECL_SOURCE_LOCATION (var);
>>>>> +      else
>>>>> +       location = phi_arg_loc;
>>>>> +    }
>>>>>
>>>>> the comment is an improvement but I think the original flow is
>>>>> easier to follow ;)
>>>>
>>>> It doesn't really simplify things so I can revert it.
>>>
>>> I've done that and pushed r12-3165, and...
>>>
>>>>> -      if (xloc.file != floc.file
>>>>> -         || linemap_location_before_p (line_table, location, cfun_loc)
>>>>> -         || linemap_location_before_p (line_table,
>>>>> cfun->function_end_locus,
>>>>> -                                       location))
>>>>> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here",
>>>>> var);
>>>>> ...
>>>>> +  inform (var_loc, "%qD was declared here", var);
>>>>>
>>>>> and this is the actual change that "fixes" the missing diagnostics?  Can
>>>>> you explain what the reason for the original sing-and-dance was?  It
>>>>> looks like it tries to print the 'was declared here' only for locations
>>>>> within the current function (but then it probably intended to do that
>>>>> on the DECL_SOURCE_LOCATION (var), not on whatever we are
>>>>> choosing now)?
>>>>
>>>> The author(s) of the logic wanted to print the note only for variables
>>>> declared in functions other than the one the uninitialized read is in.
>>>> The idea first appears in pr17506, comment #25 (and attachment 12131).
>>>> At that time GCC would print no note at all and pr17506 was about
>>>> inlining making it difficult to find the uninitialized variable.
>>>> The originally reported problem can still be reproduced on Godbolt
>>>> (with the original very large translation unit):
>>>>     https://godbolt.org/z/8WW43rxnd
>>>>
>>>> I've reduced it to a small test case:
>>>>     https://godbolt.org/z/rnPEfPqPf
>>>>
>>>> It looks like they didn't think it would also be a problem if
>>>> the variable was defined and read in the same function, even
>>>> if the read was far away from the declaration.
>> 
>> Ah, OK.  I think that makes sense in principle, the test just looked
>> really weird - for the 'decl' it would be most natural to use sth
>> like decl_function_context (DECL_ORIGIN (var)) to determine
>> the function the variable was declared in and for the location of
>> the uninitialized use you'd similarly use
>> 
>>    tree ctx = BLOCK_ORIGIN (LOCATION_BLOCK (location));
>>    while (TREE_CODE ((ctx = BLOCK_SUPERCONTEXT (ctx))) != FUNCTION_DECL)
>>       ;
>> 
>> and then you can compare both functions.  This of course requires
>> a good location of the uninitialized use.
>> 
>> So I'm not sure whether we want to increase verboseness for say
>> 
>> int foo ()
>> {
>>     int i;
>>     i = i + 1;
>>     return i;
>> }
>> 
>> by telling the user 'i' was declared the line before the uninitialized use.
>
>In this contrived case the note may not be important but it doesn't
>hurt.  It's the more realistic cases of large functions with problem
>statements far away from the objects they operate on, often indirectly,
>via pointers, where providing the additional context is helpful.
>
>This is also why most other middle end warnings (all those I worked
>on) as well as other instances of -Wmaybe-uninitialized issue these
>(and other) notes to provide enough detail to understand the problem.
>The one we're discussion is an outlier.  For example this code:
>
>void f (const int*, ...);
>
>void g (int i)
>{
>   int a[4], *p = a + i;
>   f (p, p[4]);
>}
>
>results in two warnings, each with its own note(s):
>
>z.c: In function ‘g’:
>z.c:6:3: warning: ‘a’ is used uninitialized [-Wuninitialized]
>     6 |   f (p, p[4]);
>       |   ^~~~~~~~~~~
>z.c:5:7: note: ‘a’ declared here
>     5 |   int a[4], *p = a + i;
>       |       ^
>z.c:6:3: warning: array subscript 4 is outside array bounds of ‘int[4]’ 
>[-Warray-bounds]
>     6 |   f (p, p[4]);
>       |   ^~~~~~~~~~~
>z.c:5:7: note: at offset 16 into object ‘a’ of size 16
>     5 |   int a[4], *p = a + i;
>       |       ^
>
>Sure, it might seem like overkill for such a contrived example, but
>again, it's the real-world code with functions dozens or hundreds
>of lines long that I'm trying to help.
>
>> 
>> But I do think the code deserves a comment, so do you mind adding
>> one to say what it is about?
>
>If you agree with the trend above in general, is the patch okay to
>commit as is?

I'd be fine with that but can you ask whoever added this exception? 

Richard. 

>Martin
>
>> 
>> Thanks,
>> Richard.
>> 
>>>>>
>>>>> That said, I'd prefer if you can commit the refactoring independently
>>>>> of this core change and can try to dig why this was added and what
>>>>> it was supposed to do.
>>>>
>>>> Sure, let me do that.  Please let me know if the fix for the note
>>>> is okay to commit as is on its own.
>>>
>>> ...the removal of the test guarding the note is attached.
>>>
>>>>
>>>> Martin
>>>>
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> Tested on x86_64-linux.
>>>>>>
>>>>>> Martin
>>>>
>>>
>

Re: [PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

Hello Nathan & Joern,

Richard has asked me to give you a heads up that we will be enabling
an informational note for all instances of the GCC -Wuninitialized
warning.  You added the helpful note pretty much exactly 15 years ago
to the day (in http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=116564)
but enabled it only to a limited extent.  So this is to let you know
of this change and give you an opportunity to raise any concerns you
might have with it.  Please see the discussion below for more details.

The change is as follows:

diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 394dbf40c9c..cb6d1145202 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -206,14 +206,7 @@ warn_uninit (opt_code opt, tree t, tree var, const 
char *gmsgid,
    if (location == var_loc)
      return;

-  location_t cfun_loc = DECL_SOURCE_LOCATION (cfun->decl);
-  expanded_location xloc = expand_location (location);
-  expanded_location floc = expand_location (cfun_loc);
-  if (xloc.file != floc.file
-      || linemap_location_before_p (line_table, location, cfun_loc)
-      || linemap_location_before_p (line_table, cfun->function_end_locus,
-					location))
-    inform (var_loc, "%qD was declared here", var);
+  inform (var_loc, "%qD was declared here", var);
  }

  struct check_defs_data

Regards
Martin

On 8/27/21 11:30 AM, Richard Biener wrote:
> On August 27, 2021 5:20:57 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
>> On 8/27/21 12:23 AM, Richard Biener wrote:
>>> On Thu, Aug 26, 2021 at 9:30 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> On 8/26/21 10:38 AM, Martin Sebor wrote:
>>>>> On 8/26/21 1:00 AM, Richard Biener wrote:
>>>>>> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>>>
>>>>>>> Richard, some time ago you mentioned you'd had trouble getting
>>>>>>> -Wuninitialized to print the note pointing to the uninitialized
>>>>>>> variable.  I said I'd look into it, and so I did.  The attached
>>>>>>> patch simplifies the warn_uninit() function to get rid of some
>>>>>>> redundant cruft: besides a few pointless null tests and
>>>>>>> a duplicate argument it also does away with ancient code that's
>>>>>>> responsible for the problem you saw.  It turns out that tests
>>>>>>> for the problem are already in the test suite but have been
>>>>>>> xfailed since the day they were added, so the patch simply
>>>>>>> removes the xfails.  I'll go ahead and commit this cleanup
>>>>>>> as obvious later this week unless you have suggestions for
>>>>>>> further changes.
>>>>>>
>>>>>> I do see lots of useful refactoring.
>>>>>>
>>>>>> -  if (context != NULL && gimple_has_location (context))
>>>>>> -    location = gimple_location (context);
>>>>>> -  else if (phiarg_loc != UNKNOWN_LOCATION)
>>>>>> -    location = phiarg_loc;
>>>>>> -  else
>>>>>> -    location = DECL_SOURCE_LOCATION (var);
>>>>>> +  /* Use either the location of the read statement or that of the PHI
>>>>>> +     argument, or that of the uninitialized variable, in that order,
>>>>>> +     whichever is valid.  */
>>>>>> +  location_t location = gimple_location (context);
>>>>>> +  if (location == UNKNOWN_LOCATION)
>>>>>> +    {
>>>>>> +      if (phi_arg_loc == UNKNOWN_LOCATION)
>>>>>> +       location = DECL_SOURCE_LOCATION (var);
>>>>>> +      else
>>>>>> +       location = phi_arg_loc;
>>>>>> +    }
>>>>>>
>>>>>> the comment is an improvement but I think the original flow is
>>>>>> easier to follow ;)
>>>>>
>>>>> It doesn't really simplify things so I can revert it.
>>>>
>>>> I've done that and pushed r12-3165, and...
>>>>
>>>>>> -      if (xloc.file != floc.file
>>>>>> -         || linemap_location_before_p (line_table, location, cfun_loc)
>>>>>> -         || linemap_location_before_p (line_table,
>>>>>> cfun->function_end_locus,
>>>>>> -                                       location))
>>>>>> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here",
>>>>>> var);
>>>>>> ...
>>>>>> +  inform (var_loc, "%qD was declared here", var);
>>>>>>
>>>>>> and this is the actual change that "fixes" the missing diagnostics?  Can
>>>>>> you explain what the reason for the original sing-and-dance was?  It
>>>>>> looks like it tries to print the 'was declared here' only for locations
>>>>>> within the current function (but then it probably intended to do that
>>>>>> on the DECL_SOURCE_LOCATION (var), not on whatever we are
>>>>>> choosing now)?
>>>>>
>>>>> The author(s) of the logic wanted to print the note only for variables
>>>>> declared in functions other than the one the uninitialized read is in.
>>>>> The idea first appears in pr17506, comment #25 (and attachment 12131).
>>>>> At that time GCC would print no note at all and pr17506 was about
>>>>> inlining making it difficult to find the uninitialized variable.
>>>>> The originally reported problem can still be reproduced on Godbolt
>>>>> (with the original very large translation unit):
>>>>>      https://godbolt.org/z/8WW43rxnd
>>>>>
>>>>> I've reduced it to a small test case:
>>>>>      https://godbolt.org/z/rnPEfPqPf
>>>>>
>>>>> It looks like they didn't think it would also be a problem if
>>>>> the variable was defined and read in the same function, even
>>>>> if the read was far away from the declaration.
>>>
>>> Ah, OK.  I think that makes sense in principle, the test just looked
>>> really weird - for the 'decl' it would be most natural to use sth
>>> like decl_function_context (DECL_ORIGIN (var)) to determine
>>> the function the variable was declared in and for the location of
>>> the uninitialized use you'd similarly use
>>>
>>>     tree ctx = BLOCK_ORIGIN (LOCATION_BLOCK (location));
>>>     while (TREE_CODE ((ctx = BLOCK_SUPERCONTEXT (ctx))) != FUNCTION_DECL)
>>>        ;
>>>
>>> and then you can compare both functions.  This of course requires
>>> a good location of the uninitialized use.
>>>
>>> So I'm not sure whether we want to increase verboseness for say
>>>
>>> int foo ()
>>> {
>>>      int i;
>>>      i = i + 1;
>>>      return i;
>>> }
>>>
>>> by telling the user 'i' was declared the line before the uninitialized use.
>>
>> In this contrived case the note may not be important but it doesn't
>> hurt.  It's the more realistic cases of large functions with problem
>> statements far away from the objects they operate on, often indirectly,
>> via pointers, where providing the additional context is helpful.
>>
>> This is also why most other middle end warnings (all those I worked
>> on) as well as other instances of -Wmaybe-uninitialized issue these
>> (and other) notes to provide enough detail to understand the problem.
>> The one we're discussion is an outlier.  For example this code:
>>
>> void f (const int*, ...);
>>
>> void g (int i)
>> {
>>    int a[4], *p = a + i;
>>    f (p, p[4]);
>> }
>>
>> results in two warnings, each with its own note(s):
>>
>> z.c: In function ‘g’:
>> z.c:6:3: warning: ‘a’ is used uninitialized [-Wuninitialized]
>>      6 |   f (p, p[4]);
>>        |   ^~~~~~~~~~~
>> z.c:5:7: note: ‘a’ declared here
>>      5 |   int a[4], *p = a + i;
>>        |       ^
>> z.c:6:3: warning: array subscript 4 is outside array bounds of ‘int[4]’
>> [-Warray-bounds]
>>      6 |   f (p, p[4]);
>>        |   ^~~~~~~~~~~
>> z.c:5:7: note: at offset 16 into object ‘a’ of size 16
>>      5 |   int a[4], *p = a + i;
>>        |       ^
>>
>> Sure, it might seem like overkill for such a contrived example, but
>> again, it's the real-world code with functions dozens or hundreds
>> of lines long that I'm trying to help.
>>
>>>
>>> But I do think the code deserves a comment, so do you mind adding
>>> one to say what it is about?
>>
>> If you agree with the trend above in general, is the patch okay to
>> commit as is?
> 
> I'd be fine with that but can you ask whoever added this exception?
> 
> Richard.
> 
>> Martin
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>>>>
>>>>>> That said, I'd prefer if you can commit the refactoring independently
>>>>>> of this core change and can try to dig why this was added and what
>>>>>> it was supposed to do.
>>>>>
>>>>> Sure, let me do that.  Please let me know if the fix for the note
>>>>> is okay to commit as is on its own.
>>>>
>>>> ...the removal of the test guarding the note is attached.
>>>>
>>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> Tested on x86_64-linux.
>>>>>>>
>>>>>>> Martin
>>>>>
>>>>
>>
> 

Re: [PATCH] improve note location and refactor warn_uninit

[Jeff Law]

On 8/26/2021 1:00 AM, Richard Biener via Gcc-patches wrote:
> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>> Richard, some time ago you mentioned you'd had trouble getting
>> -Wuninitialized to print the note pointing to the uninitialized
>> variable.  I said I'd look into it, and so I did.  The attached
>> patch simplifies the warn_uninit() function to get rid of some
>> redundant cruft: besides a few pointless null tests and
>> a duplicate argument it also does away with ancient code that's
>> responsible for the problem you saw.  It turns out that tests
>> for the problem are already in the test suite but have been
>> xfailed since the day they were added, so the patch simply
>> removes the xfails.  I'll go ahead and commit this cleanup
>> as obvious later this week unless you have suggestions for
>> further changes.
> I do see lots of useful refactoring.
>
> -  if (context != NULL && gimple_has_location (context))
> -    location = gimple_location (context);
> -  else if (phiarg_loc != UNKNOWN_LOCATION)
> -    location = phiarg_loc;
> -  else
> -    location = DECL_SOURCE_LOCATION (var);
> +  /* Use either the location of the read statement or that of the PHI
> +     argument, or that of the uninitialized variable, in that order,
> +     whichever is valid.  */
> +  location_t location = gimple_location (context);
> +  if (location == UNKNOWN_LOCATION)
> +    {
> +      if (phi_arg_loc == UNKNOWN_LOCATION)
> +       location = DECL_SOURCE_LOCATION (var);
> +      else
> +       location = phi_arg_loc;
> +    }
>
> the comment is an improvement but I think the original flow is
> easier to follow ;)
>
> -      if (xloc.file != floc.file
> -         || linemap_location_before_p (line_table, location, cfun_loc)
> -         || linemap_location_before_p (line_table, cfun->function_end_locus,
> -                                       location))
> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", var);
> ...
> +  inform (var_loc, "%qD was declared here", var);
>
> and this is the actual change that "fixes" the missing diagnostics?  Can
> you explain what the reason for the original sing-and-dance was?  It
> looks like it tries to print the 'was declared here' only for locations
> within the current function (but then it probably intended to do that
> on the DECL_SOURCE_LOCATION (var), not on whatever we are
> choosing now)?
>
> That said, I'd prefer if you can commit the refactoring independently
> of this core change and can try to dig why this was added and what
> it was supposed to do.
Thanks.  And just to be 100% clear, all I was objecting to was the 
assertion that the patch was obvious and that it could be committed 
under the obvious rule.
jeff

Re: [PATCH] improve note location and refactor warn_uninit

[Jeff Law]

On 8/26/2021 1:30 PM, Martin Sebor via Gcc-patches wrote:
> On 8/26/21 10:38 AM, Martin Sebor wrote:
>> On 8/26/21 1:00 AM, Richard Biener wrote:
>>> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> Richard, some time ago you mentioned you'd had trouble getting
>>>> -Wuninitialized to print the note pointing to the uninitialized
>>>> variable.  I said I'd look into it, and so I did.  The attached
>>>> patch simplifies the warn_uninit() function to get rid of some
>>>> redundant cruft: besides a few pointless null tests and
>>>> a duplicate argument it also does away with ancient code that's
>>>> responsible for the problem you saw.  It turns out that tests
>>>> for the problem are already in the test suite but have been
>>>> xfailed since the day they were added, so the patch simply
>>>> removes the xfails.  I'll go ahead and commit this cleanup
>>>> as obvious later this week unless you have suggestions for
>>>> further changes.
>>>
>>> I do see lots of useful refactoring.
>>>
>>> -  if (context != NULL && gimple_has_location (context))
>>> -    location = gimple_location (context);
>>> -  else if (phiarg_loc != UNKNOWN_LOCATION)
>>> -    location = phiarg_loc;
>>> -  else
>>> -    location = DECL_SOURCE_LOCATION (var);
>>> +  /* Use either the location of the read statement or that of the PHI
>>> +     argument, or that of the uninitialized variable, in that order,
>>> +     whichever is valid.  */
>>> +  location_t location = gimple_location (context);
>>> +  if (location == UNKNOWN_LOCATION)
>>> +    {
>>> +      if (phi_arg_loc == UNKNOWN_LOCATION)
>>> +       location = DECL_SOURCE_LOCATION (var);
>>> +      else
>>> +       location = phi_arg_loc;
>>> +    }
>>>
>>> the comment is an improvement but I think the original flow is
>>> easier to follow ;)
>>
>> It doesn't really simplify things so I can revert it.
>
> I've done that and pushed r12-3165, and...
>
>>> -      if (xloc.file != floc.file
>>> -         || linemap_location_before_p (line_table, location, cfun_loc)
>>> -         || linemap_location_before_p (line_table, 
>>> cfun->function_end_locus,
>>> -                                       location))
>>> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", 
>>> var);
>>> ...
>>> +  inform (var_loc, "%qD was declared here", var);
>>>
>>> and this is the actual change that "fixes" the missing diagnostics?  
>>> Can
>>> you explain what the reason for the original sing-and-dance was?  It
>>> looks like it tries to print the 'was declared here' only for locations
>>> within the current function (but then it probably intended to do that
>>> on the DECL_SOURCE_LOCATION (var), not on whatever we are
>>> choosing now)?
>>
>> The author(s) of the logic wanted to print the note only for variables
>> declared in functions other than the one the uninitialized read is in.
>> The idea first appears in pr17506, comment #25 (and attachment 12131).
>> At that time GCC would print no note at all and pr17506 was about
>> inlining making it difficult to find the uninitialized variable.
>> The originally reported problem can still be reproduced on Godbolt
>> (with the original very large translation unit):
>>    https://godbolt.org/z/8WW43rxnd
>>
>> I've reduced it to a small test case:
>>    https://godbolt.org/z/rnPEfPqPf
>>
>> It looks like they didn't think it would also be a problem if
>> the variable was defined and read in the same function, even
>> if the read was far away from the declaration.
>>
>>>
>>> That said, I'd prefer if you can commit the refactoring independently
>>> of this core change and can try to dig why this was added and what
>>> it was supposed to do.
>>
>> Sure, let me do that.  Please let me know if the fix for the note
>> is okay to commit as is on its own.
>
> ...the removal of the test guarding the note is attached.
>
>>
>> Martin
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Martin
>>
>
>
> gcc-17506.diff
>
> Improve note location.
>
> Related:
> PR tree-optimization/17506 - warning about uninitialized variable points to wrong location
> PR testsuite/37182 - Revision 139286 caused gcc.dg/pr17506.c and gcc.dg/uninit-15.c
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/17506
> 	PR testsuite/37182
> 	* tree-ssa-uninit.c (warn_uninit): Remove conditional guarding note.
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/17506
> 	PR testsuite/37182
> 	* gcc.dg/diagnostic-tree-expr-ranges-2.c: Add expected output.
> 	* gcc.dg/uninit-15-O0.c: Remove xfail.
> 	* gcc.dg/uninit-15.c: Same.
OK if neither Joern nor Nathan object by Wednesday morning (want to give 
them a couple workdays to chime in if they feel the need).

jeff

Re: [PATCH] improve note location and refactor warn_uninit

[Martin Sebor]

On 8/27/21 5:23 PM, Jeff Law wrote:
> 
> 
> On 8/26/2021 1:30 PM, Martin Sebor via Gcc-patches wrote:
>> On 8/26/21 10:38 AM, Martin Sebor wrote:
>>> On 8/26/21 1:00 AM, Richard Biener wrote:
>>>> On Wed, Aug 25, 2021 at 10:03 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>
>>>>> Richard, some time ago you mentioned you'd had trouble getting
>>>>> -Wuninitialized to print the note pointing to the uninitialized
>>>>> variable.  I said I'd look into it, and so I did.  The attached
>>>>> patch simplifies the warn_uninit() function to get rid of some
>>>>> redundant cruft: besides a few pointless null tests and
>>>>> a duplicate argument it also does away with ancient code that's
>>>>> responsible for the problem you saw.  It turns out that tests
>>>>> for the problem are already in the test suite but have been
>>>>> xfailed since the day they were added, so the patch simply
>>>>> removes the xfails.  I'll go ahead and commit this cleanup
>>>>> as obvious later this week unless you have suggestions for
>>>>> further changes.
>>>>
>>>> I do see lots of useful refactoring.
>>>>
>>>> -  if (context != NULL && gimple_has_location (context))
>>>> -    location = gimple_location (context);
>>>> -  else if (phiarg_loc != UNKNOWN_LOCATION)
>>>> -    location = phiarg_loc;
>>>> -  else
>>>> -    location = DECL_SOURCE_LOCATION (var);
>>>> +  /* Use either the location of the read statement or that of the PHI
>>>> +     argument, or that of the uninitialized variable, in that order,
>>>> +     whichever is valid.  */
>>>> +  location_t location = gimple_location (context);
>>>> +  if (location == UNKNOWN_LOCATION)
>>>> +    {
>>>> +      if (phi_arg_loc == UNKNOWN_LOCATION)
>>>> +       location = DECL_SOURCE_LOCATION (var);
>>>> +      else
>>>> +       location = phi_arg_loc;
>>>> +    }
>>>>
>>>> the comment is an improvement but I think the original flow is
>>>> easier to follow ;)
>>>
>>> It doesn't really simplify things so I can revert it.
>>
>> I've done that and pushed r12-3165, and...
>>
>>>> -      if (xloc.file != floc.file
>>>> -         || linemap_location_before_p (line_table, location, cfun_loc)
>>>> -         || linemap_location_before_p (line_table, 
>>>> cfun->function_end_locus,
>>>> -                                       location))
>>>> -       inform (DECL_SOURCE_LOCATION (var), "%qD was declared here", 
>>>> var);
>>>> ...
>>>> +  inform (var_loc, "%qD was declared here", var);
>>>>
>>>> and this is the actual change that "fixes" the missing diagnostics?  
>>>> Can
>>>> you explain what the reason for the original sing-and-dance was?  It
>>>> looks like it tries to print the 'was declared here' only for locations
>>>> within the current function (but then it probably intended to do that
>>>> on the DECL_SOURCE_LOCATION (var), not on whatever we are
>>>> choosing now)?
>>>
>>> The author(s) of the logic wanted to print the note only for variables
>>> declared in functions other than the one the uninitialized read is in.
>>> The idea first appears in pr17506, comment #25 (and attachment 12131).
>>> At that time GCC would print no note at all and pr17506 was about
>>> inlining making it difficult to find the uninitialized variable.
>>> The originally reported problem can still be reproduced on Godbolt
>>> (with the original very large translation unit):
>>> https://godbolt.org/z/8WW43rxnd
>>>
>>> I've reduced it to a small test case:
>>> https://godbolt.org/z/rnPEfPqPf
>>>
>>> It looks like they didn't think it would also be a problem if
>>> the variable was defined and read in the same function, even
>>> if the read was far away from the declaration.
>>>
>>>>
>>>> That said, I'd prefer if you can commit the refactoring independently
>>>> of this core change and can try to dig why this was added and what
>>>> it was supposed to do.
>>>
>>> Sure, let me do that.  Please let me know if the fix for the note
>>> is okay to commit as is on its own.
>>
>> ...the removal of the test guarding the note is attached.
>>
>>>
>>> Martin
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Tested on x86_64-linux.
>>>>>
>>>>> Martin
>>>
>>
>>
>> gcc-17506.diff
>>
>> Improve note location.
>>
>> Related:
>> PR tree-optimization/17506 - warning about uninitialized variable points to wrong location
>> PR testsuite/37182 - Revision 139286 caused gcc.dg/pr17506.c and gcc.dg/uninit-15.c
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/17506
>> 	PR testsuite/37182
>> 	* tree-ssa-uninit.c (warn_uninit): Remove conditional guarding note.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR tree-optimization/17506
>> 	PR testsuite/37182
>> 	* gcc.dg/diagnostic-tree-expr-ranges-2.c: Add expected output.
>> 	* gcc.dg/uninit-15-O0.c: Remove xfail.
>> 	* gcc.dg/uninit-15.c: Same.
> OK if neither Joern nor Nathan object by Wednesday morning (want to give 
> them a couple workdays to chime in if they feel the need).

Having heard nothing back I've just pushed r12-3315.

Martin