[NFC][CLANG] Fix uninitialized scalar field issues

[steakhal]

Speaking of the StaticAnalyzer, I think setting some default value is a fair point, and I think it's a good practice.
In our case, we initialized that field from the registration function, like this:

void ento::registerMoveChecker(CheckerManager &mgr) {
  MoveChecker *chk = mgr.registerChecker<MoveChecker>();
  chk->setAggressiveness(
      mgr.getAnalyzerOptions().getCheckerStringOption(chk, "WarnOn"), mgr); // <--- initialized here
}

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

187

By looking at the default value of the Move.WarnOn checker option here, we should probably initialize this field to the default value matching the default value of the config.

[Manna]

Thank you @steakhal for reviews. I have addressed review comment.

[erichkeane]

CFE changes look fine to me!

[Manna]

In D150744#4350048, @steakhal wrote:

In our case, we initialized that field from the registration function, like this:

void ento::registerMoveChecker(CheckerManager &mgr) {
  MoveChecker *chk = mgr.registerChecker<MoveChecker>();
  chk->setAggressiveness(
      mgr.getAnalyzerOptions().getCheckerStringOption(chk, "WarnOn"), mgr); // <--- initialized here
}

Yes, While investigating other Coverity bugs about uninitialized scaler fields in StaticAnalyzer, i noticed about the registration function.

[Manna]

In D150744#4350198, @erichkeane wrote:

CFE changes look fine to me!

Thank you @erichkeane for reviews!

[Manna]

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

187

