Fix uninitialized class members

[akshaykhadse]

Summary

This patch fixes static code analysis warnings.

[akshaykhadse]

Fix typo

[akshaykhadse]

Fix failures

[LuoYuanke]

llvm/include/llvm/CodeGen/ReachingDefAnalysis.h

90

It seems in ReachingDefAnalysis::enterBasicBlock() CurInstr is reset to 0, so I wonder if it is better to initialized to 0.

llvm/lib/CodeGen/IfConversion.cpp

200–201

In test/CodeGen/AArch64/O3-pipeline.ll, this pass runs before the register allocation pass, so set it true may be better.

llvm/lib/CodeGen/ShrinkWrap.cpp

141

Set ~0u? See https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/TargetInstrInfo.h#L101.

144

Ditto.

[akshaykhadse]

llvm/include/llvm/CodeGen/ReachingDefAnalysis.h

90

Setting it to 0 would imply that there is at least one instruction in the basic block. I am not sure if that would be true if this method would run on empty basic block. But, if we cna be sure that there are no empty basic blocks, we can set it to 0. Let me know what you think.

[akshaykhadse]

Address comments

[LuoYuanke]

LGTM

[akshaykhadse]

Failures are unrealted to this change. Same are present in previous reviews as well

[mehdi_amini]

The description is missing context, are these fixing bugs? How were these issues found?

[LuoYuanke]

In D148692#4282643, @mehdi_amini wrote:

The description is missing context, are these fixing bugs? How were these issues found?

I think these are to fix the issues that reported by Coverity/Klocwork tool.

[dblaikie]

In D148692#4282704, @LuoYuanke wrote:

In D148692#4282643, @mehdi_amini wrote:

The description is missing context, are these fixing bugs? How were these issues found?

I think these are to fix the issues that reported by Coverity/Klocwork tool.

For the record - these sort of changes may not be desirable.

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.

[vitalybuka]

In 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. We will more often introduce bugs then catch some with msan. 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.

[dblaikie]

In D148692#4338228, @vitalybuka wrote:

In 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?

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.

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.

[vitalybuka]

In D148692#4338462, @dblaikie wrote:

In D148692#4338228, @vitalybuka wrote:

In 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).

[barannikov88]

I think it is more the static analyzer issue that it is not able to figure out that
the entry point of the class is not its constructor, but runOnMachineFunction,
which initializes most (or all) member variables before using them.

Not everything has invalid values.

This is a really good point. Initializing all fields to some default values will make
it more difficult to add a new filed that does not have an obvious default
(because, you know, consistency prevails).
Even in this patch some time has been spent discussing what's the best default
value for a particular variable, and I've seen more of such discussions in other
similar patches. That just wastes people's time without any benefit for human.

Finally, I don't remember any of the recent patches caught a real bug.

[LuoYuanke]

It looks to me that initializing variable is a good practice to avoid security issue. If it doesn’t cause performance degradation, it should be better than uninitialized variable which may cause UB.
I once encountered issue that compiler behavior is not consistent when compiling several times, not sure it is related with uninitialized variable.

llvm/include/llvm/CodeGen/GlobalISel/LoadStoreOpt.h

private:
   /// optimization. \p AddrSpace indicates which address space to use when
   /// probing the legalizer info for legal stores.
   void initializeStoreMergeTargetInfo(unsigned AddrSpace = 0);
   /// A map between address space numbers and a bitvector of supported stores
   /// sizes. Each bit in the bitvector represents whether a store size of
   /// that bit's value is legal. E.g. if bit 64 is set, then 64 bit scalar
   /// stores are legal.
   DenseMap<unsigned, BitVector> LegalStoreSizes;
-  bool IsPreLegalizer;
+  bool IsPreLegalizer = false;
   /// Contains instructions to be erased at the end of a block scan.
   SmallSet<MachineInstr *, 16> InstsToErase;
 
 public:
   LoadStoreOpt();
   LoadStoreOpt(std::function<bool(const MachineFunction &)>);
 
   StringRef getPassName() const override { return "LoadStoreOpt"; }

