[analyzer][NFC] Fix some incorrect uses of -analyzer-config options

[Szelethus]

Summary

I'm in the process of refactoring AnalyzerOptions. The main motivation behind here is to emit warnings if an invalid -analyzer-config option is given from the command line, and be able to list them all.

In this patch, I found some flags that should've been used as checker options, or have absolutely no mention of in AnalyzerOptions, or are nonexistent.

• NonLocalizedStringChecker now uses its "AggressiveReport" flag as a checker option
• lib/StaticAnalyzer/Frontend/ModelInjector.cpp now accesses the "model-path" option through a getter in AnalyzerOptions
• -analyzer-config path-diagnostics-alternate=false is not a thing, I removed it,
• lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp and lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h are weird, they actually only contain an option getter. I deleted them, and fixed RetainCountChecker to get it's "leak-diagnostics-reference-allocation" option as a checker option,
• "region-store-small-struct-limit" has a proper getter now.

[NoQ]

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

1401–1403

Nice :)

I'll poke Devin about this, please wait on me before committing.

[Szelethus]

Added two more fixes, and added this to the summary:

• lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp and lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h are weird, they actually only contain an option getter. I deleted them, and fixed RetainCountChecker to get it's "leak-diagnostics-reference-allocation" option as a checker option,
• "region-store-small-struct-limit" has a proper getter now.

[Szelethus]

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

1401–1403

I added a similar change to RetainCountChecker, it takes its "leak-diagnostics-reference-allocation" as a checker option. Mention that too I guess.

[Szelethus]

RetainCountChecker now gets it's option when registering, like every other checker.

RetainCountChecker stored a reference to AnalyzerOptions, evaluated an option during construction, and one later down the line. This hid an interesting problem when I made IncludeAllocationLine a checker-specific option: since checkers get their names after their construction, it looked for -analyzer-config :IncludeAllocationLine=<val> instead of -analyzer-config osx.cocoa.RetainCount:IncludeAllocationLine=<val>.

Another thing that helped this issue hide so well is that this option has no test associated with it, but that's an unrelated observation.

Followup patches of this refactoring effort will in part focus on catching incorrect uses of AnalyzerOptions like this one.

[george.karpenkov]

Minor nitpicks, otherwise makes sense!

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

756

The comment seems completely distinct from the function name.

758

Comment?

[Szelethus]

• Added description as @george.karpenkov mentioned.

Thanks! I guess it's up to @NoQ whether he and @dcoughlin are okay with making some options checker options.

[Szelethus]

@NoQ did you have time to ping Devin about this? (Sorry for the early ping, but this patch is blocking a lot of my other patches.)

[NoQ]

Yup, seems fine, please feel free to land! The AggressiveReport option looks unused, we should probably remove it. Sry it took so long><

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

private:
   Optional<StringRef> CTUIndexName;
 
   /// \sa naiveCTUEnabled
   Optional<bool> NaiveCTU;
 
   /// \sa shouldElideConstructors
   Optional<bool> ElideConstructors;
 
+  /// \sa getModelPath
+  Optional<StringRef> ModelPath;
+
+  /// \sa getRegionStoreSmallStructLimit
+  Optional<unsigned> RegionStoreSmallStructLimit;
 
   /// A helper function that retrieves option for a given full-qualified
   /// checker name.
   /// Options for checkers can be specified via 'analyzer-config' command-line
   /// option.
   /// Example:
   /// @code-analyzer-config unix.Malloc:OptionName=CheckerOptionValue @endcode
   /// or @code-analyzer-config unix:OptionName=GroupOptionValue @endcode
public:
   bool naiveCTUEnabled();
 
   /// Returns true if elidable C++ copy-constructors and move-constructors
   /// should be actually elided during analysis. Both behaviors are allowed
   /// by the C++ standard, and the analyzer, like CodeGen, defaults to eliding.
   /// Starting with C++17 some elisions become mandatory, and in these cases
   /// the option will be ignored.
   bool shouldElideConstructors();
