Page MenuHomePhabricator

Eliminate many benign instances of "potentially uninitialized local variable" warnings
Needs ReviewPublic

Authored by ariccio on Mar 8 2016, 10:58 PM.
This revision needs review, but all specified reviewers are disabled or inactive.

Details

Reviewers
tstellarAMD
Summary

Currently, the "potentially uninitialized local variable" & "potentially uninitialized local pointer variable" warnings are turned off. At some point we should probably turn them back on, because they can find serious problems. This patch eliminates the most obviously benign offenders.

Assuming that I did everything correctly, there should be no behavior change. In several places, I added defensive checks to bulletproof against future changes.

This leaves about ~33 not-so-obviously benign offenders. which I'll include here for convenience:

SeverityCodeDescriptionProjectFileLine
WarningC4703potentially uninitialized local pointer variable 'ExprOffset' usedLLVMMipsAsmParserc:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp2668
WarningC4701potentially uninitialized local variable 'Encoding' usedclangCodeGenc:\llvm\llvm\tools\clang\lib\codegen\cgdebuginfo.cpp573
WarningC4701potentially uninitialized local variable 'result' usedclangCodeGenc:\llvm\llvm\tools\clang\lib\codegen\cgobjc.cpp2675
WarningC4701potentially uninitialized local variable 'Nullability' usedclangParsec:\llvm\llvm\tools\clang\lib\parse\parseobjc.cpp1202
WarningC4701potentially uninitialized local variable 'RepresentationMethod' usedclangParsec:\llvm\llvm\tools\clang\lib\parse\parsepragma.cpp1582
WarningC4701potentially uninitialized local variable 'FirstOpKind' usedclangSemac:\llvm\llvm\tools\clang\lib\sema\sematemplate.cpp4464
WarningC4701potentially uninitialized local variable 'FnEntry8BitAbbrev' usedLLVMBitWriterc:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp2353
WarningC4701potentially uninitialized local variable 'FnEntry7BitAbbrev' usedLLVMBitWriterc:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp2357
WarningC4701potentially uninitialized local variable 'FnEntry6BitAbbrev' usedLLVMBitWriterc:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp2355
WarningC4701potentially uninitialized local variable 'OutIt' usedLLVMCodeGenc:\llvm\llvm\lib\codegen\liveinterval.cpp876
WarningC4701potentially uninitialized local variable 'mid' usedLLVMHexagonCodeGenc:\llvm\build\lib\target\hexagon\hexagongeninstrinfo.inc9722
WarningC4701potentially uninitialized local variable 'RealEightBitCounterArray' usedLLVMInstrumentationc:\llvm\llvm\lib\transforms\instrumentation\sanitizercoverage.cpp294
WarningC4701potentially uninitialized local variable 'LoOffset' usedLLVMMipsAsmParserc:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp2668
WarningC4701potentially uninitialized local variable 'HiOffset' usedLLVMMipsAsmParserc:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp2659
WarningC4701potentially uninitialized local variable 'ExprOffset' usedLLVMMipsAsmParserc:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp2668
WarningC4701potentially uninitialized local variable 'BranchTargetNoTraps' usedLLVMMipsAsmParserc:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp3072
WarningC4701potentially uninitialized local variable 'MatImm' usedLLVMPowerPCCodeGenc:\llvm\llvm\lib\target\powerpc\ppciseldagtodag.cpp808
WarningC4701potentially uninitialized local variable 'MaskEnd' usedLLVMPowerPCCodeGenc:\llvm\llvm\lib\target\powerpc\ppciseldagtodag.cpp809
WarningC4701potentially uninitialized local variable 'FirstLP' usedLLVMScalarOptsc:\llvm\llvm\lib\transforms\scalar\loadcombine.cpp198
WarningC4701potentially uninitialized local variable 'CC' usedLLVMSelectionDAGc:\llvm\llvm\lib\codegen\selectiondag\dagcombiner.cpp13889
WarningC4701potentially uninitialized local variable 'Operation' usedllvm-arc:\llvm\llvm\tools\llvm-ar\llvm-ar.cpp287
WarningC4701potentially uninitialized local variable 'Kind' usedllvm-arc:\llvm\llvm\tools\llvm-ar\llvm-ar.cpp602
WarningC4701potentially uninitialized local variable 'CurSegAddress' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp8548
WarningC4701potentially uninitialized local variable 'objc_class' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp5435
WarningC4701potentially uninitialized local variable 'r_type' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp1781
WarningC4701potentially uninitialized local variable 'r_value' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp1783
WarningC4701potentially uninitialized local variable 'pair_r_value' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp1784
WarningC4701potentially uninitialized local variable 'r_value' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp1970
WarningC4701potentially uninitialized local variable 'other_half' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp1940
WarningC4701potentially uninitialized local variable 'pair_r_value' usedllvm-objdumpc:\llvm\llvm\tools\llvm-objdump\machodump.cpp1980
WarningC4703potentially uninitialized local pointer variable 'result' usedclangCodeGenc:\llvm\llvm\tools\clang\lib\codegen\cgobjc.cpp2675
WarningC4703potentially uninitialized local pointer variable 'OutIt' usedLLVMCodeGenc:\llvm\llvm\lib\codegen\liveinterval.cpp876
WarningC4703potentially uninitialized local pointer variable 'RealEightBitCounterArray' usedLLVMInstrumentationc:\llvm\llvm\lib\transforms\instrumentation\sanitizercoverage.cpp294