llvm/include/llvm/CodeGen/ReachingDefAnalysis.h

 };
 
 /// This class provides the reaching def analysis.
 class ReachingDefAnalysis : public MachineFunctionPass {
 private:
   MachineFunction *MF = nullptr;
   const TargetRegisterInfo *TRI = nullptr;
   LoopTraversal::TraversalOrder TraversedMBBOrder;
-  unsigned NumRegUnits;
+  unsigned NumRegUnits = 0;
   /// Instruction that defined each register, relative to the beginning of the
   /// current basic block.  When a LiveRegsDefInfo is used to represent a
   /// live-out register, this value is relative to the end of the basic block,
   /// so it will be a negative number.
   using LiveRegsDefInfo = std::vector<int>;
   LiveRegsDefInfo LiveRegs;
 
   /// Keeps clearance information for all registers. Note that this
   /// is different from the usual definition notion of liveness. The CPU
   /// doesn't care whether or not we consider a register killed.
   using OutRegsInfoMap = SmallVector<LiveRegsDefInfo, 4>;
   OutRegsInfoMap MBBOutRegsInfos;
 
   /// Current instruction number.
   /// The first instruction in each basic block is 0.
-  int CurInstr;
+  int CurInstr = -1;
 
   /// Maps instructions to their instruction Ids, relative to the beginning of
   /// their basic blocks.
   DenseMap<MachineInstr *, int> InstIds;
 
   /// All reaching defs of a given RegUnit for a given MBB.
   using MBBRegUnitDefs = TinyPtrVector<ReachingDef>;
   /// All reaching defs of all reg units for a given MBB

llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h

protected:
 
     /// The beginning of the range to be scheduled.
     MachineBasicBlock::iterator RegionBegin;
 
     /// The end of the range to be scheduled.
     MachineBasicBlock::iterator RegionEnd;
 
     /// Instructions in this region (distance(RegionBegin, RegionEnd)).
-    unsigned NumRegionInstrs;
+    unsigned NumRegionInstrs = 0;
 
     /// After calling BuildSchedGraph, each machine instruction in the current
     /// scheduling region is mapped to an SUnit.
     DenseMap<MachineInstr*, SUnit*> MISUnitMap;
 
     // State internal to DAG building.
     // -------------------------------
 

llvm/lib/CodeGen/BranchFolding.h

public:
       }
 
       void setTailStartPos(MachineBasicBlock::iterator Pos) {
         TailStartPos = Pos;
       }
     };
     std::vector<SameTailElt> SameTails;
 
-    bool AfterBlockPlacement;
-    bool EnableTailMerge;
-    bool EnableHoistCommonCode;
-    bool UpdateLiveIns;
+    bool AfterBlockPlacement = false;
+    bool EnableTailMerge = false;
+    bool EnableHoistCommonCode = false;
+    bool UpdateLiveIns = false;
     unsigned MinCommonTailLength;
     const TargetInstrInfo *TII = nullptr;
     const MachineRegisterInfo *MRI = nullptr;
     const TargetRegisterInfo *TRI = nullptr;
     MachineLoopInfo *MLI = nullptr;
     LivePhysRegs LiveRegs;
 
   private:

llvm/lib/CodeGen/CodeGenPrepare.cpp

class AddressingModeMatcher {
   // A GEP which has too large offset to be folded into the addressing mode.
   std::pair<AssertingVH<GetElementPtrInst>, int64_t> &LargeOffsetGEP;
 
   /// This is set to true when we should not do profitability checks.
   /// When true, IsProfitableToFoldIntoAddressingMode always returns true.
   bool IgnoreProfitability;
 
   /// True if we are optimizing for size.
-  bool OptSize;
+  bool OptSize = false;
 
   ProfileSummaryInfo *PSI;
   BlockFrequencyInfo *BFI;
 
