On Thu, Aug 26, 2021 at 9:30 PM Martin Sebor <mse...@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 <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.
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
> >
>
