r234591 - Initializing an uninitialized data member; should be NFC.

[David Blaikie]

On Fri, Apr 10, 2015 at 8:24 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Apr 10, 2015 at 11:20 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> On Fri, Apr 10, 2015 at 8:06 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> On Fri, Apr 10, 2015 at 11:03 AM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>> >
>>> >
>>> > On Fri, Apr 10, 2015 at 6:05 AM, Aaron Ballman <aaron at aaronballman.com>
>>> > wrote:
>>> >>
>>> >> Author: aaronballman
>>> >> Date: Fri Apr 10 08:05:04 2015
>>> >> New Revision: 234591
>>> >>
>>> >> URL: http://llvm.org/viewvc/llvm-project?rev=234591&view=rev
>>> >> Log:
>>> >> Initializing an uninitialized data member; should be NFC.
>>> >
>>> >
>>> > Was this being used uninitialized? Where?
>>> >
>>> > It's actually rather helpful to keep uninitialized members
>>> > uninitiialized if
>>> > they aren't intended to be used, so MSan et, al, can catch their use.
>>>
>>> It was not, that I could tell. However, it was the only member to not
>>> be initialized, and it is a public member of the class, which strikes
>>> me as a dangerous code smell.
>>
>>
>> The danger is rather mitigated by having tools like MSan.
>
> Not everyone has those tools available to them, or uses them
> regularly, so I'm not completely convinced that's a reasonable
> mitigation in this case. I would agree it's a great last resort, but I
> don't think it is sufficient for a *bool* (for which it is really easy
> to get accidentally correct behavior from uninitialized variables).
> Perhaps it's more sufficient for other types where it's less likely to
> get correct behavior while debugging locally. But it is very annoying
> to have locally "correct" code that winds up breaking bots and sends
> out a ton of spam when that can be otherwise avoided.

It's a tricky tradeoff, to be sure - but usually msan ends up catching
the more uncommon cases after commit anyway, I think - cases that
hadn't been well tested, so it also seems unfortunate to tradeoff the
relatively common/easy/early-found bugs for hiding the longer tail
ever further...

I'm not sure what the story/timeline is for MSan on Windows, but it's
probably plausible in a VM, especially for small test cases.

It's not a tool I run all the time either, but it's a tool that's
available when you hit bugs that might be memory related (along with
the other sanitizers).

>> But in any case, there doesn't seem to be anything special about that member
>> to warrant it being uninitialized while the others are initialized, as you
>> say.
>>
>> I had thought this might be one of the cases of a discriminated union that
>> I've seen in other places, where initializing the non-active members really
>> hinders MSan's ability to check that union accesses all only access the
>> active members. But looking at it more closely that doesn't seem to be the
>> case here - it's just one of a bunch of similar members, nothing special.
>
> That was the conclusion I came to as well. Plus, it looked like a red
> herring while I was debugging something else. Every other member was
> initialized except this one, so I spent time chasing down why that
> was.
>
> ~Aaron