+
+  /// The largest number of fields a struct can have and still be
+  /// considered "small".
+  ///
+  /// This is currently used to decide whether or not it is worth "forcing" a
+  /// LazyCompoundVal on bind.
+  ///
+  /// This is controlled by 'region-store-small-struct-limit' option.
+  /// To disable all small-struct-dependent behavior, set the option to "0".
+  unsigned getRegionStoreSmallStructLimit();
+
+  /// The analyzer can inline an alternative implementation written in C at the
+  /// call site if the called function's body is not available. This is a path
+  /// where to look for those alternative implementations (called models)
+  StringRef getModelPath();
 };
 
 using AnalyzerOptionsRef = IntrusiveRefCntPtr<AnalyzerOptions>;
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_ANALYZEROPTIONS_H

lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h

-//=--- AllocationDiagnostics.h - Config options for allocation diags *- C++ -*-//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-// Declares the configuration functions for leaks/allocation diagnostics.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONDIAGNOSTICS_H
-#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONDIAGNOSTICS_H
-
-#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
-
-namespace clang { namespace ento {
-
-/// Returns true if leak diagnostics should directly reference
-/// the allocatin site (where possible).
-///
-/// The default is false.
-///
-bool shouldIncludeAllocationSiteInLeakDiagnostics(AnalyzerOptions &AOpts);
-
-}}
-
-#endif
-

lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp

-//=- AllocationDiagnostics.cpp - Config options for allocation diags *- C++ -*-//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-// Declares the configuration functions for leaks/allocation diagnostics.
-//
-//===--------------------------
-
-#include "AllocationDiagnostics.h"
-
-namespace clang {
-namespace ento {
-
-bool shouldIncludeAllocationSiteInLeakDiagnostics(AnalyzerOptions &AOpts) {
-  return AOpts.getBooleanOption("leak-diagnostics-reference-allocation",
-                                false);
-}
-
-}}

lib/StaticAnalyzer/Checkers/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangStaticAnalyzerCheckers
-  AllocationDiagnostics.cpp
   AnalysisOrderChecker.cpp
   AnalyzerStatsChecker.cpp
   ArrayBoundChecker.cpp
   ArrayBoundCheckerV2.cpp
   BasicObjCFoundationChecks.cpp
   BlockInCriticalSectionChecker.cpp
   BoolAssignmentChecker.cpp
   BuiltinFunctionChecker.cpp

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

 //===----------------------------------------------------------------------===//
 // Checker registration.
 //===----------------------------------------------------------------------===//
 
 void ento::registerNonLocalizedStringChecker(CheckerManager &mgr) {
   NonLocalizedStringChecker *checker =
       mgr.registerChecker<NonLocalizedStringChecker>();
   checker->IsAggressive =
-      mgr.getAnalyzerOptions().getBooleanOption("AggressiveReport", false);
+      mgr.getAnalyzerOptions().getBooleanOption("AggressiveReport", false,
+                                                checker);
 }
 
 void ento::registerEmptyLocalizationContextChecker(CheckerManager &mgr) {
   mgr.registerChecker<EmptyLocalizationContextChecker>();
 }
 
 void ento::registerPluralMisuseChecker(CheckerManager &mgr) {
   mgr.registerChecker<PluralMisuseChecker>();
 }

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

 //  a reference count checker for Core Foundation and Cocoa on (Mac OS X).
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_RETAINCOUNTCHECKER_H
 #define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_RETAINCOUNTCHECKER_H
 
 #include "../ClangSACheckers.h"
-#include "../AllocationDiagnostics.h"
 #include "RetainCountDiagnostics.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceManager.h"
class RetainCountChecker
   typedef llvm::DenseMap<SymbolRef, const CheckerProgramPointTag *> SymbolTagMap;
 
   // This map is only used to ensure proper deletion of any allocated tags.
   mutable SymbolTagMap DeadSymbolTags;
 
   mutable std::unique_ptr<RetainSummaryManager> Summaries;
   mutable SummaryLogTy SummaryLog;
 
-  AnalyzerOptions &Options;
   mutable bool ShouldResetSummaryLog;
 
+public:
   /// Optional setting to indicate if leak reports should include
   /// the allocation line.
-  mutable bool IncludeAllocationLine;
+  bool IncludeAllocationLine;
+  bool ShouldCheckOSObjectRetainCount;
 