void setAggressiveness(StringRef Str, CheckerManager &Mgr) {
    Aggressiveness =
        llvm::StringSwitch<AggressivenessKind>(Str)
            .Case("KnownsOnly", AK_KnownsOnly)
            .Case("KnownsAndLocals", AK_KnownsAndLocals)
            .Case("All", AK_All)
            .Default(AK_Invalid);

    if (Aggressiveness == AK_Invalid)
      Mgr.reportInvalidCheckerOptionValue(this, "WarnOn",
          "either \"KnownsOnly\", \"KnownsAndLocals\" or \"All\" string value");

By looking at the codes above, my understanding was that default value is AK_Invalid. Thank you @steakhal for the pointer and the explanation.

[Manna]

@steakhal, do you have any more concerns with StaticAnalyzer changes?

[tahonermann]

I'm not opposed to these changes, but please note that these changes mean is will no longer be possible to use ubsan to discover when these data members are used before having been assigned an appropriate value. That is only a concern when an appropriate value would differ from the member initializers added via this change. I haven't reviewed the changes in depth, so I don't know if there are any such cases for which such a concern is applicable.

[shafik]

clang/include/clang/Parse/Parser.h

1190

@tahonermann I feel like we should have a default member initializer for any member that by default is not initialized. I realize in this case there currently should be no way for this to be bot initialized but that may not stay true and I don't believe there will be a penalty for doing this since it is initialized in the mem-initializer-list and therefore the default init should be omitted when calling the constructor.

[tahonermann]

clang/include/clang/Parse/Parser.h

1190

I think that is very reasonable and I'm not opposed to such a policy. Per my other comment, the trade off to always initializing is that a default initializer can prevent use of tools like ubsan to discover when an appropriate (presumably non-default) value has not been assigned. I suspect (but have no data to draw on) that adding a default member initializer prevents more bugs than would be found by tools like ubsan detecting use of an uninitialized data member that should have a (non-default) assigned value.

I'm definitely in favor of using default member initializers over mem-initializer-list!

[Manna]

clang/include/clang/Parse/Parser.h

1190

since it is initialized in the mem-initializer-list and therefore the default init should be omitted when calling the constructor.

Agreed. That is motivated me to add default initialization.

he trade off to always initializing is that a default initializer can prevent use of tools like ubsan to discover when an appropriate (presumably non-default) value has not been assigned. I suspect (but have no data to draw on) that adding a default member initializer prevents more bugs than would be found by tools like ubsan detecting use of an uninitialized data member that should have a (non-default) assigned value.

Thanks @tahonermann for the information! i was not aware of this ubsan tool.

@shafik and @tahonermann, thank you for your comments. Do you think we should hold off these changes for merging?

[schittir]

@tahonermann @shafik - is it ok to land this patch?

[tahonermann]

is it ok to land this patch?

No objection from me!

[Manna]

Thank you everyone for reviews!

clang/include/clang/Analysis/Analyses/Consumed.h

public:
     bool isBackEdgeTarget(const CFGBlock *Block);
   };
 
   /// A class that handles the analysis of uniqueness violations.
   class ConsumedAnalyzer {
     ConsumedBlockInfo BlockInfo;
     std::unique_ptr<ConsumedStateMap> CurrStates;
 
-    ConsumedState ExpectedReturnState;
+    ConsumedState ExpectedReturnState = CS_None;
 
     void determineExpectedReturnState(AnalysisDeclContext &AC,
                                       const FunctionDecl *D);
     bool splitState(const CFGBlock *CurrBlock,
                     const ConsumedStmtVisitor &Visitor);
 
   public:
     ConsumedWarningsHandlerBase &WarningsHandler;

clang/include/clang/Parse/Parser.h

public:
   void ExitScope();
 
   /// Re-enter the template scopes for a declaration that might be a template.
   unsigned ReenterTemplateScopes(MultiParseScope &S, Decl *D);
 
 private:
   /// RAII object used to modify the scope flags for the current scope.
   class ParseScopeFlags {
     Scope *CurScope;
-    unsigned OldFlags;
+    unsigned OldFlags = 0;
     ParseScopeFlags(const ParseScopeFlags &) = delete;
     void operator=(const ParseScopeFlags &) = delete;
 
   public:
     ParseScopeFlags(Parser *Self, unsigned ScopeFlags, bool ManageFlags = true);
     ~ParseScopeFlags();
   };
 

clang/lib/AST/ExprConstant.cpp

struct IgnoreSideEffectsRAII {
     ~IgnoreSideEffectsRAII() { Info.EvalMode = OldMode; }
   };
 
   /// RAII object used to optionally suppress diagnostics and side-effects from
   /// a speculative evaluation.
   class SpeculativeEvaluationRAII {
     EvalInfo *Info = nullptr;
     Expr::EvalStatus OldStatus;
-    unsigned OldSpeculativeEvaluationDepth;
+    unsigned OldSpeculativeEvaluationDepth = 0;
 
     void moveFromAndCancel(SpeculativeEvaluationRAII &&Other) {
       Info = Other.Info;
       OldStatus = Other.OldStatus;
       OldSpeculativeEvaluationDepth = Other.OldSpeculativeEvaluationDepth;
       Other.Info = nullptr;
     }
 

clang/lib/CodeGen/CGOpenMPRuntime.h

 };
 
 class CGOpenMPRuntime {
 public:
   /// Allows to disable automatic handling of functions used in target regions
   /// as those marked as `omp declare target`.
   class DisableAutoDeclareTargetRAII {
     CodeGenModule &CGM;
-    bool SavedShouldMarkAsGlobal;
+    bool SavedShouldMarkAsGlobal = false;
 
   public:
     DisableAutoDeclareTargetRAII(CodeGenModule &CGM);
     ~DisableAutoDeclareTargetRAII();
   };
 
   /// Manages list of nontemporal decls for the specified directive.
   class NontemporalDeclsRAII {

clang/lib/CodeGen/ConstantEmitter.h

private:
   /// Whether the constant-emission failed.
   bool Failed = false;
 
   /// Whether we're in a constant context.
   bool InConstantContext = false;
 
   /// The AST address space where this (non-abstract) initializer is going.
   /// Used for generating appropriate placeholders.
-  LangAS DestAddressSpace;
+  LangAS DestAddressSpace = LangAS::Default;
 
   llvm::SmallVector<std::pair<llvm::Constant *, llvm::GlobalVariable*>, 4>
     PlaceholderAddresses;
 
 public:
   ConstantEmitter(CodeGenModule &CGM, CodeGenFunction *CGF = nullptr)
     : CGM(CGM), CGF(CGF) {}
 

clang/lib/Serialization/ASTReaderDecl.cpp

class ASTDeclReader : public DeclVisitor<ASTDeclReader, void> {
     ASTRecordReader &Record;
     ASTReader::RecordLocation Loc;
     const DeclID ThisDeclID;
     const SourceLocation ThisDeclLoc;
 
     using RecordData = ASTReader::RecordData;
 
     TypeID DeferredTypeID = 0;
-    unsigned AnonymousDeclNumber;
+    unsigned AnonymousDeclNumber = 0;
     GlobalDeclID NamedDeclForTagDecl = 0;
     IdentifierInfo *TypedefNameForLinkage = nullptr;
 
     bool HasPendingBody = false;
 
     ///A flag to carry the information for a decl from the entity is
     /// used. We use it to delay the marking of the canonical decl as used until
     /// the entire declaration is deserialized and merged.

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

private:
     const MemRegion *Region;
     // The class of the tracked object.
     const CXXRecordDecl *RD;
     // How exactly the object was misused.
     const MisuseKind MK;
     bool Found;
   };
 
-  AggressivenessKind Aggressiveness;
+  AggressivenessKind Aggressiveness = AK_KnownsAndLocals;
 
 public:
   void setAggressiveness(StringRef Str, CheckerManager &Mgr) {
     Aggressiveness =
         llvm::StringSwitch<AggressivenessKind>(Str)
             .Case("KnownsOnly", AK_KnownsOnly)
             .Case("KnownsAndLocals", AK_KnownsAndLocals)
             .Case("All", AK_All)

clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp

const CallDescriptionMap<FnCheck> Callbacks = {
     {{{"std", "search_n"}, 4}, &STLAlgorithmModeling::evalFind},
     {{{"std", "search_n"}, 5}, &STLAlgorithmModeling::evalFind},
     {{{"std", "search_n"}, 6}, &STLAlgorithmModeling::evalFind},
   };
 
 public:
   STLAlgorithmModeling() = default;
 
-  bool AggressiveStdFindModeling;
+  bool AggressiveStdFindModeling = false;
 
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
 }; //
 
 bool STLAlgorithmModeling::evalCall(const CallEvent &Call,
                                     CheckerContext &C) const {
   const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)