[PATCH] improve note location and refactor warn_uninit

Richard Biener richard.guenther@gmail.com
Fri Aug 27 17:30:41 GMT 2021


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



More information about the Gcc-patches mailing list