-public:
-  RetainCountChecker(AnalyzerOptions &Options)
-      : Options(Options), ShouldResetSummaryLog(false),
-        IncludeAllocationLine(
-            shouldIncludeAllocationSiteInLeakDiagnostics(Options)) {}
+  RetainCountChecker() : ShouldResetSummaryLog(false) {}
 
   ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); }
 
-  bool shouldCheckOSObjectRetainCount() const {
-    return Options.getBooleanOption("CheckOSObject", true, this);
-  }
-
   void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
                         ExprEngine &Eng) const {
     // FIXME: This is a hack to make sure the summary log gets cleared between
     // analyses of different code bodies.
     //
     // Why is this necessary? Because a checker's lifetime is tied to a
     // translation unit, but an ExplodedGraph's lifetime is just a code body.
     // Once in a blue moon, a new ExplodedNode will have the same address as an
public:
   }
 
   RetainSummaryManager &getSummaryManager(ASTContext &Ctx) const {
     // FIXME: We don't support ARC being turned on and off during one analysis.
     // (nor, for that matter, do we support changing ASTContexts)
     bool ARCEnabled = (bool)Ctx.getLangOpts().ObjCAutoRefCount;
     if (!Summaries) {
       Summaries.reset(new RetainSummaryManager(
-          Ctx, ARCEnabled, shouldCheckOSObjectRetainCount()));
+          Ctx, ARCEnabled, ShouldCheckOSObjectRetainCount));
     } else {
       assert(Summaries->isARCEnabled() == ARCEnabled);
     }
     return *Summaries;
   }
 
   RetainSummaryManager &getSummaryManager(CheckerContext &C) const {
     return getSummaryManager(C.getASTContext());

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

void RetainCountChecker::printState(raw_ostream &Out, ProgramStateRef State,
   }
 }
 
 //===----------------------------------------------------------------------===//
 // Checker registration.
 //===----------------------------------------------------------------------===//
 
 void ento::registerRetainCountChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<RetainCountChecker>(Mgr.getAnalyzerOptions());
+  auto *Chk = Mgr.registerChecker<RetainCountChecker>();
+
+  AnalyzerOptions &Options = Mgr.getAnalyzerOptions();
+
+  Chk->IncludeAllocationLine = Options.getBooleanOption(
+                           "leak-diagnostics-reference-allocation", false, Chk);
+  Chk->ShouldCheckOSObjectRetainCount = Options.getBooleanOption(
+                                                   "CheckOSObject", true, Chk);
 }

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

 bool AnalyzerOptions::naiveCTUEnabled() {
   if (!NaiveCTU.hasValue()) {
     NaiveCTU = getBooleanOption("experimental-enable-naive-ctu-analysis",
                                 /*Default=*/false);
   }
   return NaiveCTU.getValue();
 }
 
+unsigned AnalyzerOptions::getRegionStoreSmallStructLimit() {
+  if (!RegionStoreSmallStructLimit.hasValue())
+    RegionStoreSmallStructLimit =
+                       getOptionAsInteger("region-store-small-struct-limit", 2);
+  return RegionStoreSmallStructLimit.getValue();
+}
+
 StringRef AnalyzerOptions::getCTUIndexName() {
   if (!CTUIndexName.hasValue())
     CTUIndexName = getOptionAsString("ctu-index-name", "externalFnMap.txt");
   return CTUIndexName.getValue();
 }
+
+StringRef AnalyzerOptions::getModelPath() {
+  if (!ModelPath.hasValue())
+    ModelPath = getOptionAsString("model-path", "");
+  return ModelPath.getValue();
+}

