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 <mse...@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
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
