[PATCH] improve note location and refactor warn_uninit

Richard Biener richard.guenther@gmail.com
Fri Aug 27 06:23:50 GMT 2021


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


More information about the Gcc-patches mailing list