   AddressingModeMatcher(
       SmallVectorImpl<Instruction *> &AMI, const TargetLowering &TLI,
       const TargetRegisterInfo &TRI, const LoopInfo &LI,
       const std::function<const DominatorTree &()> getDTFn, Type *AT,

llvm/lib/CodeGen/IfConversion.cpp

class IfConverter : public MachineFunctionPass {
     const TargetLoweringBase *TLI = nullptr;
     const TargetInstrInfo *TII = nullptr;
     const TargetRegisterInfo *TRI = nullptr;
     const MachineBranchProbabilityInfo *MBPI = nullptr;
     MachineRegisterInfo *MRI = nullptr;
 
     LivePhysRegs Redefs;
 
-    bool PreRegAlloc;
-    bool MadeChange;
+    bool PreRegAlloc = true;
+    bool MadeChange = false;
     int FnNum = -1;
     std::function<bool(const MachineFunction &)> PredicateFtor;
 
   public:
     static char ID;
 
     IfConverter(std::function<bool(const MachineFunction &)> Ftor = nullptr)
         : MachineFunctionPass(ID), PredicateFtor(std::move(Ftor)) {

llvm/lib/CodeGen/MachineBlockPlacement.cpp

class MachineBlockPlacement : public MachineFunctionPass {
   /// must be done inline.
   TailDuplicator TailDup;
 
   /// Partial tail duplication threshold.
   BlockFrequency DupThreshold;
 
   /// True:  use block profile count to compute tail duplication cost.
   /// False: use block frequency to compute tail duplication cost.
-  bool UseProfileCount;
+  bool UseProfileCount = false;
 
   /// Allocator and owner of BlockChain structures.
   ///
   /// We build BlockChains lazily while processing the loop structure of
   /// a function. To reduce malloc traffic, we allocate them using this
   /// slab-like allocator, and destroy them after the pass completes. An
   /// important guarantee is that this allocator produces stable pointers to
   /// the chains.

llvm/lib/CodeGen/MachineCombiner.cpp

class MachineCombiner : public MachineFunctionPass {
   MachineTraceMetrics::Ensemble *TraceEnsemble = nullptr;
   MachineBlockFrequencyInfo *MBFI = nullptr;
   ProfileSummaryInfo *PSI = nullptr;
   RegisterClassInfo RegClassInfo;
 
   TargetSchedModel TSchedModel;
 
   /// True if optimizing for code size.
-  bool OptSize;
+  bool OptSize = false;
 
 public:
   static char ID;
   MachineCombiner() : MachineFunctionPass(ID) {
     initializeMachineCombinerPass(*PassRegistry::getPassRegistry());
   }
   void getAnalysisUsage(AnalysisUsage &AU) const override;
   bool runOnMachineFunction(MachineFunction &MF) override;

llvm/lib/CodeGen/MachineCopyPropagation.cpp

private:
   /// Candidates for deletion.
   SmallSetVector<MachineInstr *, 8> MaybeDeadCopies;
 
   /// Multimap tracking debug users in current BB
   DenseMap<MachineInstr *, SmallSet<MachineInstr *, 2>> CopyDbgUsers;
 
   CopyTracker Tracker;
 
-  bool Changed;
+  bool Changed = false;
 };
 
 } // end anonymous namespace
 
 char MachineCopyPropagation::ID = 0;
 
 char &llvm::MachineCopyPropagationID = MachineCopyPropagation::ID;
 

llvm/lib/CodeGen/MachineLICM.cpp

namespace {
 
   class MachineLICMBase : public MachineFunctionPass {
     const TargetInstrInfo *TII = nullptr;
     const TargetLoweringBase *TLI = nullptr;
     const TargetRegisterInfo *TRI = nullptr;
     const MachineFrameInfo *MFI = nullptr;
     MachineRegisterInfo *MRI = nullptr;
     TargetSchedModel SchedModel;
-    bool PreRegAlloc;
-    bool HasProfileData;
+    bool PreRegAlloc = false;
+    bool HasProfileData = false;
 
     // Various analyses that we use...
     AliasAnalysis *AA = nullptr;               // Alias analysis info.
     MachineBlockFrequencyInfo *MBFI = nullptr; // Machine block frequncy info
     MachineLoopInfo *MLI = nullptr;            // Current MachineLoopInfo
     MachineDominatorTree *DT = nullptr; // Machine dominator tree for the cur loop
 
     // State that is updated as we process loops
-    bool         Changed;          // True if a loop is changed.
-    bool         FirstInLoop;      // True if it's the first LICM in the loop.
+    bool Changed = false;           // True if a loop is changed.
+    bool FirstInLoop = false;       // True if it's the first LICM in the loop.
     MachineLoop *CurLoop = nullptr; // The current loop we are working on.
     MachineBasicBlock *CurPreheader = nullptr; // The preheader for CurLoop.
 
     // Exit blocks for CurLoop.
     SmallVector<MachineBasicBlock *, 8> ExitBlocks;
 
     bool isExitBlock(const MachineBasicBlock *MBB) const {
       return is_contained(ExitBlocks, MBB);
enum {
       SpeculateFalse   = 0,
       SpeculateTrue    = 1,
       SpeculateUnknown = 2
     };
 
     // If a MBB does not dominate loop exiting blocks then it may not safe
     // to hoist loads from this block.
     // Tri-state: 0 - false, 1 - true, 2 - unknown
-    unsigned SpeculationState;
+    unsigned SpeculationState = SpeculateUnknown;
 
   public:
     MachineLICMBase(char &PassID, bool PreRegAlloc)
         : MachineFunctionPass(PassID), PreRegAlloc(PreRegAlloc) {}
 
     bool runOnMachineFunction(MachineFunction &MF) override;
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {

llvm/lib/CodeGen/MachineVerifier.cpp

struct MachineVerifier {
     const char *Banner;
     const MachineFunction *MF = nullptr;
     const TargetMachine *TM = nullptr;
     const TargetInstrInfo *TII = nullptr;
     const TargetRegisterInfo *TRI = nullptr;
     const MachineRegisterInfo *MRI = nullptr;
     const RegisterBankInfo *RBI = nullptr;
 
-    unsigned foundErrors;
+    unsigned foundErrors = 0;
 
     // Avoid querying the MachineFunctionProperties for each operand.
-    bool isFunctionRegBankSelected;
-    bool isFunctionSelected;
-    bool isFunctionTracksDebugUserValues;
+    bool isFunctionRegBankSelected = false;
+    bool isFunctionSelected = false;
+    bool isFunctionTracksDebugUserValues = false;
 
     using RegVector = SmallVector<Register, 16>;
     using RegMaskVector = SmallVector<const uint32_t *, 4>;
     using RegSet = DenseSet<Register>;
     using RegMap = DenseMap<Register, const MachineInstr *>;
     using BlockSet = SmallPtrSet<const MachineBasicBlock *, 8>;
 
     const MachineInstr *FirstNonPHI = nullptr;

llvm/lib/CodeGen/PrologEpilogInserter.cpp

private:
   // Save and Restore blocks of the current function. Typically there is a
   // single save block, unless Windows EH funclets are involved.
   MBBVector SaveBlocks;
   MBBVector RestoreBlocks;
 
   // Flag to control whether to use the register scavenger to resolve
   // frame index materialization registers. Set according to
   // TRI->requiresFrameIndexScavenging() for the current function.
-  bool FrameIndexVirtualScavenging;
+  bool FrameIndexVirtualScavenging = false;
 
   // Flag to control whether the scavenger should be passed even though
   // FrameIndexVirtualScavenging is used.
-  bool FrameIndexEliminationScavenging;
+  bool FrameIndexEliminationScavenging = false;
 
   // Emit remarks.
   MachineOptimizationRemarkEmitter *ORE = nullptr;
 
   void calculateCallFrameInfo(MachineFunction &MF);
   void calculateSaveRestoreBlocks(MachineFunction &MF);
   void spillCalleeSavedRegs(MachineFunction &MF);
 

llvm/lib/CodeGen/RegAllocGreedy.h

enum CutOffStage {
 
     // lcr-max-depth cutoff encountered
     CO_Depth = 1,
 
     // lcr-max-interf cutoff encountered
     CO_Interf = 2
   };
 
-  uint8_t CutOffInfo;
+  uint8_t CutOffInfo = CutOffStage::CO_None;
 
 #ifndef NDEBUG
   static const char *const StageName[];
 #endif
 
   // splitting state.
   std::unique_ptr<SplitAnalysis> SA;
   std::unique_ptr<SplitEditor> SE;
#endif
   SmallSetVector<const LiveInterval *, 8> SetOfBrokenHints;
 
   /// The register cost values. This list will be recreated for each Machine
   /// Function
   ArrayRef<uint8_t> RegCosts;
 
   /// Flags for the live range priority calculation, determined once per
   /// machine function.
-  bool RegClassPriorityTrumpsGlobalness;
+  bool RegClassPriorityTrumpsGlobalness = false;
 
-  bool ReverseLocalAssignment;
+  bool ReverseLocalAssignment = false;
 
 public:
   RAGreedy(const RegClassFilterFunc F = allocateAllRegClasses);
 
   /// Return the pass name.
   StringRef getPassName() const override { return "Greedy Register Allocator"; }
 
   /// RAGreedy analysis usage.

llvm/lib/CodeGen/ShrinkWrap.cpp

class ShrinkWrap : public MachineFunctionPass {
   /// Hold the loop information. Used to determine if Save and Restore
   /// are in the same loop.
   MachineLoopInfo *MLI = nullptr;
 
   // Emit remarks.
   MachineOptimizationRemarkEmitter *ORE = nullptr;
 
   /// Frequency of the Entry block.
-  uint64_t EntryFreq;
+  uint64_t EntryFreq = 0;
 
   /// Current opcode for frame setup.
-  unsigned FrameSetupOpcode;
+  unsigned FrameSetupOpcode = ~0u;
 
   /// Current opcode for frame destroy.
-  unsigned FrameDestroyOpcode;
+  unsigned FrameDestroyOpcode = ~0u;
 
   /// Stack pointer register, used by llvm.{savestack,restorestack}
   Register SP;
 
   /// Entry block.
   const MachineBasicBlock *Entry = nullptr;
 
   using SetOfRegs = SmallSetVector<unsigned, 16>;

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

class TwoAddressInstructionPass : public MachineFunctionPass {
   MachineFunction *MF = nullptr;
   const TargetInstrInfo *TII = nullptr;
   const TargetRegisterInfo *TRI = nullptr;
   const InstrItineraryData *InstrItins = nullptr;
   MachineRegisterInfo *MRI = nullptr;
   LiveVariables *LV = nullptr;
   LiveIntervals *LIS = nullptr;
   AliasAnalysis *AA = nullptr;
-  CodeGenOpt::Level OptLevel;
+  CodeGenOpt::Level OptLevel = CodeGenOpt::None;
 
   // The current basic block being processed.
   MachineBasicBlock *MBB = nullptr;
 
   // Keep track the distance of a MI from the start of the current basic block.
   DenseMap<MachineInstr*, unsigned> DistanceMap;
 
   // Set of already processed instructions in the current block.