lib/StaticAnalyzer/Core/RegionStore.cpp

 public:
   RegionStoreManager(ProgramStateManager& mgr, const RegionStoreFeatures &f)
     : StoreManager(mgr), Features(f),
       RBFactory(mgr.getAllocator()), CBFactory(mgr.getAllocator()),
       SmallStructLimit(0) {
     if (SubEngine *Eng = StateMgr.getOwningEngine()) {
       AnalyzerOptions &Options = Eng->getAnalysisManager().options;
       SmallStructLimit =
-        Options.getOptionAsInteger("region-store-small-struct-limit", 2);
+        Options.getRegionStoreSmallStructLimit();
     }
   }
 
 
   /// setImplicitDefaultValue - Set the default binding for the provided
   ///  MemRegion to the value implicitly defined for compound literals when
   ///  the value is not specified.
   RegionBindingsRef setImplicitDefaultValue(RegionBindingsConstRef B,

lib/StaticAnalyzer/Frontend/ModelInjector.cpp

void ModelInjector::onBodySynthesis(const NamedDecl *D) {
   // about file name index? Mangled names may not be suitable for that either.
   if (Bodies.count(D->getName()) != 0)
     return;
 
   SourceManager &SM = CI.getSourceManager();
   FileID mainFileID = SM.getMainFileID();
 
   AnalyzerOptionsRef analyzerOpts = CI.getAnalyzerOpts();
-  llvm::StringRef modelPath = analyzerOpts->Config["model-path"];
+  llvm::StringRef modelPath = analyzerOpts->getModelPath();
 
   llvm::SmallString<128> fileName;
 
   if (!modelPath.empty())
     fileName =
         llvm::StringRef(modelPath.str() + "/" + D->getName().str() + ".model");
   else
     fileName = llvm::StringRef(D->getName().str() + ".model");

test/Analysis/analyzer-config.c

 // CHECK-NEXT: eagerly-assume = true
 // CHECK-NEXT: elide-constructors = true
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
-// CHECK-NEXT: leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 25
+// CHECK-NEXT: num-entries = 24

test/Analysis/analyzer-config.cpp

 // CHECK-NEXT: elide-constructors = true
 // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false
 // CHECK-NEXT: exploration_strategy = unexplored_first_queue
 // CHECK-NEXT: faux-bodies = true
 // CHECK-NEXT: graph-trim-interval = 1000
 // CHECK-NEXT: inline-lambdas = true
 // CHECK-NEXT: ipa = dynamic-bifurcate
 // CHECK-NEXT: ipa-always-inline-size = 3
-// CHECK-NEXT: leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 32
+// CHECK-NEXT: num-entries = 31

test/Analysis/cstring-plist.c

 // RUN: rm -f %t
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,unix.Malloc,unix.cstring.NullArg -analyzer-disable-checker=alpha.unix.cstring.OutOfBounds -analyzer-output=plist -analyzer-config path-diagnostics-alternate=false -o %t %s
+// RUN: %clang_analyze_cc1 -fblocks \
+// RUN:   -analyzer-checker=core,unix.Malloc,unix.cstring.NullArg \
+// RUN:   -analyzer-disable-checker=alpha.unix.cstring.OutOfBounds \
+// RUN:   -analyzer-output=plist -o %t %s
 // RUN: FileCheck -input-file %t %s
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
 char *strncpy(char *restrict s1, const char *restrict s2, size_t n);
 
 

test/Analysis/localization-aggressive.m

 // RUN: %clang_cc1 -fblocks -x objective-c-header -emit-pch -o %t.pch %S/Inputs/localization-pch.h
 
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-store=region  -analyzer-checker=optin.osx.cocoa.localizability.NonLocalizedStringChecker -analyzer-checker=optin.osx.cocoa.localizability.EmptyLocalizationContextChecker -include-pch %t.pch -verify  -analyzer-config AggressiveReport=true %s
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-store=region \
+// RUN:   -analyzer-config optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport=true \
+// RUN:   -analyzer-checker=optin.osx.cocoa.localizability.NonLocalizedStringChecker \
+// RUN:   -analyzer-checker=optin.osx.cocoa.localizability.EmptyLocalizationContextChecker \
+// RUN:   -include-pch %t.pch -verify  %s
 
 // These declarations were reduced using Delta-Debugging from Foundation.h
 // on Mac OS X.
 
 #define nil ((id)0)
 #define NSLocalizedString(key, comment)                                        \
   [[NSBundle mainBundle] localizedStringForKey:(key) value:@"" table:nil]
 #define NSLocalizedStringFromTable(key, tbl, comment)                          \