Diff Detail

Event Timeline

ariccio updated this revision to Diff 50108.Mar 8 2016, 10:58 PM
ariccio retitled this revision from to Eliminate many benign instances of "potentially uninitialized local variable" warnings.
ariccio updated this object.
ariccio added subscribers: llvm-commits, cfe-commits.
ariccio updated this object.Mar 8 2016, 11:00 PM
ariccio edited edge metadata.
ariccio updated this object.
ariccio updated this object.Mar 8 2016, 11:02 PM

Thanks for working on this.

llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2715–2719

I think this should be Mips::NoRegister.

llvm/lib/Target/Mips/MipsAsmPrinter.cpp
889–935

These two are false positives since all the enum values are covered.

The default labels cause warnings when clang is the compiler:

lib/Target/Mips/MipsAsmPrinter.cpp:911:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

which will cause -Werror builders to fail.

I'd suggest dropping the default labels to avoid the clang warning, but keeping the initializations to keep your compiler happy.

Hello. I will take a look at fixing the warnings coming from LLVMMipsAsmParser.

I think we had a discussion about this before.

Clang has a few versions of this warning. One version is probably as
aggressive as the warning you are trying to quiet (-Wmaybe-uninitialized)
and we made a deliberate choice not to enable it for the project. I think
we should probably make the same choice here and not enable it.

Initializations we never expect to use (eg because we have a covered switch
that initializes in all cases, or some slightly complex control flow the
compiler can't see through) hinder our ability to find uses of those with
tools like msan.

We do enable clangs -Wsometimes-uninitialized which is more restrictive
(fewer false positives. At the cost of more false negatives).

Thanks for doing this!

I will fix these 3, which one of my changes would have provoked. It is a benign case since we do have an assert at the use site checking the same condition that the inits are guarded with, but there is no reason not to 0 init these to avoid the bogus warning.

Warning C4701 potentially uninitialized local variable 'FnEntry8BitAbbrev' used LLVMBitWriter c:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp 2353
Warning C4701 potentially uninitialized local variable 'FnEntry7BitAbbrev' used LLVMBitWriter c:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp 2357
Warning C4701 potentially uninitialized local variable 'FnEntry6BitAbbrev' used LLVMBitWriter c:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp 2355

Initializations we never expect to use (eg because we have a covered switch
that initializes in all cases, or some slightly complex control flow the
compiler can't see through) hinder our ability to find uses of those with
tools like msan.

Too bad... Maybe msan should have a way to specify that you're default initializing a variable, and treat it as if you're not initializing it at all?

That said, I'm not sure that I agree with keeping Potentially uninitialized local pointer variable 'name' used off (that's C4703, not C4701), because uninitialized local pointer bugs are more dangerous (I'm thinking of write-what-where uninitialized pointer vulnerabilities), and I'd rather crash on a nullptr deref than some undefined behavior.

I will drop the default cases as @dsanders noted, but should I drop all the local variable inits too? In several cases (which I didn't specifically note) I chose default initialization values that would trigger pre-existing asserts if nobody assigned anything else to them.

Regarding the 33 warnings I noted, i couldn't tell whether they're obviously false positives, which I think warrants someone other than me carefully looking through it. Some of the warnings are caused by assigning to a variable inside of a loop with a dynamically sized bound; for those, I'd much rather (a) assert that the bound is nonzero/loop will be executed at least once, or (b) assign the variable some sentinel value, and then assert it before the variable is actually used. Thoughts?

Oh, and by the way, what's the policy on using enum classes instead of C style enums? I bet the compiler would have fewer false positives with strongly typed enums?

ariccio updated this revision to Diff 50241.Mar 9 2016, 11:36 PM

For some reason, this smaller diff was harder to upload than the first diff.

Hi is there any update on this issue, which is blocking https://llvm.org/bugs/show_bug.cgi?id=11446 ?

@ariccio, are you blocked on this or waiting for reviewers ? This seems to be blocking the patch that fixes plugins on MSVC, thanks

@dblaikie mentioned before that it is not clear that this is something we want to do at all (initialize all local variables).
Would it be something we should have policy on in the first place before integrating such a patch? (Reminds me of constructor that don't initializes all members "on purpose").
Maybe send an RFC to LLVM-dev and codify this in the coding guidelines?

... I'd rather crash on a nullptr deref than some undefined behavior.

Wait... Isn't nullptr deref UB?

@ariccio, are you blocked on this or waiting for reviewers ? This seems to be blocking the patch that fixes plugins on MSVC, thanks

https://llvm.org/bugs/show_bug.cgi?id=11446 comment #6 links to this review, but I think it might have been a mistake and unrelated.