On August 27, 2021 5:20:57 PM GMT+02:00, Martin Sebor <mse...@gmail.com> wrote:
>On 8/27/21 12:23 AM, Richard Biener wrote:
>> 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.
>
>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
>>>>
>>>
>
