[PATCH] D148692: Fix uninitialized class members

Fri May 12 12:20:54 PDT 2023

vitalybuka added a comment.

In D148692#4338462 <https://reviews.llvm.org/D148692#4338462>, @dblaikie wrote:

> In D148692#4338228 <https://reviews.llvm.org/D148692#4338228>, @vitalybuka wrote:
>
>> In D148692#4338165 <https://reviews.llvm.org/D148692#4338165>, @dblaikie wrote:
>>
>>> Adding initializers to variables that don't need it (because through some dynamic path they are initialized before they're used) may hinder tools like msan from finding real bugs.
>>
>> It's can be useful for local variables. But for class members, especially complex, when half members are already initialized, it's seems cont-productive.
>
> Actually for class members there are cases where it comes to mind - when a member is only used under some substate of the object, etc. (we do also have compile time warnings to catch some of these cases too, that aren't just "make sure everything's always initialized at the declaration" - checking for private members that are only ever read and never written, which is admittedly a smaller set of cases)
>
>> We will more often introduce bugs then catch some with msan.
>
> Happen to have data on this?

Not very scientific, just my experience. I have to do a lot of msan cleanups. 90%+ is pure UBs, like values loaded but not rely used. But it's UB and may trigger miss-compile, so need to be fixed. I've see logical bugs very rarely. So mindless init in most cases is a perfect fix.

>> Unless it's not perf issue, I would rather see consistent initialized state.
>> Dynamic checks do not cover all cases in testing, plus optimizations suppress some bugs.
>>
>> Fopr logical bug I would rather recommend to set default to some invalid value, and then assert() or diagnostics.
>
> Not everything has invalid values.

Yes, the point is usually always a better way to check invariant, than keeping variable uninitialized. Msan should be considered as the last resort.

> In any case, I think it'd be a pretty major stylistic requirement for LLVM if we're going to adopt initializing members at declaration in all cases - maybe worth a style discussion on the forums, etc, before we go adding these where they aren't demonstrably needed/only motivated by a stylistic checker.
>
> If we do want to adopt such a policy, it seems like a pretty easy one to enforce/check/etc, rather than these sporadic "someone ran a checker" situations.

Yes, discussed and agreed rule would be nice to have.

Still if we have no reverse rule as well, do we want to push back on such patches? That genuine question, I don't know what should be the answer.
I don't advocate for the patch. I just claim that intentionally keeping value uninitialized, to let dynamic tools to catch a bug, causes more maintenance issues, than "just-in-case" initialization.

BTW. @akshaykhadse  In particular I don't like about the patch:
1 it changes many unrelated things in one patch.
2 it mention "static code analysis", without details.

3. enforcing build bot would be nice, as nothing prevents anyone from reverting the patch. (unlikely possible to make people to respect the bot without formal rule).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148692/new/

https://reviews.llvm.org/D148692