[PATCH] c++: Implement -Wuninitialized for mem-initializers [PR19808]

[Jason Merrill]

On 11/20/20 6:08 PM, Martin Sebor wrote:
> On 11/19/20 11:41 AM, Jason Merrill wrote:
>> On 11/17/20 3:44 AM, Jan Hubicka wrote:
>>>> On Tue, Nov 17, 2020 at 01:33:48AM -0500, Jason Merrill via 
>>>> Gcc-patches wrote:
>>>>>>> Why doesn't the middle-end warning work for inline functions?
>>>>>>
>>>>>> It does but only when they're called (and, as usual, also unless
>>>>>> the uninitialized use is eliminated).
>>>>>
>>>>> Yes, but why?  I assume because we don't bother going through all 
>>>>> the phases
>>>>> of compilation for unused inlines, but couldn't we change that when 
>>>>> we're
>>>>> asking for (certain) warnings?
>>>>
>>>> CCing Richard and Honza on this.
>>>>
>>>> I think for unused functions we don't even gimplify unused 
>>>> functions, the
>>>> cgraph code just throws them away.  Even trying just to run the 
>>>> first few
>>>> passes (gimplification up to uninit1) would have several high costs,
>>> Note that uninit1 is a late pass so it is not just few passes we speak
>>> about.  Late passes are run only on cocde that really lands in .s file
>>> so enabling them would mean splitting the pass queue and running another
>>> unreachable code somewhere.  That would confuse inliner and other IPA
>>> passes since they will have to somehow deal with dead code in their
>>> program size estimate and also affect LTO.
>>>
>>> Even early passes are run only on reachable portion of program, since
>>> functions are analyzed by cgraphunit on demand (only if they are
>>> analyzed by someone else). Simlar logic is also done be C++ FE to decide
>>> what templates.  Changling this would also have quite some compile
>>> time/memory use impact.
>>>
>>> There is -fkeep-inline-functions.
>>
>> OK, thanks for the explanation.  -fkeep-inline-functions seems like an 
>> acceptable answer for people who want a warning audit of their library 
>> header inlines.
>>
>> Martin, I notice that the middle-end warning doesn't currently catch 
>> this:
>>
>> struct B { int i,j; };
>>
>> struct A
>> {
>>    B b;
>>    A(): b({b.i}) { }
>> };
>>
>> A a;
>>
>> It does warn if B only has one member; adding the second wrongly 
>> silences the warning.
> 
> The GIMPLE for A's ctor looks like this:
> 
> A::A (struct A * const this)
> {
>    *this = {CLOBBER};
>    {
>      this->b = {};
>      _1 = this->b.i;
>      this->b.i = _1;
>    }
> }
> 
> so the b member is cleared first before it's read from and there's
> nothing to warn about.  This happens for C structs too:
> 
> void f (void*);
> 
> struct A { int i, j; };
> 
> void g (void)
> {
>    struct A a = { a.i };
>    f (&a);
> }
> 
> The uninitialized read is in the original dump (below) so the zero
> initialization happens in the gimplifier and could be diagnosed
> there.
> 
> ;; enabled by -tree-original
> 
> 
> {
>    struct A a = {.i=a.i};
> 
>      struct A a = {.i=a.i};
>    f ((void *) &a);
> }

Ah, yes, because gimplify_init_constructor leaves the first assignment 
to clear the whole object if there aren't initializers for all the members.

Jason