[PATCH] D56632: [analyzer] Track region liveness only through base regions.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Awesome detective work! I glanced over the code, it looks great. I'd love to 
dedicate more time to your liveness-related patches, but university is a thing, 
so finding typos and the like is the best I can do for a while.

Wild thought, would `debug.DumpLiveStmts` be of any use here?




Comment at: test/Analysis/symbol-reaper.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+

Core intentionally left out?



Comment at: test/Analysis/symbol-reaper.cpp:31
+  clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}

N00b question: What does `SYMBOL DEAD` mean here exactly?



Comment at: unittests/StaticAnalyzer/CMakeLists.txt:8
   RegisterCustomCheckersTest.cpp
+  SymbolReaperTest.cpp
   )

Woohoo!


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

https://reviews.llvm.org/D56632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ping^2


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

https://reviews.llvm.org/D51866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56632: [analyzer] Track region liveness only through base regions.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/symbol-reaper.cpp:31
+  clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}

NoQ wrote:
> Szelethus wrote:
> > N00b question: What does `SYMBOL DEAD` mean here exactly?
> It's a warning produced by `clang_analyzer_warnOnDeadSymbol(a.x)` when the 
> value that was in `a.x` (that was there when that function was called) dies. 
> This is an `ExprInspection` utility that was created in order to test 
> `SymbolReaper` more directly. See `symbol-reaper.c` for more such tests.
Oooh right. I thought it's produced by `clang_analyzer_eval(glob);`. Thanks!


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

https://reviews.llvm.org/D56632



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

Overall I think this looks great, thanks! I left some inlines that would be 
nice to fix before commiting, but all of them are minor nits.

Would it be possible for you to commit the clang-formatting and the actual 
logic separately?




Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:393
+  def DeprecatedOrUnsafeBufferHandling : 
Checker<"DeprecatedOrUnsafeBufferHandling">,
+HelpText<"Warn on uses of unsecure or deprecated buffer manipulating 
functions">,
+DescFile<"CheckSecuritySyntaxOnly.cpp">;

Lately we started paying attention to the 80 column limit in `Checkers.td`. 
Could you please break this line?



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:583
+// Use of 'sprintf', 'vsprintf', 'scanf', 'wscanf',
+//'fscanf',
+//'fwscanf', 'vscanf', 'vwscanf', 'vfscanf', 'vfwscanf', 'sscanf',

This is out of place.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:591
+//===--===//
+void WalkAST::checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE,
+const FunctionDecl *FD) {

Could you please add an extra newline?



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:598
 
//===--===//
-
 void WalkAST::checkCall_rand(const CallExpr *CE, const FunctionDecl *FD) {

I think this newline should stay.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:603-610
+  int ArgIndex =
+  llvm::StringSwitch(Name)
+  .Cases("scanf", "wscanf", "vscanf", "vwscanf", 0)
+  .Cases("sprintf", "vsprintf", "fscanf", "fwscanf", "vfscanf",
+ "vfwscanf", "sscanf", "swscanf", "vsscanf", "vswscanf", 1)
+  .Cases("swprintf", "snprintf", "vswprintf", "vsnprintf", "memcpy",
+ "memmove", "memset", "strncpy", "strncat", DEPR_ONLY)

I wonder whether using `CallDescription` would be (way) more efficient here. 
Comparing `IndentifierInfo`s would also be better I guess.

I'm a little unsure about performance implications here, it might not be worth 
the chore to refactor this.



Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:627-630
+  SmallString<128> buf1;
+  SmallString<512> buf2;
+  llvm::raw_svector_ostream out1(buf1);
+  llvm::raw_svector_ostream out2(buf2);

Could you please start these variable names with a capital letter?



Comment at: test/Analysis/security-syntax-checks.m:253
+  FILE *file;
+  sprintf(buf, "a"); // expected-warning{{Call to function 'sprintf' is 
insecure as it does not provide security checks introduced in the C11 standard. 
Replace with analogous functions that support length arguments or provides 
boundary checks such as 'sprintf_s' in case of C11}}
+  scanf("%d", &a); // expected-warning{{Call to function 'scanf' is insecure 
as it does not provide security checks introduced in the C11 standard. Replace 
with analogous functions that support length arguments or provides boundary 
checks such as 'scanf_s' in case of C11}}

When using `{{}}`, you actually supply a regex as an argument, and the output 
of the analyzer is matched against it. My point is, could you instead just write
```
// expected-warning{{Call to function 'sprintf' is insecure}}
```
to improve readability?


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

https://reviews.llvm.org/D35068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/security-syntax-checks.m:253
+  FILE *file;
+  sprintf(buf, "a"); // expected-warning{{Call to function 'sprintf' is 
insecure as it does not provide security checks introduced in the C11 standard. 
Replace with analogous functions that support length arguments or provides 
boundary checks such as 'sprintf_s' in case of C11}}
+  scanf("%d", &a); // expected-warning{{Call to function 'scanf' is insecure 
as it does not provide security checks introduced in the C11 standard. Replace 
with analogous functions that support length arguments or provides boundary 
checks such as 'scanf_s' in case of C11}}

Szelethus wrote:
> When using `{{}}`, you actually supply a regex as an argument, and the output 
> of the analyzer is matched against it. My point is, could you instead just 
> write
> ```
> // expected-warning{{Call to function 'sprintf' is insecure}}
> ```
> to improve readability?
Or whatever the shortest string is needed to know whether the expected output 
it there.


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

https://reviews.llvm.org/D35068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

In D35068#811436 , @NoQ wrote:

> I wonder how noisy this check is - did you test it on large codebases? 
> Because these functions are popular, and in many cases it'd be fine to use 
> insecure functions, i wonder if it's worth it to have this check on by 
> default. Like, if it's relatively quiet - it's fine, but if it'd constitute 
> 90% of the analyzer's warnings on popular projects, that'd probably not be 
> fine.




In D35068#1049530 , @george.karpenkov 
wrote:

> @koldaniel Have you evaluated this checker? On which codebases? Were the 
> warnings real security issues, or were they mostly spurious? The code seems 
> fine, but I'm not sure whether it should be in `security` or in `alpha`.


Sorry, didn't read the discussion, there are some fair points in the quoted 
comments.

In D35068#1069880 , @koldaniel wrote:

> I've evaluated this checker on LLVM+Clang, there were only a few (about 15) 
> warnings,  because of the C11 flag check at the beginning of the checker 
> body. However, if this check was removed, number of the warnings would be 
> increased significantly. I wouldn't say the findings were real security 
> issues, most of the warnings were about usages of deprecated functions, which 
> has not been considered unsecure (but which may cause problems if the code is 
> modified in an improper way in the future).


My problem is that LLVM+Clang isn't really a C (neither a C11) project, and I 
think judging this checker on it is a little misleading. Could you please test 
it on some C11 projects? I think tmux uses C11.

In D35068#1361195 , @xazax.hun wrote:

> I think this is quiet coding guideline specific check which is useful for a 
> set of  security critical projects. As this is an opt in kind of check, I 
> think it does no harm to have it upstream.


I do generally agree with this statement, but I'd be more comfortable either 
landing it in alpha or seeing some other results.


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

https://reviews.llvm.org/D35068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ping^3. @NoQ @george.karpenkov Could you please take a look? @xazax.hun Any 
other objections?


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

https://reviews.llvm.org/D51866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I'm working on rebasing my dependency branches on top of trunk, and I'm 
somewhat stuck on this patch, could you lend a hand please?

So, I'm seeing in essence 3 checkers: the class `RetainCountChecker`, that does 
the modeling, and two checkers that are exposed to the users, 
`osx.cocoa.RetainCount` and `osx.OSObjectRetainCount`. While generally the 
solution would be to create a register checker called `RetainCountBase`, that 
is a common base of both of the already registered checkers, it raises a 
serious problem: all of the plist files that say "this error originates from 
checker `osx.cocoa.RetainCount`" will now instead refer to 
`osx.cocoa.RetainCountBase`. Unfortunately, I can't just make both checkers 
have the same name, as how would the user disable one and not the other?

There were other checkers that suffered from this, but those affected very few 
test files, and this one hits around 9, and I know that very serious work is 
being put into it from your part. I'd like to propose 3 solutions, and I'm very 
much open to new ones:

- Deal with the consequences of this, and just correct all plist files to now 
refer to the new base checker.
- Each time a report is emitted from these checkers, create a 
`ProgramPointTag`, and "manually" make sure the emitted checker name doesn't 
change.
- Rename `osx.cocoa.RetainCount` to something else. This one is clearly 
nonsensical, but let's put it here for the sake of completeness.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55424: [analyzer] Supply all checkers with a shouldRegister function

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D55424#1326397 , @NoQ wrote:

> Should we also pass `CheckerManager` into `shouldRegister...`? Or is it 
> entirely useless?


I wouldn't say useless, but I'm struggling to come up with an example where 
registering would depend on non-language dependent reasons. I'm stuck on 
rebasing my patches, so I'll give it a thought whether it's worth the chore to 
change everything (but where I'd like to leave a system for the long term, 
doing extra work shouldn't be a major concern).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55424



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55424: [analyzer] Supply all checkers with a shouldRegister function

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 182721.
Szelethus added a comment.

Rebase.


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

https://reviews.llvm.org/D55424

Files:
  include/clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h
  include/clang/StaticAnalyzer/Core/Checker.h
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
  lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCInstMethSignature.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
  lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp
  lib/StaticAnalyzer/Checkers/GTestChecker.cpp
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp
  lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp
  lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp
  lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
  lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp

[PATCH] D56988: [analyzer][NFC] Supply CheckerRegistry with AnalyzerOptions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, baloghadamsoftware, 
xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

Title says it all.


Repository:
  rC Clang

https://reviews.llvm.org/D56988

Files:
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -40,8 +40,9 @@
 
 CheckerRegistry::CheckerRegistry(ArrayRef plugins,
  DiagnosticsEngine &diags,
+ AnalyzerOptions &AnOpts,
  const LangOptions &LangOpts)
-  : Diags(diags), LangOpts(LangOpts) {
+  : Diags(diags), AnOpts(AnOpts), LangOpts(LangOpts) {
 
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI)\
@@ -106,8 +107,7 @@
   return false;
 }
 
-CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers(
-const AnalyzerOptions &Opts) const {
+CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers() const {
 
   assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
  "In order to efficiently gather checkers, this function expects them "
@@ -116,7 +116,7 @@
   CheckerInfoSet enabledCheckers;
   const auto end = Checkers.cend();
 
-  for (const std::pair &opt : Opts.CheckersControlList) {
+  for (const std::pair &opt : AnOpts.CheckersControlList) {
 // Use a binary search to find the possible start of the package.
 CheckerRegistry::CheckerInfo
 packageInfo(nullptr, nullptr, opt.first, "", "");
@@ -167,13 +167,12 @@
   }
 }
 
-void CheckerRegistry::initializeManager(CheckerManager &checkerMgr,
-const AnalyzerOptions &Opts) const {
+void CheckerRegistry::initializeManager(CheckerManager &checkerMgr) const {
   // Sort checkers for efficient collection.
   llvm::sort(Checkers, checkerNameLT);
 
   // Collect checkers enabled by the options.
-  CheckerInfoSet enabledCheckers = getEnabledCheckers(Opts);
+  CheckerInfoSet enabledCheckers = getEnabledCheckers();
 
   // Initialize the CheckerManager with all enabled checkers.
   for (const auto *i : enabledCheckers) {
@@ -182,9 +181,8 @@
   }
 }
 
-void CheckerRegistry::validateCheckerOptions(
-const AnalyzerOptions &opts) const {
-  for (const auto &config : opts.Config) {
+void CheckerRegistry::validateCheckerOptions() const {
+  for (const auto &config : AnOpts.Config) {
 size_t pos = config.getKey().find(':');
 if (pos == StringRef::npos)
   continue;
@@ -241,13 +239,12 @@
   }
 }
 
-void CheckerRegistry::printList(raw_ostream &out,
-const AnalyzerOptions &opts) const {
+void CheckerRegistry::printList(raw_ostream &out) const {
   // Sort checkers for efficient collection.
   llvm::sort(Checkers, checkerNameLT);
 
   // Collect checkers enabled by the options.
-  CheckerInfoSet enabledCheckers = getEnabledCheckers(opts);
+  CheckerInfoSet enabledCheckers = getEnabledCheckers();
 
   for (const auto *i : enabledCheckers)
 out << i->FullName << '\n';
Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -33,35 +33,36 @@
 DiagnosticsEngine &diags) {
   auto checkerMgr = llvm::make_unique(context, opts);
 
-  CheckerRegistry allCheckers(plugins, diags, context.getLangOpts());
+  CheckerRegistry allCheckers(plugins, diags, opts, context.getLangOpts());
 
   for (const auto &Fn : checkerRegistrationFns)
 Fn(allCheckers);
 
-  allCheckers.initializeManager(*checkerMgr, opts);
-  allCheckers.validateCheckerOptions(opts);
+  allCheckers.initializeManager(*checkerMgr);
+  allCheckers.validateCheckerOptions();
   checkerMgr->finishedCheckerRegistration();
 
   return checkerMgr;
 }
 
 void ento::printCheckerHelp(raw_ostream &out, ArrayRef plugins,
+AnalyzerOptions &anopts,
 DiagnosticsEngine &diags,
 const LangOptions &langOpts) {
   out << "OVERVIEW: Clang Static Analyzer Checkers List\n\n";
   out << "USAGE: -analyzer-checker \n\n";
 
-  CheckerRegistry(plugins, diags, langOpts).printHelp(out);
+  CheckerRegistry(plugins, diags, anopts, langOpts).printHelp(out);
 }
 
 void ento::printEnabledCheckerLi

[PATCH] D56989: [analyzer][NFC] Fully initialize CheckerRegistry in by the end of construction

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs, 
baloghadamsoftware.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, mgrang, szepet, whisperity.
Szelethus retitled this revision from "[analyzer][NFC] Fully initialize 
CheckerRegistry in by the end of construction, make all methods const" to 
"[analyzer][NFC] Fully initialize CheckerRegistry in by the end of 
construction".

One of the bugs that I left in in my dependency patch D54438 
 was luckily discovered thanks to 
@george.karpenkov commiting a patch a couple days before I attempted to commit: 
Namely, the rigid structure of how the user can enable/disable checkers wasn't 
respected, and chaos ensued when a dependency was disabled, but the checker 
that was depending on it was enabled.

This patch moves all the code that modifies the state of `CheckerRegistry` into 
it's constructor, ensuring that once constructed, it'll be in its final state.

The important thing here is that I added a new enum to `CheckerInfo`, so we can 
easily track whether the check is explicitly enabled, explicitly disabled, or 
isn't specified in this regard. Checkers belonging in the latter category may 
be implicitly enabled through dependencies in the followup patch.


Repository:
  rC Clang

https://reviews.llvm.org/D56989

Files:
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -38,12 +38,66 @@
   return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
+static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
+  const CheckerRegistry::CheckerInfo &b) {
+  return a.FullName < b.FullName;
+}
+
+static constexpr char PackageSeparator = '.';
+
+static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
+StringRef packageName) {
+  // Does the checker's full name have the package as a prefix?
+  if (!checker.FullName.startswith(packageName))
+return false;
+
+  // Is the package actually just the name of a specific checker?
+  if (checker.FullName.size() == packageName.size())
+return true;
+
+  // Is the checker in the package (or a subpackage)?
+  if (checker.FullName[packageName.size()] == PackageSeparator)
+return true;
+
+  return false;
+}
+
+CheckerRegistry::CheckerInfoListRange
+CheckerRegistry::getMutableCheckersForCmdLineArg(StringRef CmdLineArg) {
+
+  assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
+ "In order to efficiently gather checkers, this function expects them "
+ "to be already sorted!");
+
+  // Use a binary search to find the possible start of the package.
+  CheckerRegistry::CheckerInfo
+  packageInfo(nullptr, nullptr, CmdLineArg, "", "");
+  auto it = std::lower_bound(Checkers.begin(), Checkers.end(),
+ packageInfo, checkerNameLT);
+
+  if (!isInPackage(*it, CmdLineArg))
+return { Checkers.end(), Checkers.end() };
+
+  // See how large the package is.
+  // If the package doesn't exist, assume the option refers to a single
+  // checker.
+  size_t size = 1;
+  llvm::StringMap::const_iterator packageSize =
+  Packages.find(CmdLineArg);
+
+  if (packageSize != Packages.end())
+size = packageSize->getValue();
+
+  return { it, it + size };
+}
+
 CheckerRegistry::CheckerRegistry(ArrayRef plugins,
  DiagnosticsEngine &diags,
  AnalyzerOptions &AnOpts,
  const LangOptions &LangOpts)
   : Diags(diags), AnOpts(AnOpts), LangOpts(LangOpts) {
 
+  // Register builtin checkers.
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI)\
   addChecker(register##CLASS, shouldRegister##CLASS, FULLNAME, HELPTEXT,   \
@@ -52,6 +106,7 @@
 #undef CHECKER
 #undef GET_CHECKERS
 
+  // Register checkers from plugins.
   for (ArrayRef::iterator i = plugins.begin(), e = plugins.end();
i != e; ++i) {
 // Get access to the plugin.
@@ -81,73 +136,35 @@
 if (registerPluginCheckers)
   registerPluginCheckers(*this);
   }
-}
-
-static constexpr char PackageSeparator = '.';
 
-static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
-  const CheckerRegistry::CheckerInfo &b) {
-  return a.FullName < b.FullName;
-}
-
-static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
-StringRef packageName) {
-  // Does the checker's full name have the package as a prefix?
-  if (!checker.FullName.startswith(packageName))
-return false;
+  // Sort checkers for eff

[PATCH] D56989: [analyzer][NFC] Fully initialize CheckerRegistry in by the end of construction

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 182724.
Szelethus added a comment.

Remove `mutable` specifiers from all fields of `CheckerRegistry`.


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

https://reviews.llvm.org/D56989

Files:
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -38,12 +38,66 @@
   return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
+static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
+  const CheckerRegistry::CheckerInfo &b) {
+  return a.FullName < b.FullName;
+}
+
+static constexpr char PackageSeparator = '.';
+
+static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
+StringRef packageName) {
+  // Does the checker's full name have the package as a prefix?
+  if (!checker.FullName.startswith(packageName))
+return false;
+
+  // Is the package actually just the name of a specific checker?
+  if (checker.FullName.size() == packageName.size())
+return true;
+
+  // Is the checker in the package (or a subpackage)?
+  if (checker.FullName[packageName.size()] == PackageSeparator)
+return true;
+
+  return false;
+}
+
+CheckerRegistry::CheckerInfoListRange
+CheckerRegistry::getMutableCheckersForCmdLineArg(StringRef CmdLineArg) {
+
+  assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
+ "In order to efficiently gather checkers, this function expects them "
+ "to be already sorted!");
+
+  // Use a binary search to find the possible start of the package.
+  CheckerRegistry::CheckerInfo
+  packageInfo(nullptr, nullptr, CmdLineArg, "", "");
+  auto it = std::lower_bound(Checkers.begin(), Checkers.end(),
+ packageInfo, checkerNameLT);
+
+  if (!isInPackage(*it, CmdLineArg))
+return { Checkers.end(), Checkers.end() };
+
+  // See how large the package is.
+  // If the package doesn't exist, assume the option refers to a single
+  // checker.
+  size_t size = 1;
+  llvm::StringMap::const_iterator packageSize =
+  Packages.find(CmdLineArg);
+
+  if (packageSize != Packages.end())
+size = packageSize->getValue();
+
+  return { it, it + size };
+}
+
 CheckerRegistry::CheckerRegistry(ArrayRef plugins,
  DiagnosticsEngine &diags,
  AnalyzerOptions &AnOpts,
  const LangOptions &LangOpts)
   : Diags(diags), AnOpts(AnOpts), LangOpts(LangOpts) {
 
+  // Register builtin checkers.
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI)\
   addChecker(register##CLASS, shouldRegister##CLASS, FULLNAME, HELPTEXT,   \
@@ -52,6 +106,7 @@
 #undef CHECKER
 #undef GET_CHECKERS
 
+  // Register checkers from plugins.
   for (ArrayRef::iterator i = plugins.begin(), e = plugins.end();
i != e; ++i) {
 // Get access to the plugin.
@@ -81,73 +136,38 @@
 if (registerPluginCheckers)
   registerPluginCheckers(*this);
   }
-}
-
-static constexpr char PackageSeparator = '.';
-
-static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
-  const CheckerRegistry::CheckerInfo &b) {
-  return a.FullName < b.FullName;
-}
 
-static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
-StringRef packageName) {
-  // Does the checker's full name have the package as a prefix?
-  if (!checker.FullName.startswith(packageName))
-return false;
+  // Sort checkers for efficient collection.
+  // FIXME: Alphabetical sort puts 'experimental' in the middle.
+  // Would it be better to name it '~experimental' or something else
+  // that's ASCIIbetically last?
+  llvm::sort(Checkers, checkerNameLT);
 
-  // Is the package actually just the name of a specific checker?
-  if (checker.FullName.size() == packageName.size())
-return true;
+  // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
+  // command line.
+  for (const std::pair &opt : AnOpts.CheckersControlList) {
+CheckerInfoListRange checkersForCmdLineArg =
+ getMutableCheckersForCmdLineArg(opt.first);
 
-  // Is the checker in the package (or a subpackage)?
-  if (checker.FullName[packageName.size()] == PackageSeparator)
-return true;
+if (checkersForCmdLineArg.begin() == checkersForCmdLineArg.end()) {
+  Diags.Report(diag::err_unknown_analyzer_checker) << opt.first;
+  Diags.Report(diag::note_suggest_disabling_all_checkers);
+}
 
-  return false;
+for (CheckerInfo &checker : checkersForCmdLineArg) {
+  checker.State = opt.second ? StateFromCmdLine::State_Enabled :
+   

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 182726.
Szelethus added a comment.

- Rebase
- Resolve the issue mentioned above by not enabling checkers that has any of 
their dependencies explicitly disabled
- Introduce `osx.RetainCountBase` to "solve" the issue mentioned in 
D55400#1364683 , but the test files 
are still untouched. This leads to 9 lit test failures, but since this patch is 
ancient, I'd rather update it now and follow up with another, final version 
later.


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

https://reviews.llvm.org/D54438

Files:
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
  test/Analysis/NewDelete+MismatchedDeallocator_intersections.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/test-separate-retaincount.cpp
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -90,6 +90,17 @@
   .str();
 }
 
+static void printChecker(llvm::raw_ostream &OS, const Record &R) {
+OS << "CHECKER(" << "\"";
+OS.write_escaped(getCheckerFullName(&R)) << "\", ";
+OS << R.getName() << ", ";
+OS << "\"";
+OS.write_escaped(getStringValue(R, "HelpText")) << "\", ";
+OS << "\"";
+OS.write_escaped(getCheckerDocs(R));
+OS << "\"";
+}
+
 namespace clang {
 void EmitClangSACheckers(RecordKeeper &Records, raw_ostream &OS) {
   std::vector checkers = Records.getAllDerivedDefinitions("Checker");
@@ -100,7 +111,12 @@
   OS << "// This file is automatically generated. Do not edit this file by "
 "hand.\n";
 
-  OS << "\n#ifdef GET_PACKAGES\n";
+  // Emit packages.
+  //
+  // PACKAGE(PACKAGENAME)
+  //   - PACKAGENAME: The name of the package.
+  OS << "\n"
+"#ifdef GET_PACKAGES\n";
   {
 SortedRecords sortedPackages;
 for (unsigned i = 0, e = packages.size(); i != e; ++i)
@@ -115,22 +131,50 @@
   OS << ")\n";
 }
   }
-  OS << "#endif // GET_PACKAGES\n\n";
-  
-  OS << "\n#ifdef GET_CHECKERS\n";
-  for (unsigned i = 0, e = checkers.size(); i != e; ++i) {
-const Record &R = *checkers[i];
-
-OS << "CHECKER(" << "\"";
-OS.write_escaped(getCheckerFullName(&R)) << "\", ";
-OS << R.getName() << ", ";
-OS << "\"";
-OS.write_escaped(getStringValue(R, "HelpText")) << "\", ";
-OS << "\"";
-OS.write_escaped(getCheckerDocs(R));
-OS << "\"";
+  OS << "#endif // GET_PACKAGES\n"
+"\n";
+
+  // Emit checkers.
+  //
+  // CHECKER(FULLNAME, CLASS, HELPTEXT)
+  //   - FULLNAME: The full name of the checker, including packages, e.g.:
+  //   alpha.cplusplus.UninitializedObject
+  //   - CLASS: The name of the checker, with "Checker" appended, e.g.:
+  //UninitializedObjectChecker
+  //   - HELPTEXT: The description of the checker.
+  OS << "\n"
+"#ifdef GET_CHECKERS\n"
+"\n";
+  for (const Record *checker : checkers) {
+printChecker(OS, *checker);
 OS << ")\n";
   }
-  OS << "#endif // GET_CHECKERS\n\n";
+  OS << "\n"
+"#endif // GET_CHECKERS\n"
+"\n";
+
+  // Emit dependencies.
+  //
+  // CHECKER_DEPENDENCY(FULLNAME, DEPENDENCY)
+  //   - FULLNAME: The full name of the checker that depends on another checker.
+  //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
+  OS << "\n"
+"#ifdef GET_CHECKER_DEPENDENCIES\n";
+  for (const Record *checker : checkers) {
+if (checker->isValueUnset("Dependencies"))
+  continue;
+
+for (const Record *Dependency :
+checker->getValueAsListOfDefs("Dependencies")) {
+  OS << "CHECKER_DEPENDENCY(";
+  OS << '\"';
+  OS.write_escaped(getCheckerFullName(checker)) << "\", ";
+  OS << '\"';
+  OS.write_escaped(getCheckerFullName(Dependency)) << '\"';
+  OS << ")\n";
+}
+  }
+  OS << "\n"
+"#endif // GET_CHECKER_DEPENDENCIES\n";
 }
 } // end namespace clang
Index: 

[PATCH] D55429: [analyzer] Add CheckerManager::getChecker, make sure that a registry function registers no more than 1 checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 182727.
Szelethus added a comment.

Rebase.


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

https://reviews.llvm.org/D55429

Files:
  include/clang/StaticAnalyzer/Core/CheckerManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  test/Analysis/analyzer-checker-config.c
  test/Analysis/free.c
  test/Analysis/outofbound.c
  test/Analysis/undef-buffers.c

Index: test/Analysis/undef-buffers.c
===
--- test/Analysis/undef-buffers.c
+++ test/Analysis/undef-buffers.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,core.uninitialized -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=core.uninitialized \
+// RUN:   -analyzer-config unix:Optimistic=true
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: test/Analysis/outofbound.c
===
--- test/Analysis/outofbound.c
+++ test/Analysis/outofbound.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,unix,alpha.security.ArrayBound -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=alpha.security.ArrayBound \
+// RUN:   -analyzer-config unix:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
Index: test/Analysis/free.c
===
--- test/Analysis/free.c
+++ test/Analysis/free.c
@@ -1,5 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc
+//
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void free(void *);
 void *alloca(size_t);
Index: test/Analysis/analyzer-checker-config.c
===
--- test/Analysis/analyzer-checker-config.c
+++ test/Analysis/analyzer-checker-config.c
@@ -4,7 +4,7 @@
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config ..:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unrelated:Optimistic=true 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.Malloc:Optimistic=true
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 // Just to test clang is working.
 # foo
Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -409,7 +409,7 @@
 
 #define REGISTER_CHECKER(name) \
   void ento::register##name##Checker(CheckerManager &mgr) {\
-ValistChecker *checker = mgr.registerChecker(); \
+ValistChecker *checker = mgr.getChecker();  \
 checker->ChecksEnabled[ValistChecker::CK_##name] = true;   \
 checker->CheckNames[ValistChecker::CK_##name] = mgr.getCurrentCheckName(); \
   }\
Index: lib/StaticAnalyzer/Checkers/StackAddrEsc

[PATCH] D54429: [analyzer] Creating standard Sphinx documentation

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Cheers, this is as good as it gets! IRL we also mentioned making a unipage for 
each checker which would be awesome (and would require a lot of tedious work), 
but as a start, I'd much prefer browsing through this doc than the current one. 
Very much appreciated!

I won't formally accept this to make it stand out a little more.




Comment at: docs/analyzer/checkers.rst:2003
+View Exploded Graphs using GraphViz.
+

While I would argue very strongly against the current website's every effort at 
hiding implicit checkers, when we deliberately call this site a documentation 
site, I definitely think that we should most include them here.

Although, don't sweat it too much just yet, while the structure is still being 
decided upon.


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

https://reviews.llvm.org/D54429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Yup, I'm sold on that.


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

https://reviews.llvm.org/D35068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Poor wording on my end, sorry about that. Let me clarify.

In D55400#1366684 , @george.karpenkov 
wrote:

> > Deal with the consequences of this, and just correct all plist files to now 
> > refer to the new base checker.
>
> What does it mean?


When a report is emitted in the plist output, it'll also that which checker is 
"responsible" for that warning. I vaguely remember reading that this isn't used 
that much in your application, but we use this information extensively here.

Now, what I'd propose, is register a new, `osx.RetainCountBase` checker, and 
make both of the already existing checkers depend on that. Long story short, 
this will make the name of the checker that is listed in that plist entry 
`osx.RetainCountBase`, as opposed to `osx.cocoa.RetainCount`, but this doesn't 
sound that illogical, considering that both of those checkers emit the same 
checker name anyways (meaning, that if one report originates from 
`osx.OSObjectRetainCount` and one from `osx.cocoa.RetainCount`, both reports 
will be listed as they originated from `osx.cocoa.RetainCount`).

> 
> 
>> Each time a report is emitted from these checkers, create a ProgramPointTag, 
>> and "manually" make sure the emitted checker name doesn't change.
> 
> I'm not sure what do you propose exactly, but sounds pretty bad.

It does. I think `IvarInvalidation` uses something like that, and it's both 
unsustainable and very ugly.

>> Rename osx.cocoa.RetainCount to something else. This one is clearly 
>> nonsensical, but let's put it here for the sake of completeness.
> 
> I don't think we can rename the old checker, as older Xcode versions have 
> "osx.cocoa.RetainCount" hardcoded in them.

Yea, this one makes little sense. Should've just left this one out really.

> TBH I'm not really sold on the way we "bundle" multiple checkers (from the 
> tablegen) into a single class. [...] essentially the checker name is defined 
> by the class which was registered by the tablegen first (which should not 
> even be visible, and should be an internal implementation detail)
>  Do you have better proposals on how, conceptually, grouped checkers should 
> be organized?

Yup, I've had the same thought before. I would argue strongly for registering 
them in the tblgen file, because a clear dependency graph can be easily 
established this way. What would be neat, however, is to also a new bit field 
to `Checker` in `CheckerBase.td`, which if set, will make the checker hidden in 
the `-analyzer-checker-help` list. There are many checkers that should be there 
anyways, implicit checker come to my mind first.

> For one, that means options don't work at all, and [...]

Hmmm, does this mess with options that bad? Could you please clarify?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

If you don't mind, I'd prefer to finally get over this patch. I'll commit on 
the 31st, but will wait for any potential feedback 'til then.


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

https://reviews.llvm.org/D51866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D55400#1367046 , @george.karpenkov 
wrote:

> > Hmmm, does this mess with options that bad? Could you please clarify?
>
> `registerChecker` gets-or-creates a checker object. A checker name (used for 
> getting the options) is set the first time it's created.
>  The checker which was created first "wins" and gets to name the resulting 
> checker.
>  In practice it basically means that options and checkers reusing the same 
> class do not work.
>  Do you have better ideas on how this could be arranged?


Sure, in fact its already implemented and got accepted by @NoQ, I just didnt 
have the time to commit and fine tune :)

The solution is essentially to create `CheckerManager::getChecker` alongside 
`registerChecker`, and making them in order assert if the checker is 
unregistered, and assert when the checked is already registered. In an earlier 
patch, I implement handling of dependencies on a higher level, essentially 
making `CheckerRegistry` take care of this, instead of making checker registry 
functions register multiple checkers, or, like in this case, register a common 
base. Before the registry function for either RetainCound or 
OSObjectRetainCount is called (which will now invoke `getChecker`), the 
registry function for RetainCountBase will be called, that will register the 
actual checker object.

You can take a look at how this would look like in D54438 
.

Would love to hear your thoughts on this! :)

> I think the current situation is a mess - ideally I would prefer to be able 
> to access the options in the constructor, but we can't even do that,
>  since `registerChecker` sets the checker name and is called after the object 
> has been constructed.
>  It seems that it would only make sense if the checker name is known at the 
> time the checker is constructed: probably the function `registerXChecker` 
> should get it as an argument.

The proposed solution solves this issue as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Hmmm, came across this one in the not too distant future, and always wondered 
how painful that performance hit would be that even Release+Asserts should be 
spared from it. I think 5% performance hit, if asserts are enabled, is an 
acceptable tradeoff, if the assert is crucial.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57062



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

And if we find out that this is far too painful, we can always put it back 
anyways. Doubt that'll happen though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57062



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57230: [analyzer] Toning down invalidation a bit

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus edited reviewers, added: NoQ; removed: dergachev.a.
Szelethus added a subscriber: NoQ.
Szelethus added a comment.
This revision is now accepted and ready to land.

Let's also have a link to your cfe-dev mail in this patch: 
http://lists.llvm.org/pipermail/cfe-dev/2019-January/060968.html

Overall, I like this quite a bit, as I personally experienced the consequence 
of this invalidation technique while developing `UninitializedObjectChecker`. 
I'd be interested to see how many more reports will it emit with this patch, I 
have a suspicion that it'll be very significant.

Let's wait for what @NoQ thinks of this patch.

> What should be the spelling of such an annotation?

How about these: `uses_offsetof`, `may_use_offsetof`?

> How to handle indirect calls? Even if we were in an ideal world where all the 
> callees are annotated, we might not know who the actual callee is (function 
> pointers, virtual calls etc). So what should we do? Less or more invalidation 
> for all indirect calls or have a separate mechanism to let the user define at 
> the call site how to handle a specific call?

Hmm, I suspect that such functions are few and far in between, so maybe the 
inconvenience of annotating calls to a function through a function pointer that 
may use `offsetof` can be justified. 
For virtual methods, I guess we can't expect the user to always have the 
ability of adding the annotation to first base where the virtual method is 
declared, and not even CTU can ensure that we can scan all methods that 
implement it. Maybe for these very exceptionally rare cases, annotating the 
actual calls to the functions would also be justified.

Sadly, I can't say anything meaningful about Artem's ideas on top of my head. :)




Comment at: test/Analysis/cxx-uninitialized-object.cpp:371
   PassingToUnknownFunctionTest1(int) {
-mayInitialize(a);
-// All good!
+mayInitialize(a); // expected-warning{{1 uninitialized field at the end of 
the constructor call}}
   }

I bet that the current invalidation technique crippled much of this checker's 
capabilities, so I'm happy to see it change. Hooray!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57230



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55424: [analyzer] Supply all checkers with a shouldRegister function

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D55424#1364696 , @Szelethus wrote:

> In D55424#1326397 , @NoQ wrote:
>
> > Should we also pass `CheckerManager` into `shouldRegister...`? Or is it 
> > entirely useless?
>
>
> I wouldn't say useless, but I'm struggling to come up with an example where 
> registering would depend on non-language dependent reasons. I'm stuck on 
> rebasing my patches, so I'll give it a thought whether it's worth the chore 
> to change everything (but where I'd like to leave a system for the long term, 
> doing extra work shouldn't be a major concern).


For now, I decided against it just to make progress with the project, but if 
the need arises, I'll be happy to change it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55424



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55424: [analyzer] Supply all checkers with a shouldRegister function

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352277: [analyzer] Supply all checkers with a shouldRegister 
function (authored by Szelethus, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D55424

Files:
  include/clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h
  include/clang/StaticAnalyzer/Core/Checker.h
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
  lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCInstMethSignature.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
  lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp
  lib/StaticAnalyzer/Checkers/GTestChecker.cpp
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp
  lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp
  lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp
  lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  lib/StaticAn

[PATCH] D55425: [analyzer] Split unix.API up to UnixAPIMisuseChecker and UnixAPIPortabilityChecker

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352278: [analyzer] Split unix.API up to UnixAPIMisuseChecker 
and… (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55425?vs=177168&id=183703#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55425

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  cfe/trunk/test/Analysis/Inputs/expected-plists/unix-fns.c.plist

Index: cfe/trunk/test/Analysis/Inputs/expected-plists/unix-fns.c.plist
===
--- cfe/trunk/test/Analysis/Inputs/expected-plists/unix-fns.c.plist
+++ cfe/trunk/test/Analysis/Inputs/expected-plists/unix-fns.c.plist
@@ -3,7 +3,7 @@
 
 
  clang_version
-clang version 8.0.0 
+clang version 8.0.0
  diagnostics
  
   
@@ -744,9 +744,9 @@
descriptionCall to 'malloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context0e841458f0cb7cf161d35f9db5862dcf
+   issue_hash_content_of_line_in_context4ddbefeb3fa802a0636dc63d679bdc89
   issue_context_kindfunction
   issue_contextpr2899
   issue_hash_function_offset1
@@ -835,9 +835,9 @@
descriptionCall to 'calloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_contexta267ff573c7e8b959a3f886677893eb0
+   issue_hash_content_of_line_in_context9f12ad2f0a645cb7e4485fed526f536e
   issue_context_kindfunction
   issue_contexttest_calloc
   issue_hash_function_offset1
@@ -926,9 +926,9 @@
descriptionCall to 'calloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context14eb72957baab3c63bac610a10e6f48b
+   issue_hash_content_of_line_in_context835b2375daee5b05ac48f24ac578de4c
   issue_context_kindfunction
   issue_contexttest_calloc2
   issue_hash_function_offset1
@@ -1017,9 +1017,9 @@
descriptionCall to 'realloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context7f6f67ebe3d481aed7750005bea7e371
+   issue_hash_content_of_line_in_contextbbdabcb6c5a3783012ae34bfea2a10fb
   issue_context_kindfunction
   issue_contexttest_realloc
   issue_hash_function_offset1
@@ -1108,9 +1108,9 @@
descriptionCall to 'reallocf' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context4941698efbd81601653dff10ef9c645b
+   issue_hash_content_of_line_in_context5d222055bbf58b08ec345f0ebfd7b9d1
   issue_context_kindfunction
   issue_contexttest_reallocf
   issue_hash_function_offset1
@@ -1199,9 +1199,9 @@
descriptionCall to 'alloca' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_contextb7ca3488e81d9d9d4b8dc545258ce97c
+   issue_hash_content_of_line_in_contextf7bdefde93c0a58ec236918fb0c3a54e
   issue_context_kindfunction
   issue_contexttest_alloca
   issue_hash_function_offset1
@@ -1290,9 +1290,9 @@
descriptionCall to 'alloca' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context1ec52551362b070237f47f6bb6c3847d
+   issue_hash_content_of_line_in_context4247526f8da82479508f2d364c2992d5
   issue_context_kindfunction
   issue_contexttest_builtin_alloca
   issue_hash_function_offset1
@@ -1381,9 +1381,9 @@
descriptionCall to 'valloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context675741e04c8d0071d280324e23f41d35
+   issue_hash_content_of_line_in_contexte16dfa9598fd2fafe6dc5563990c1dd3
   issue_context_kindfunction
   issue_contexttest_valloc
   issue_hash_function_offset1
@@ -3015,7 +3015,7 @@
  
  files
  
-  /clang/test/Analysis/unix-fns.c
+  /home/szelethus/Documents/analyzer_opts/clang/test/Analysis/unix-fns.c
  
 
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp

[PATCH] D56988: [analyzer][NFC] Supply CheckerRegistry with AnalyzerOptions

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352279: [analyzer][NFC] Supply CheckerRegistry with 
AnalyzerOptions (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56988?vs=182722&id=183704#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56988

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  cfe/trunk/include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  cfe/trunk/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

Index: cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -33,35 +33,36 @@
 DiagnosticsEngine &diags) {
   auto checkerMgr = llvm::make_unique(context, opts);
 
-  CheckerRegistry allCheckers(plugins, diags, context.getLangOpts());
+  CheckerRegistry allCheckers(plugins, diags, opts, context.getLangOpts());
 
   for (const auto &Fn : checkerRegistrationFns)
 Fn(allCheckers);
 
-  allCheckers.initializeManager(*checkerMgr, opts);
-  allCheckers.validateCheckerOptions(opts);
+  allCheckers.initializeManager(*checkerMgr);
+  allCheckers.validateCheckerOptions();
   checkerMgr->finishedCheckerRegistration();
 
   return checkerMgr;
 }
 
 void ento::printCheckerHelp(raw_ostream &out, ArrayRef plugins,
+AnalyzerOptions &anopts,
 DiagnosticsEngine &diags,
 const LangOptions &langOpts) {
   out << "OVERVIEW: Clang Static Analyzer Checkers List\n\n";
   out << "USAGE: -analyzer-checker \n\n";
 
-  CheckerRegistry(plugins, diags, langOpts).printHelp(out);
+  CheckerRegistry(plugins, diags, anopts, langOpts).printHelp(out);
 }
 
 void ento::printEnabledCheckerList(raw_ostream &out,
ArrayRef plugins,
-   const AnalyzerOptions &opts,
+   AnalyzerOptions &anopts,
DiagnosticsEngine &diags,
const LangOptions &langOpts) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  CheckerRegistry(plugins, diags, langOpts).printList(out, opts);
+  CheckerRegistry(plugins, diags, anopts, langOpts).printList(out);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -40,8 +40,9 @@
 
 CheckerRegistry::CheckerRegistry(ArrayRef plugins,
  DiagnosticsEngine &diags,
+ AnalyzerOptions &AnOpts,
  const LangOptions &LangOpts)
-  : Diags(diags), LangOpts(LangOpts) {
+  : Diags(diags), AnOpts(AnOpts), LangOpts(LangOpts) {
 
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI)\
@@ -106,8 +107,7 @@
   return false;
 }
 
-CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers(
-const AnalyzerOptions &Opts) const {
+CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers() const {
 
   assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
  "In order to efficiently gather checkers, this function expects them "
@@ -116,7 +116,7 @@
   CheckerInfoSet enabledCheckers;
   const auto end = Checkers.cend();
 
-  for (const std::pair &opt : Opts.CheckersControlList) {
+  for (const std::pair &opt : AnOpts.CheckersControlList) {
 // Use a binary search to find the possible start of the package.
 CheckerRegistry::CheckerInfo
 packageInfo(nullptr, nullptr, opt.first, "", "");
@@ -167,13 +167,12 @@
   }
 }
 
-void CheckerRegistry::initializeManager(CheckerManager &checkerMgr,
-const AnalyzerOptions &Opts) const {
+void CheckerRegistry::initializeManager(CheckerManager &checkerMgr) const {
   // Sort checkers for efficient collection.
   llvm::sort(Checkers, checkerNameLT);
 
   // Collect checkers enabled by the options.
-  CheckerInfoSet enabledCheckers = getEnabledCheckers(Opts);
+  CheckerInfoSet enabledCheckers = getEnabledCheckers();
 
   // Initialize the CheckerManager with all enabled checkers.
   for (const auto *i : enabledCheckers) {
@@ -182,9 +181,8 @@
   }
 }
 
-void CheckerRegistry::validateCheckerOptions(
-

[PATCH] D56989: [analyzer][NFC] Keep track of whether enabling a checker was explictly specified in command line arguments

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:143
+  // Would it be better to name it '~experimental' or something else
+  // that's ASCIIbetically last?
+  llvm::sort(Checkers, checkerNameLT);

NoQ wrote:
> I learned a new word today :) :)
Wish it came from me, but I just moved it around... put a smile on my face 
every time I came across it too though :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D56989



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56989: [analyzer][NFC] Keep track of whether enabling a checker was explictly specified in command line arguments

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352282: [analyzer][NFC] Keep track of whether enabling a 
checker was explictly… (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56989?vs=182724&id=183705#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56989

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

Index: cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -38,12 +38,66 @@
   return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
+static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
+  const CheckerRegistry::CheckerInfo &b) {
+  return a.FullName < b.FullName;
+}
+
+static constexpr char PackageSeparator = '.';
+
+static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
+StringRef packageName) {
+  // Does the checker's full name have the package as a prefix?
+  if (!checker.FullName.startswith(packageName))
+return false;
+
+  // Is the package actually just the name of a specific checker?
+  if (checker.FullName.size() == packageName.size())
+return true;
+
+  // Is the checker in the package (or a subpackage)?
+  if (checker.FullName[packageName.size()] == PackageSeparator)
+return true;
+
+  return false;
+}
+
+CheckerRegistry::CheckerInfoListRange
+CheckerRegistry::getMutableCheckersForCmdLineArg(StringRef CmdLineArg) {
+
+  assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
+ "In order to efficiently gather checkers, this function expects them "
+ "to be already sorted!");
+
+  // Use a binary search to find the possible start of the package.
+  CheckerRegistry::CheckerInfo
+  packageInfo(nullptr, nullptr, CmdLineArg, "", "");
+  auto it = std::lower_bound(Checkers.begin(), Checkers.end(),
+ packageInfo, checkerNameLT);
+
+  if (!isInPackage(*it, CmdLineArg))
+return { Checkers.end(), Checkers.end() };
+
+  // See how large the package is.
+  // If the package doesn't exist, assume the option refers to a single
+  // checker.
+  size_t size = 1;
+  llvm::StringMap::const_iterator packageSize =
+  Packages.find(CmdLineArg);
+
+  if (packageSize != Packages.end())
+size = packageSize->getValue();
+
+  return { it, it + size };
+}
+
 CheckerRegistry::CheckerRegistry(ArrayRef plugins,
  DiagnosticsEngine &diags,
  AnalyzerOptions &AnOpts,
  const LangOptions &LangOpts)
   : Diags(diags), AnOpts(AnOpts), LangOpts(LangOpts) {
 
+  // Register builtin checkers.
 #define GET_CHECKERS
 #define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI)\
   addChecker(register##CLASS, shouldRegister##CLASS, FULLNAME, HELPTEXT,   \
@@ -52,6 +106,7 @@
 #undef CHECKER
 #undef GET_CHECKERS
 
+  // Register checkers from plugins.
   for (ArrayRef::iterator i = plugins.begin(), e = plugins.end();
i != e; ++i) {
 // Get access to the plugin.
@@ -81,73 +136,38 @@
 if (registerPluginCheckers)
   registerPluginCheckers(*this);
   }
-}
-
-static constexpr char PackageSeparator = '.';
-
-static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
-  const CheckerRegistry::CheckerInfo &b) {
-  return a.FullName < b.FullName;
-}
 
-static bool isInPackage(const CheckerRegistry::CheckerInfo &checker,
-StringRef packageName) {
-  // Does the checker's full name have the package as a prefix?
-  if (!checker.FullName.startswith(packageName))
-return false;
+  // Sort checkers for efficient collection.
+  // FIXME: Alphabetical sort puts 'experimental' in the middle.
+  // Would it be better to name it '~experimental' or something else
+  // that's ASCIIbetically last?
+  llvm::sort(Checkers, checkerNameLT);
 
-  // Is the package actually just the name of a specific checker?
-  if (checker.FullName.size() == packageName.size())
-return true;
+  // Parse '-analyzer-checker' and '-analyzer-disable-checker' options from the
+  // command line.
+  for (const std::pair &opt : AnOpts.CheckersControlList) {
+CheckerInfoListRange checkersForCmdLineArg =
+ getMutableCheckersForCmdLineArg(opt.first);
 
-  // Is the checker in the package (or a subpackage)?
-  if (checker.FullName[packageName.size()] == PackageSeparator)
-return true;
+if (checkersForCmdLineArg.begin() == checkersForCmdLineArg.end()) {
+  Diags.Report(diag::e

[PATCH] D56989: [analyzer][NFC] Keep track of whether enabling a checker was explictly specified in command line arguments

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Oops, I ran into the issue of `check-clang-analysis` not actually running out 
unit tests. I decided not to revert this patch and just commit the fix: 
rC352284 . Any objections?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56989



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54438#1372324 , @NoQ wrote:

> *gets hyped for the upcoming patchlanding party*


Oh yeah, super excited about this! It was a blast!

> In D54438#1329425 , @Szelethus wrote:
> 
>> Hmmm, I left plenty of room for improvement in the tblgen generation, we 
>> could generate compile-time errors on cycles within the dependency graph, 
>> try to include the generated file only once, but these clearly are very 
>> low-prio issues, so I'll do it later. I'll have to revisit the entire thing 
>> soon enough.
> 
> 
> Hmm. The dependency cycle check sounds cool as long as we do actually have 
> problems with dependency cycles. I guess we could just enable/disable the 
> whole cycle of checkers all together? This might even be a useful feature.

That sounds pretty cool actually! For all the crap I give people about 
documentation, I'm struggling quite a bit with this one (will probably end up 
making one for the entirety of `Frontend/`), but will definitely include this 
in there.

> What do you mean by "include the generated file only once"?

Never mind. It was thinking aloud, which I later realized is nonsense.

> Aha, ok, so the current approach to conflict resolution is to only enable the 
> checker if none of its dependencies were disabled on the command line. So if, 
> say, `DependentChecker` depends on `BaseChecker`, once an 
> -analyzer-disable-checker `BaseChecker` flag is passed, it needs to be 
> reverted via `-analyzer-checker BaseChecker` before the `-analyzer-checker 
> DependentChecker` directive would take effect, regardless of in which order 
> it is with respect to the `BaseChecker`-enabling/disabling directives.

Exactly.

> So we kinda choose to err on the side of disabling in ambiguous situations. 
> Which kinda makes sense because the disable-argument is more rare and 
> indicates an irritation of the user. But it is also kinda inconsistent with 
> how the options interact in absence of dependencies: "the flag that was 
> passed last overrides all previous flags". And we can kinda fix this 
> inconsistency by introducing a different behavior:
> 
> - whenever an `-analyzer-checker` flag is passed, remove the "disabled" marks 
> from all checkers it depends on;
> - whenever an `-analyzer-disable-checker` flag is passed, remove the 
> "enabled" marks from all checkers that depend on it.
> 
> This approach still ensures that the set of enabled checkers is consistent 
> (i.e., users are still not allowed to shoot themselves in the foot by running 
> a checker without its dependencies), but it also looks respects every flag in 
> an intuitive manner. WDYT?

Sounds great! Got nothing against that.

Hmm, I didn't really add tests about the dependency related potential implicit 
disabling, not even a warning, so there still is more work to be done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55429: [analyzer] Add CheckerManager::getChecker, make sure that a registry function registers no more than 1 checker

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352292: [analyzer] Add CheckerManager::getChecker, make sure 
that a registry function… (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55429?vs=182727&id=183727#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55429

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  cfe/trunk/test/Analysis/analyzer-checker-config.c
  cfe/trunk/test/Analysis/free.c
  cfe/trunk/test/Analysis/outofbound.c
  cfe/trunk/test/Analysis/undef-buffers.c

Index: cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -141,7 +141,7 @@
   using CheckerDtor = CheckerFn;
 
 //===--===//
-// registerChecker
+// Checker registration.
 //===--===//
 
   /// Used to register checkers.
@@ -153,8 +153,7 @@
   CHECKER *registerChecker(AT &&... Args) {
 CheckerTag tag = getTag();
 CheckerRef &ref = CheckerTags[tag];
-if (ref)
-  return static_cast(ref); // already registered.
+assert(!ref && "Checker already registered, use getChecker!");
 
 CHECKER *checker = new CHECKER(std::forward(Args)...);
 checker->Name = CurrentCheckName;
@@ -164,8 +163,17 @@
 return checker;
   }
 
+  template 
+  CHECKER *getChecker() {
+CheckerTag tag = getTag();
+assert(CheckerTags.count(tag) != 0 &&
+   "Requested checker is not registered! Maybe you should add it as a "
+   "dependency in Checkers.td?");
+return static_cast(CheckerTags[tag]);
+  }
+
 //===--===//
-// Functions for running checkers for AST traversing..
+// Functions for running checkers for AST traversing.
 //===--===//
 
   /// Run checkers handling Decls.
Index: cfe/trunk/test/Analysis/free.c
===
--- cfe/trunk/test/Analysis/free.c
+++ cfe/trunk/test/Analysis/free.c
@@ -1,5 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc
+//
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void free(void *);
 void *alloca(size_t);
Index: cfe/trunk/test/Analysis/undef-buffers.c
===
--- cfe/trunk/test/Analysis/undef-buffers.c
+++ cfe/trunk/test/Analysis/undef-buffers.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,core.uninitialized -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=core.uninitialized \
+// RUN:   -analyzer-config unix:Optimistic=true
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: cfe/trunk/test/Analysis/analyzer-checker-config.c
===
--- cfe/trunk/test/Analysis/analyzer-checker-config.c
+++ cfe/trunk/test/Analysis/analyzer-checker-config.c
@@ -4,7 +4,7 @@
 // RUN: not %

[PATCH] D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus abandoned this revision.
Szelethus added a comment.

As we currently support the actual `macro_expansions` format in CodeChecker, 
and AFAIK you guys at Apple prefer relying on Xcode 
(http://lists.llvm.org/pipermail/cfe-dev/2018-September/059231.html), I'd like 
to focus on more pressing matters.


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

https://reviews.llvm.org/D52790



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td:230
 
 def NullReturnedFromNonnullChecker : Checker<"NullReturnedFromNonnull">,
   HelpText<"Warns when a null pointer is returned from a function that has "

uabelho wrote:
> Any reason this checker shouldn't get a dependecy too?
> 
> If I run it with
> 
>  clang -cc1 -analyze -analyzer-checker=core 
> -analyzer-checker=nullability.NullReturnedFromNonnull empty.c
> 
> on an empty file empty.c I get
> 
> clang: ../tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:171: 
> CHECKER *clang::ento::CheckerManager::getChecker() [CHECKER = (anonymous 
> namespace)::NullabilityChecker]: Assertion `CheckerTags.count(tag) != 0 && 
> "Requested checker is not registered! Maybe you should add it as a " 
> "dependency in Checkers.td?"' failed.
> 
> If I add
>  Dependencies<[NullabilityBase]>,
> to it, then it doesn't trigger the assert.
> 
> I don't know anything about this code, what do you think about it?
Yup, should be there. Thanks for catching this one! I'll be able to commit the 
fix in about 4ish hours, or if it blocks you, feel free to so before that.

Since I didnt add test for each and every affected checker, I wonder whether 
there is any more of these that I forgot/messed up while rebasing.

Thanks again! :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54438#1375858 , @george.karpenkov 
wrote:

> After this landed, options for RetainCountChecker stopped working - e.g. I 
> can't use `osx.cocoa.RetainCount:blah=X`.
>  Do you know why is this the case / how to fix it?


That's bad. I'll look into this ASAP, might take a day or two though if that's 
alright.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54438#1378425 , @Szelethus wrote:

> In D54438#1375858 , 
> @george.karpenkov wrote:
>
> > After this landed, options for RetainCountChecker stopped working - e.g. I 
> > can't use `osx.cocoa.RetainCount:blah=X`.
> >  Do you know why is this the case / how to fix it?
>
>
> That's bad. I'll look into this ASAP, might take a day or two though if 
> that's alright.


Nvm, just poppoed into my head. I suspect that this has been a bug for a while, 
this patch merely unearthed it. As you said:

In D55400#1367046 , @george.karpenkov 
wrote:

> `registerChecker` gets-or-creates a checker object. A checker name (used for 
> getting the options) is set the first time it's created.
>  The checker which was created first "wins" and gets to name the resulting 
> checker.
>  In practice it basically means that options and checkers reusing the same 
> class do not work.


This patch now ensures that //every time// `osx.cocoa.RetaiCount` and/or 
`osx.OSObjectREtainCount= is enabled, `osx.RetainCountBase`˙is registered 
first, which makes the checker object's name just that, making (incorrectly 
ofc) `AnalyzerOptions` look for `osx.RetainCountBase:blah=X`.

Should we not have multiple checkers belong to the same chekcer object, the 
approach of requsting this checker object for it's options would make sense, 
but since this isn't the case, this bug exposes a weakness of 
`AnalyzerOptions::getChecker*Option`, namely that these subcheckers aren't able 
to possess any options at all, and even if one manages to get it working, as 
you said, merely changing the order of `-analyzer-checker=` could mess things 
up.

So clearly, `AnalyzerOptions` should just take the actual checker name as 
parameter, which each checker knows via the `Name` field, or via 
`CheckerManager::getCurrentCheckName` in the checker registry function. I'll 
try to sort this out tonight.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54429: [analyzer] Creating standard Sphinx documentation

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

@NoQ @dcoughlin @george.karpenkov Any objections to this? :)


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

https://reviews.llvm.org/D54429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

The code not directly related to adding the actual flag looks great, but this 
still should be an `-analyzer-config` option.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55135



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM! :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55135



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

We should probably prefer `%clang_analyze_cc1` to `%clang_cc1 -analyze` here 
too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Thanks for the fix! :)

In D55226#1318853 , @Pierre-vh wrote:

> Hello again! I updated the diff and completely removed the outer if.  Please 
> let me know what you think!


If we're sure that `Target` isn't a `nullptr`, it's fine not to use a defensive 
check, but I think we definitely should add `assert(Target);` before using it.




Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:662
 
+
   // Issue a warning.

No need for this newline.


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

https://reviews.llvm.org/D55226



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I hope you don't mind me changing the revision title -- many of us are 
automatically subscribed to revisions with `analyzer` in the title, that also 
helps with getting feedback sooner :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D53280#1319446 , @NoQ wrote:

> Ugh. We broke backwards compatibility when an invalid `-analyzer-config` is 
> specified but `-analyze` isn't specified. Yes, people are accidentally 
> relying on that :/ :/ :/
>
> In this case the driver doesn't know that it needs to add 
> `analyzer-config-compatibility-mode=true` (or, even better, outright ignore 
> all analyzer options) but the frontend doesn't know that it doesn't need to 
> parse them.


Oh my god. Can you go a little more in depth? Was the problem something along 
the lines of `treu` typo, or a non-existent flag? Is that at all salvageable? I 
mean, losing the entire patch would be super bad imo.

Also, I can't attend to this issue immediately -- replying on the phone is one 
thing, but almost any other action is out of my reach for the next couple 
days/weeks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53280



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:3307-3309
 void Clang::ConstructJob(Compilation &C, const JobAction &JA,
  const InputInfo &Output, const InputInfoList &Inputs,
  const ArgList &Args, const char *LinkingOutput) const 
{

Hmmm. If we just moved the 
`CmdArgs.push_back("-analyzer-config-compatibility-mode=true");` line to this 
function (which actually invokes `RenderAnalyzerOptions`), that would solve the 
issue, wouldn't it? Or somewhere in an even more general driver function.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53280



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added a comment.

In D53280#1319503 , @NoQ wrote:

> It's a flag that was removed but old soft doesn't know that it was removed, 
> and it won't be fixed because it's old.


We could just add the flag back, and do nothing with it. We have already 
mentioned converting some frontend flags to analyzer configs, and removing some 
long enabled-by-default options while still leaving them in the code in some 
form or another for backward compatibility, so some carefully documented litter 
in `AnalyzerOptions` here and there wouldn't be the end of the world.




Comment at: lib/Driver/ToolChains/Clang.cpp:3307-3309
 void Clang::ConstructJob(Compilation &C, const JobAction &JA,
  const InputInfo &Output, const InputInfoList &Inputs,
  const ArgList &Args, const char *LinkingOutput) const 
{

NoQ wrote:
> Szelethus wrote:
> > Hmmm. If we just moved the 
> > `CmdArgs.push_back("-analyzer-config-compatibility-mode=true");` line to 
> > this function (which actually invokes `RenderAnalyzerOptions`), that would 
> > solve the issue, wouldn't it? Or somewhere in an even more general driver 
> > function.
> Yeah, but it's not going to be an `AnalyzeJobAction` in the problematic case. 
> So you'd have to add this flag to every single clang invocation.
And I bet that's something we can't do just like that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53280



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55280: [CTU] Remove redundant error check

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

From what I can see, this patch LGTM, but I lack the experience in the CTU 
department just yet to give any meaningful feedback.




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:147
 
 llvm::Expected
 CrossTranslationUnitContext::getCrossTUDefinition(const FunctionDecl *FD,

Would it be worth to add a comment that this function never returns with 
`nullptr` on success?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55280



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55131: [CTU] Add more lit tests and better error handling

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/ctu-main.c:4
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c
+// RUN: cp %S/Inputs/externalFnMap2.txt %t/ctudir2/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze 
-analyzer-checker=core,debug.ExprInspection  -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir2 
-verify %s

martong wrote:
> Szelethus wrote:
> > This is a question rather than anything else, why do we have both 
> > externalFnMap2.txt and externalFnMap.txt?
> `externalFnMap.txt` goes for `ctu-other.cpp`.
> `externalFnMap2.txt` goes for `ctu-other.c`.
> Perhaps we should rename them in the `Inputs` directory to include the 
> indexed file name. E.g. `ctu-other.cpp.externalFnMap.txt`. What do you think?
Sounds great! :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55131



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Cool!




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:528
 ArrayRef ExplicitRegions,
 ArrayRef Regions, const LocationContext *LCtx,
 const CallEvent *Call) const {

This isn't specific to this revision, but I find the parameter name `Regions` 
way too vague. Maybe `ImplicitRegions`?



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:540-542
+// Explicit regions are the regions passed into the call directly, but
+// not all of them end up being invalidated. The ones that do appear in
+// the Regions array as well.

Really? Then I guess this needs to be updated in `CheckerDocumentation.cpp`:

```
  /// \param ExplicitRegions The regions explicitly requested for invalidation.
  ///For a function call, this would be the arguments. For a bind, this
  ///would be the region being bound to.
```

To me, this clearly indicates that the elements of `ExplicitRegions` will be 
invalidated. Does "requested for" really just mean "requested for potential"? 
Since this happens //before// any invalidation, it could easily be interpreted 
as "invalidated after the end of the callback".


Repository:
  rC Clang

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

https://reviews.llvm.org/D55289



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added a comment.

This looks great!

> Had to pass the checker into the visitor and make some methods non-static 
> because globals with constructors are discouraged.

How about making the set `constexpr`?




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:122
   private:
+const MoveChecker *Chk;
 // The tracked region.

Why not const reference?



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

Can't we just change this to `// expected-warning{{Called C++ object pointer is 
null}}`? This file is so tiny, I think it wouldn't cause much confusion, and  
reduces unnecessary maintenance work.



Comment at: test/Analysis/use-after-move.cpp:16
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &&move(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T &a, T &b) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T &t);
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 

This is cool, thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I like what I'm seeing.

In D55226#1320281 , @Pierre-vh wrote:

> Hi again!
>
> As I'm quite new to this, I don't know what the next step is. Do we need to 
> wait for more people to review this diff?
>  What happens when it's considered "ready"? How is it committed? (I don't 
> have commit access)


Welcome! :) Always great to see new people around.

Once the revision is accepted (and, preferably accepted by someone 
knowledgeable in the analyzer) you may commit. When you start contributing to 
the project, and you don't have commit access just yet, you should ask someone 
who does to commit the patch on your behalf. Once you have "enough" patches 
under your belt, you may ask for a commit access.

Here are some links with more info on patch reviews, commiting, and the like:

https://llvm.org/docs/Contributing.html
https://llvm.org/docs/DeveloperPolicy.html ("Making and Submitting a Patch" 
seems outdated, but the following sections are worth a read: "Code Reviews", 
"Obtaining Commit Access" (and the rest, if you have the time))
https://llvm.org/docs/Phabricator.html

Sadly, I won't be around my working PC for a while, so I can't commit on your 
behalf just yet. If you asked for someone to commit but no one did, it's 
perfectly fine to ping it once a week.


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

https://reviews.llvm.org/D55226



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

AFAIK `constexpr` arrays can be `std::sort`-ed, but it probably isn't worth the 
effort, I tried it myself when I was working with non-checker configs, and it's 
a big hassle for ultimately very little gain.




Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

NoQ wrote:
> Szelethus wrote:
> > Can't we just change this to `// expected-warning{{Called C++ object 
> > pointer is null}}`? This file is so tiny, I think it wouldn't cause much 
> > confusion, and  reduces unnecessary maintenance work.
> I don't think it'll work. The warning is not on this line, it is in 
> `system-header-simulator-cxx.h`, so we need to specify it somehow, and it'll 
> appear only in this test, not in other tests that include that header, so we 
> can't put it directly into the header.
Ah, okay.


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

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52790: [analyzer][PlistMacroExpansion] New flag to convert macro expansions to events

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus planned changes to this revision.
Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

Since the way how these macro events are created as a whole changed in part 1, 
this path will need a big overhaul.


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

https://reviews.llvm.org/D52790



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Can't we just change this to `// expected-warning{{Called C++ object 
> > > pointer is null}}`? This file is so tiny, I think it wouldn't cause much 
> > > confusion, and  reduces unnecessary maintenance work.
> > I don't think it'll work. The warning is not on this line, it is in 
> > `system-header-simulator-cxx.h`, so we need to specify it somehow, and 
> > it'll appear only in this test, not in other tests that include that 
> > header, so we can't put it directly into the header.
> Ah, okay.
But since this is the only warning in the file, we could get away with it 
of we use `FileCheck`! But I leave it up to you.

I had a lot of bots breaking on me because I forgot `git add` on this file 
once, so I might end up fixing it myself.


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

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54401#1318282 , @alexfh wrote:

> In D54401#1315354 , @NoQ wrote:
>
> > The code looks good, but i vaguely remember that we might accidentally 
> > break clang-tidy integration that uses this API to enable/disable specific 
> > checkers via `-checks=-analyzer-...`.
> >
> > *summons @alexfh*
>
>
> It's hard to tell without trying. Could you build and test clang-tools-extra 
> with this patch? There should be a test for the relevant functionality in 
> clang-tidy.


It compiles without problem, and `check-all` doesn't produce a single 
`clang-tools-extra` failure. Yay!


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

https://reviews.llvm.org/D54401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus planned changes to this revision.
Szelethus added a comment.

Let's try that again then! :)


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

https://reviews.llvm.org/D51866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:528
 ArrayRef ExplicitRegions,
 ArrayRef Regions, const LocationContext *LCtx,
 const CallEvent *Call) const {

NoQ wrote:
> Szelethus wrote:
> > This isn't specific to this revision, but I find the parameter name 
> > `Regions` way too vague. Maybe `ImplicitRegions`?
> How about these?
Awesome, thanks! :)



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:540-542
+// Explicit regions are the regions passed into the call directly, but
+// not all of them end up being invalidated. The ones that do appear in
+// the Regions array as well.

NoQ wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Really? Then I guess this needs to be updated in 
> > > `CheckerDocumentation.cpp`:
> > > 
> > > ```
> > >   /// \param ExplicitRegions The regions explicitly requested for 
> > > invalidation.
> > >   ///For a function call, this would be the arguments. For a 
> > > bind, this
> > >   ///would be the region being bound to.
> > > ```
> > > 
> > > To me, this clearly indicates that the elements of `ExplicitRegions` will 
> > > be invalidated. Does "requested for" really just mean "requested for 
> > > potential"? Since this happens //before// any invalidation, it could 
> > > easily be interpreted as "invalidated after the end of the callback".
> > The callback fires after invalidation - cf. 
> > `ProgramState::invalidateRegionsImpl`. Note that the `if (Eng)` condition 
> > is always true, because how the heck were we supposed to run path-sensitive 
> > analysis without `ExprEngine`.
> > 
> > The terminology is really weird here because we the word "invalidation" has 
> > too many meanings. Essentially, `ExplicitRegions` are regions that were 
> > specifically requested for invalidation, but it is up to the `CallEvent` / 
> > `ProgramState` / `StoreManager` (depending on which `invalidateRegions()` 
> > was called) to decide what invalidation means in this case by filling in 
> > the `RegionAndSymbolInvalidationTraits` map.
> > 
> > For regions that represent values of const pointers/references directly 
> > passed into the function, `CallEvent` decides to set the 
> > `TK_PreserveContents` trait, which says "invalidate the region but keep its 
> > contents intact". It would still perform other forms of invalidation on 
> > that region, say, pointer escape: if there are pointer values written down 
> > somewhere within that region, checkers need to stop tracking them.
> > 
> > Now, the callback never said that it has something to do with 
> > "invalidation". Instead, it is about "region changes", which means changes 
> > of the region's contents in the Store. This doesn't necessarily happen due 
> > to invalidation; it may also happen while evaluating a simple assignment in 
> > the program, as we see in the newly added `testUpdateField()` test. And, as 
> > we've seen above, not every "invalidation" changes contents of the region.
> > 
> > Similarly, `TK_SuppressEscape` would suppress the `checkPointerEscape()` 
> > callback for the region, but will not suppress `checkRegionChanges()` for 
> > that (non-explicit) region.
> > 
> > Therefore we end up in a weird situation when some regions were requested 
> > to be invalidated but never were actually invalidated in terms of the 
> > Store. It kinda  makes sense, but i hate it anyway.  The 
> > `checkRegionChanges()` callback is hard to understand and hard to use and 
> > too general and has too much stuff stuffed into it. `checkPointerEscape` is 
> > an easier-to-use fork of it, but it doesn't cover the use case of checkers 
> > that track objects via regions. I suspect that the right solution here is 
> > to start tracking objects as "symbols", i.e., assign a unique opaque ID to 
> > every state through which every object in the program goes (regardless of 
> > which object it is) and use that as keys in the map. That should remove the 
> > stress of dealing with invalidation from C++ checkers that need to track 
> > objects opaquely. The problem won't magically disappear, as we still need 
> > to identify when exactly does the state change, but an additional level of 
> > indirection (Region -> ID -> CheckerState instead of just Region -> 
> > CheckerState) allows us to decompose it into smaller parts and de-duplicate 
> > some of this work.
> > 
> > But regardless of that, it is still weird that `ExplicitRegions` is not a 
> > sub-set of `Regions`. We need to either document it or fix it, and for some 
> > reason i prefer the latter. In particular, the only checker that actually 
> > actively acts upon `ExplicitRegions` when they're const is 
> > `RetainCountChecker`, but in fact people don't ever use `const` in stuff 
> > that it checks, it just isn't idiomatic.
> Another funny thing about `RegionAndSymbolInvalidationTraits`  i

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177002.
Szelethus marked 32 inline comments as done.
Szelethus added a reviewer: MTC.
Szelethus added a comment.

Addressing inline comments.


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

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy &MemFunctionInfo, CheckerContext &C, const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

NoQ wrote:
> Szelethus wrote:
> > MTC wrote:
> > > Szelethus wrote:
> > > > MTC wrote:
> > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > > > `MallocChecker` that hold a bunch of memory function identifiers and 
> > > > > provide corresponding helper APIs. And we should pick a proper time 
> > > > > to initialize it.
> > > > > 
> > > > > I want to known why you call `initIdentifierInfo()` in 
> > > > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > > > `checkPreCall(const CallEvent &Call, )` in which we need 
> > > > > `MemFunctionInfo`.
> > > > Well, I'd like to know too! I think lazy initializing this struct 
> > > > creates more problems that it solves, but I wanted to draw a line 
> > > > somewhere in terms of how invasive I'd like to make this patch.
> > > How about put the initialization stuff into constructor? Let the 
> > > constructor do the initialization that it should do, let `register*()` do 
> > > its registration, like setting `setOptimism` and so on.
> > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > performance argument can be made here. I specifically checked whether 
> > there's any point in doing this hackery, and the answer is... well, some. 
> > I'll probably touch on these in a followup patch.
> Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> optimization. Hence `CallDescription`.
> 
> Also it cannot be done during registration because the AST is not constructed 
> yet at this point. Well, probably it can be done anyway because the empty 
> identifier table is already there in the `ASTContext` and we can always 
> eagerly add a few items into it, but it still sounds scary.
I would personally prefer to risk initializing these during registration, but 
if we're this many comments into this discussion, then I believe it deserves a 
separate patch.

I'll leave a `TODO:` in the code.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

Szelethus wrote:
> MTC wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I'm not sure `hasNonTrivialConstructorCall()` is a good name although 
> > > > you explained it in details in the comments below. And the comments for 
> > > > this function is difficult for me to understand, which is maybe my 
> > > > problem. 
> > > > 
> > > > And I also think this function doesn't do as much as described in the 
> > > > comments, it is just `true if the invoked constructor in \p NE has a 
> > > > parameter - pointer or reference to a record`, right?
> > > I admit that I copy-pasted this from the source I provided below, and I 
> > > overlooked it before posting it here. I //very// much prefer what you 
> > > proposed :)
> > The comments you provided is from the summary of the patch [[ 
> > https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the 
> > summary information in time to adapt to his patch, so I think it is 
> > appropriate to extract the summary information when he submitted this 
> > patch, see [[ 
> > https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668
> >  | [Analyzer] fix for PR19102 ]]. What do you think?
> Wow, nice detective work there :D Sounds good to me.
Umm, yea, I really couldn't come up with a better name. Hopefully the comments 
both here and at the call site help on this.


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

https://reviews.llvm.org/D54823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > Szelethus wrote:
> > > > > MTC wrote:
> > > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > > > > `MallocChecker` that hold a bunch of memory function identifiers 
> > > > > > and provide corresponding helper APIs. And we should pick a proper 
> > > > > > time to initialize it.
> > > > > > 
> > > > > > I want to known why you call `initIdentifierInfo()` in 
> > > > > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > > > > `checkPreCall(const CallEvent &Call, )` in which we need 
> > > > > > `MemFunctionInfo`.
> > > > > Well, I'd like to know too! I think lazy initializing this struct 
> > > > > creates more problems that it solves, but I wanted to draw a line 
> > > > > somewhere in terms of how invasive I'd like to make this patch.
> > > > How about put the initialization stuff into constructor? Let the 
> > > > constructor do the initialization that it should do, let `register*()` 
> > > > do its registration, like setting `setOptimism` and so on.
> > > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > > performance argument can be made here. I specifically checked whether 
> > > there's any point in doing this hackery, and the answer is... well, some. 
> > > I'll probably touch on these in a followup patch.
> > Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> > optimization. Hence `CallDescription`.
> > 
> > Also it cannot be done during registration because the AST is not 
> > constructed yet at this point. Well, probably it can be done anyway because 
> > the empty identifier table is already there in the `ASTContext` and we can 
> > always eagerly add a few items into it, but it still sounds scary.
> I would personally prefer to risk initializing these during registration, but 
> if we're this many comments into this discussion, then I believe it deserves 
> a separate patch.
> 
> I'll leave a `TODO:` in the code.
Or just go with `CallDescription`.


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

https://reviews.llvm.org/D54823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Thanks! I'll commit in a couple days to give some breathing room for anyone 
who'd object.


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

https://reviews.llvm.org/D54401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177057.
Szelethus added a comment.

Woops, forgot to fix the doc of deallocation functions.


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

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy &MemFunctionInfo, CheckerContext &C, const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream &os, CheckerContext &C,
+

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54438#1322266 , @NoQ wrote:

> In D54438#1315953 , @Szelethus wrote:
>
> > - ✔️ There are in fact a variety of checkers that contain subcheckers like 
> > `MallocChecker`, which I think is all good, but the concept that if a 
> > subchecker is enabled it only sort of registeres the main checker (in 
> > `MallocChecker`'s case it enables modeling, but not reporting) is very hard 
> > to follow. I propose that all such checkers should clearly have a base, 
> > called something `DynamicMemoryModeling`, or `IteratorModeling` etc.
>
>
> This will be the most time-consuming part, as i imagine. You'll have to split 
> each callback into two callbacks in two different checkers (`PostCall` vs. 
> `PostStmt` doesn't work!) and make sure that they are called in the 
> correct order. I expect most modeling to go into `PostStmt` 
> ("post-conditions") and most checking go to `PreStmt` ("pre-conditions"). 
> I.e., "if the pre-condition of the statement is not fulfilled, the checker 
> reports a bug. Otherwise, the model enforces the post-condition after the 
> statement"). Some code (eg., on which particular statements does the checker 
> react?) might be annoying to de-duplicate. Not sure how callbacks that don't 
> have pre/post variants will behave.


Actually, it's already implemented, and the beauty of it is that it requires 
pretty much no change to the checker files. `MallocChecker` is a mess, but 
splitting it up is avoidable in order to pull this off. I'll probably upload a 
patch in the coming days with my proposed solution. Super excited about it, 
actually!

Side note: Before realizing this, I worked out a neat plan to split up 
`MallocChecker` that addresses all of the issues you mentioned above. I'll 
share once it I get the time to put together a nice looking proposal -- since 
everything is so intertwined, I really have to go into tiny details with it, a 
high level "gonna along the lines of this and that" won't cut it. I already 
have a bunch of txt files, whiteboards and papers filled with it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177080.
Szelethus added a comment.
This revision is now accepted and ready to land.

- Rebased to master.
- Adding functions calls with the `noreturn` attribute to the guards.
- Shamelessly stole all sorts of assert-like function names from 
`NoReturnFunctionChecker`.
- Added new test cases accordingly.

This should be it! Sorry for being so slow with this -- I genuinely struggled a 
lot with macros before ultimately giving up.


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

https://reviews.llvm.org/D51866

Files:
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  test/Analysis/cxx-uninitialized-object-unguarded-access.cpp

Index: test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
===
--- /dev/null
+++ test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
@@ -0,0 +1,440 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
+// RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:IgnoreGuardedFields=true \
+// RUN:   -std=c++11 -verify  %s
+
+//===--===//
+// Helper functions for tests.
+//===--===//
+
+[[noreturn]] void halt();
+
+void assert(int b) {
+  if (!b)
+halt();
+}
+
+int rand();
+
+//===--===//
+// Tests for fields properly guarded by asserts.
+//===--===//
+
+class NoUnguardedFieldsTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area;
+  Kind K;
+
+public:
+  NoUnguardedFieldsTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0;
+  break;
+}
+  }
+
+  void operator-() {
+assert(K == Kind::A);
+(void)Area;
+  }
+
+  void operator+() {
+assert(K == Kind::V);
+(void)Volume;
+  }
+};
+
+void fNoUnguardedFieldsTest() {
+  NoUnguardedFieldsTest T1(NoUnguardedFieldsTest::Kind::A);
+  NoUnguardedFieldsTest T2(NoUnguardedFieldsTest::Kind::V);
+}
+
+class NoUngardedFieldsNoReturnFuncCalledTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area;
+  Kind K;
+
+public:
+  NoUngardedFieldsNoReturnFuncCalledTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0;
+  break;
+}
+  }
+
+  void operator-() {
+halt();
+(void)Area;
+  }
+
+  void operator+() {
+halt();
+(void)Volume;
+  }
+};
+
+void fNoUngardedFieldsNoReturnFuncCalledTest() {
+  NoUngardedFieldsNoReturnFuncCalledTest
+T1(NoUngardedFieldsNoReturnFuncCalledTest::Kind::A);
+  NoUngardedFieldsNoReturnFuncCalledTest
+T2(NoUngardedFieldsNoReturnFuncCalledTest::Kind::V);
+}
+
+class NoUnguardedFieldsWithUndefMethodTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area;
+  Kind K;
+
+public:
+  NoUnguardedFieldsWithUndefMethodTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0;
+  break;
+}
+  }
+
+  void operator-() {
+assert(K == Kind::A);
+(void)Area;
+  }
+
+  void operator+() {
+assert(K == Kind::V);
+(void)Volume;
+  }
+
+  // We're checking method definitions for guards, so this is a no-crash test
+  // whether we handle methods without definitions.
+  void methodWithoutDefinition();
+};
+
+void fNoUnguardedFieldsWithUndefMethodTest() {
+  NoUnguardedFieldsWithUndefMethodTest
+  T1(NoUnguardedFieldsWithUndefMethodTest::Kind::A);
+  NoUnguardedFieldsWithUndefMethodTest
+  T2(NoUnguardedFieldsWithUndefMethodTest::Kind::V);
+}
+
+class UnguardedFieldThroughMethodTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+private:
+  int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
+  Kind K;
+
+public:
+  UnguardedFieldThroughMethodTest(Kind K) : K(K) {
+switch (K) {
+case V:
+  Volume = 0;
+  break;
+case A:
+  Area = 0; // expected-warning {{1 uninitialized field}}
+  break;
+}
+  }
+
+  void operator-() {
+assert(K == Kind::A);
+(void)Area;
+  }
+
+  void operator+() {
+(void)Volume;
+  }
+};
+
+void fUnguardedFieldThroughMethodTest() {
+  UnguardedFieldThroughMethodTest T1(UnguardedFieldThroughMethodTest::Kind::A);
+}
+
+class UnguardedPublicFieldsTest {
+public:
+  enum Kind {
+V,
+A
+  };
+
+public:
+  // Note that fields are public.
+  int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}}
+  Kind K;
+
+public:
+  UnguardedPublicFieldsTest(Kind K) : K(K) {
+   

[PATCH] D55424: [analyzer] Supply all checkers with a shouldRegister function

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, MTC, martong.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, jfb, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.
Herald added a reviewer: teemperor.
Szelethus added a parent revision: D54437: [analyzer][NFC] Merge 
ClangCheckerRegistry to CheckerRegistry.

Introduce the boolean `ento::shouldRegister##CHECKERNAME(const LangOptions 
&LO)` function very similarly to `ento::register##CHECKERNAME`. This will force 
every checker to implement this function, but maybe it isn't that bad: I saw a 
lot of ObjC or C++ specific checkers that should probably not register 
themselves based on some `LangOptions` (mine too), but they do anyways.

A big benefit of this is that //all// registry functions now register their 
checker, once it is called, registration is guaranteed.

This patch is a part of a greater effort to reinvent checker registration, more 
info here: D54438#1315953 


Repository:
  rC Clang

https://reviews.llvm.org/D55424

Files:
  include/clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h
  include/clang/StaticAnalyzer/Core/Checker.h
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
  lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCInstMethSignature.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
  lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp
  lib/StaticAnalyzer/Checkers/GTestChecker.cpp
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp
  lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
  lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCUnusedIVa

[PATCH] D55425: [analyzer] Split unix.API up to UnixAPIMisuseChecker and UnixAPIPortabilityChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, MTC.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, jfb, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.

The actual implementation of `unix.API` features a dual-checker: two checkers 
in one, even though they don't even interact at all. Split them up, as this is 
a problem for establishing dependencies.

Since the plist files change (and that's a benefit!) this patch isn't NFC.


Repository:
  rC Clang

https://reviews.llvm.org/D55425

Files:
  lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  test/Analysis/Inputs/expected-plists/unix-fns.c.plist

Index: test/Analysis/Inputs/expected-plists/unix-fns.c.plist
===
--- test/Analysis/Inputs/expected-plists/unix-fns.c.plist
+++ test/Analysis/Inputs/expected-plists/unix-fns.c.plist
@@ -3,7 +3,7 @@
 
 
  clang_version
-clang version 8.0.0 
+clang version 8.0.0
  diagnostics
  
   
@@ -744,9 +744,9 @@
descriptionCall to 'malloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context0e841458f0cb7cf161d35f9db5862dcf
+   issue_hash_content_of_line_in_context4ddbefeb3fa802a0636dc63d679bdc89
   issue_context_kindfunction
   issue_contextpr2899
   issue_hash_function_offset1
@@ -835,9 +835,9 @@
descriptionCall to 'calloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_contexta267ff573c7e8b959a3f886677893eb0
+   issue_hash_content_of_line_in_context9f12ad2f0a645cb7e4485fed526f536e
   issue_context_kindfunction
   issue_contexttest_calloc
   issue_hash_function_offset1
@@ -926,9 +926,9 @@
descriptionCall to 'calloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context14eb72957baab3c63bac610a10e6f48b
+   issue_hash_content_of_line_in_context835b2375daee5b05ac48f24ac578de4c
   issue_context_kindfunction
   issue_contexttest_calloc2
   issue_hash_function_offset1
@@ -1017,9 +1017,9 @@
descriptionCall to 'realloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context7f6f67ebe3d481aed7750005bea7e371
+   issue_hash_content_of_line_in_contextbbdabcb6c5a3783012ae34bfea2a10fb
   issue_context_kindfunction
   issue_contexttest_realloc
   issue_hash_function_offset1
@@ -1108,9 +1108,9 @@
descriptionCall to 'reallocf' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context4941698efbd81601653dff10ef9c645b
+   issue_hash_content_of_line_in_context5d222055bbf58b08ec345f0ebfd7b9d1
   issue_context_kindfunction
   issue_contexttest_reallocf
   issue_hash_function_offset1
@@ -1199,9 +1199,9 @@
descriptionCall to 'alloca' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_contextb7ca3488e81d9d9d4b8dc545258ce97c
+   issue_hash_content_of_line_in_contextf7bdefde93c0a58ec236918fb0c3a54e
   issue_context_kindfunction
   issue_contexttest_alloca
   issue_hash_function_offset1
@@ -1290,9 +1290,9 @@
descriptionCall to 'alloca' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context1ec52551362b070237f47f6bb6c3847d
+   issue_hash_content_of_line_in_context4247526f8da82479508f2d364c2992d5
   issue_context_kindfunction
   issue_contexttest_builtin_alloca
   issue_hash_function_offset1
@@ -1381,9 +1381,9 @@
descriptionCall to 'valloc' has an allocation size of 0 bytes
categoryUnix API
typeUndefined allocation of 0 bytes (CERT MEM04-C; CWE-131)
-   check_nameunix.API
+   check_nameoptin.portability.UnixAPI

-   issue_hash_content_of_line_in_context675741e04c8d0071d280324e23f41d35
+   issue_hash_content_of_line_in_contexte16dfa9598fd2fafe6dc5563990c1dd3
   issue_context_kindfunction
   issue_contexttest_valloc
   issue_hash_function_offset1
@@ -3015,7 +3015,7 @@
  
  files
  
-  /clang/test/Analysis/unix-fns.c
+  /home/szelethus/Documents/analyzer_opts/clang/test

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177182.
Szelethus retitled this revision from "[analyzer][WIP] Reimplement dependencies 
between checkers" to "[analyzer] Reimplement dependencies between checkers".
Szelethus edited the summary of this revision.
Szelethus added reviewers: MTC, baloghadamsoftware.
Szelethus added a comment.
This revision is now accepted and ready to land.

- No longer splitting up `InnerPointerChecker`
- After spending a lot of more time with the checker files, and adding various 
asserts in followup patches, realize that dependencies in between checkers are 
far, //far// more common than anticipated. I find it really cool that this is 
clearly visible now from the tblgen file. Add the following new checkers:
  - `StackAddrEscapeBase`
  - `StackAddrEscapeBase`
  - `CStringModeling`
  - `DynamicMemoryModeling` (base of the `MallocChecker` family)
  - `IteratorModeling` (base of the `IteratorChecker` family)
  - `ValistBase`
  - `SecuritySyntaxChecker` (base of `bcmp`, `bcopy`, etc...)
  - `NSOrCFErrorDerefChecker` (base of `NSErrorChecker` and  `CFErrorChecker`)
  - `IvarInvalidationModeling` (base of `IvarInvalidation` checker family)
- Make sure checkers are only enabled if all of their dependencies can be 
enabled
- Put strong emphasis on preserving the order of insertions when collecting 
enabled checkers
- It is no longer possible to enable `CallAndMessageUnInitRefArg` without 
`CallAndMessageChecker`. Disabling core checkers isn't supported anyways.


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

https://reviews.llvm.org/D54438

Files:
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/NewDelete+MismatchedDeallocator_intersections.cpp
  test/Analysis/NewDelete-checker-test.cpp
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -57,6 +57,13 @@
   return std::string();
 }
 
+static void printChecker(llvm::raw_ostream &OS, const Record &R) {
+  OS << '\"';
+  OS.write_escaped(getCheckerFullName(&R)) << "\", ";
+  OS << R.getName() << ", \"";
+  OS.write_escaped(getStringValue(R, "HelpText")) << '\"';
+}
+
 namespace clang {
 void EmitClangSACheckers(RecordKeeper &Records, raw_ostream &OS) {
   std::vector checkers = Records.getAllDerivedDefinitions("Checker");
@@ -64,7 +71,12 @@
 
   using SortedRecords = llvm::StringMap;
 
-  OS << "\n#ifdef GET_PACKAGES\n";
+  // Emit packages.
+  //
+  // PACKAGE(PACKAGENAME)
+  //   - PACKAGENAME: The name of the package.
+  OS << "\n"
+"#ifdef GET_PACKAGES\n";
   {
 SortedRecords sortedPackages;
 for (unsigned i = 0, e = packages.size(); i != e; ++i)
@@ -79,19 +91,51 @@
   OS << ")\n";
 }
   }
-  OS << "#endif // GET_PACKAGES\n\n";
-  
-  OS << "\n#ifdef GET_CHECKERS\n";
-  for (unsigned i = 0, e = checkers.size(); i != e; ++i) {
-const Record &R = *checkers[i];
+  OS << "#endif // GET_PACKAGES\n"
+"\n";
 
-OS << "CHECKER(" << "\"";
-OS.write_escaped(getCheckerFullName(&R)) << "\", ";
-OS << R.getName() << ", ";
-OS << "\"";
-OS.write_escaped(getStringValue(R, "HelpText")) << '\"';
+  // Emit checkers.
+  //
+  // CHECKER(FULLNAME, CLASS, HELPTEXT)
+  //   - FULLNAME: The full name of the checker, including packages, e.g.:
+  //   alpha.cplusplus.UninitializedObject
+  //   - CLASS: The name of the checker, with "Checker" appended, e.g.:
+  //UninitializedObjectChecker
+  //   - HELPTEXT: The description of the checker.
+  OS << "\n"
+"#ifdef GET_CHECKERS\n"
+"\n";
+  for (const Record *checker : checkers) {
+OS << "CHECKER(";
+printChecker(OS, *checker);
 OS << ")\n";
   }
-  OS << "#endif // GET_CHECKERS\n\n";
+  OS << "\n"
+"#endif // GET_CHECKERS\n"
+"\n";
+
+  // Emit dependencies.
+  //
+  // CHECKER_DEPENDENCY(FULLNAME, DEPENDENCY)
+  //   - FULLNAME: The full name of the checker that depends on another checker.
+  //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
+  OS << "\n"
+"#ifdef GET_CHECKER_DEPENDENCIES\n";
+  for (cons

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177189.
Szelethus added a comment.

- Register the checker after it's dependencies (accidentally left this change 
in the next branch). This implies that `unix.MallocChecker:Optimistic` is no 
longer a thing, and is now called ųnix.DynamicMemoryModeling:Optimistic`. This 
//could// break backward compatibility, but is also addressable, since it can 
always be converted to whatever full name `DynamicMemoryModeling` will have in 
the future. @george.karpenkov @NoQ feelings on this?
- In `test/Analysis/Inputs/expected-plists/nullability-notes.m.plist`, the name 
of the checker associated with a report changed, but this is **NOT** an issue 
raised by this patch -- simply changing the order of `analyzer-checker=ABC,CBA` 
to `CBA,ABC` will cause the same issue. Please don't mind me, but I'm not 
immediately interested in fixing this issue.


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

https://reviews.llvm.org/D54438

Files:
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
  test/Analysis/NewDelete+MismatchedDeallocator_intersections.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/malloc-annotations.c
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -57,6 +57,13 @@
   return std::string();
 }
 
+static void printChecker(llvm::raw_ostream &OS, const Record &R) {
+  OS << '\"';
+  OS.write_escaped(getCheckerFullName(&R)) << "\", ";
+  OS << R.getName() << ", \"";
+  OS.write_escaped(getStringValue(R, "HelpText")) << '\"';
+}
+
 namespace clang {
 void EmitClangSACheckers(RecordKeeper &Records, raw_ostream &OS) {
   std::vector checkers = Records.getAllDerivedDefinitions("Checker");
@@ -64,7 +71,12 @@
 
   using SortedRecords = llvm::StringMap;
 
-  OS << "\n#ifdef GET_PACKAGES\n";
+  // Emit packages.
+  //
+  // PACKAGE(PACKAGENAME)
+  //   - PACKAGENAME: The name of the package.
+  OS << "\n"
+"#ifdef GET_PACKAGES\n";
   {
 SortedRecords sortedPackages;
 for (unsigned i = 0, e = packages.size(); i != e; ++i)
@@ -79,19 +91,51 @@
   OS << ")\n";
 }
   }
-  OS << "#endif // GET_PACKAGES\n\n";
-  
-  OS << "\n#ifdef GET_CHECKERS\n";
-  for (unsigned i = 0, e = checkers.size(); i != e; ++i) {
-const Record &R = *checkers[i];
+  OS << "#endif // GET_PACKAGES\n"
+"\n";
 
-OS << "CHECKER(" << "\"";
-OS.write_escaped(getCheckerFullName(&R)) << "\", ";
-OS << R.getName() << ", ";
-OS << "\"";
-OS.write_escaped(getStringValue(R, "HelpText")) << '\"';
+  // Emit checkers.
+  //
+  // CHECKER(FULLNAME, CLASS, HELPTEXT)
+  //   - FULLNAME: The full name of the checker, including packages, e.g.:
+  //   alpha.cplusplus.UninitializedObject
+  //   - CLASS: The name of the checker, with "Checker" appended, e.g.:
+  //UninitializedObjectChecker
+  //   - HELPTEXT: The description of the checker.
+  OS << "\n"
+"#ifdef GET_CHECKERS\n"
+"\n";
+  for (const Record *checker : checkers) {
+OS << "CHECKER(";
+printChecker(OS, *checker);
 OS << ")\n";
   }
-  OS << "#endif // GET_CHECKERS\n\n";
+  OS << "\n"
+"#endif // GET_CHECKERS\n"
+"\n";
+
+  // Emit dependencies.
+  //
+  // CHECKER_DEPENDENCY(FULLNAME, DEPENDENCY)
+  //   - FULLNAME: The full name of the checker that depends on another checker.
+  //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
+  OS << "\n"
+"#ifdef GET_CHECKER_DEPENDENCIES\n";
+  for (const Record *checker : checkers) {
+if (checker->isValueUnset("Dependencies"))
+  continue;
+
+for (const Record *Dependency :
+checker->getValueAsListOfDefs("Dependencies")) {
+  OS << "CHECKER_DEPENDENCY(";
+  OS << '\"';
+  OS.write_escaped(getCheckerFullName(checker)) << "\", ";
+  OS << '\"';
+  OS.write_escaped(getCheckerFullName(Dependency)) << '\"';
+  OS << ")\n";
+}
+  }
+  OS << "\n"
+"#endif // GET_CHECKER_DEPENDENCIES\n";
 }

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested review of this revision.
Szelethus added a comment.

Please take a second look, this is a pretty important to get right.


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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested review of this revision.
Szelethus added a comment.

Marking this patch as non-accepted.


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

https://reviews.llvm.org/D51866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55424: [analyzer] Supply all checkers with a shouldRegister function

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

The checker file changes in this patch are pretty boring, here are those that 
don't always return true in `shouldRegister`:

- `CastSizeChecker`
- `GTestChecker`
- `NSAutoreleasePoolChecker`
- `ObjCAtSyncChecker`


Repository:
  rC Clang

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

https://reviews.llvm.org/D55424



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55429: [analyzer] Add CheckerManager::getChecker that asserts on non-registered checkers, assert on registering already registered checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, MTC, 
baloghadamsoftware.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.

Title says it all, this is pretty important. While this doesn't immediately 
make it impossible to register multiple checkers in a registry function, it 
does make it completely avoidable.


Repository:
  rC Clang

https://reviews.llvm.org/D55429

Files:
  include/clang/StaticAnalyzer/Core/CheckerManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/analyzer-checker-config.c
  test/Analysis/free.c
  test/Analysis/outofbound.c
  test/Analysis/undef-buffers.c

Index: test/Analysis/undef-buffers.c
===
--- test/Analysis/undef-buffers.c
+++ test/Analysis/undef-buffers.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,core.uninitialized -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=core.uninitialized \
+// RUN:   -analyzer-config unix:Optimistic=true
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: test/Analysis/outofbound.c
===
--- test/Analysis/outofbound.c
+++ test/Analysis/outofbound.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,unix,alpha.security.ArrayBound -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=alpha.security.ArrayBound \
+// RUN:   -analyzer-config unix:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
Index: test/Analysis/free.c
===
--- test/Analysis/free.c
+++ test/Analysis/free.c
@@ -1,5 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc
+//
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void free(void *);
 void *alloca(size_t);
Index: test/Analysis/analyzer-checker-config.c
===
--- test/Analysis/analyzer-checker-config.c
+++ test/Analysis/analyzer-checker-config.c
@@ -4,7 +4,7 @@
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config ..:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unrelated:Optimistic=true 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.Malloc:Optimistic=true
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 // Just to test clang is working.
 # foo
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -196,6 +196,7 @@
 for (auto lastRelatedChecker = firstRelatedChecker+size;
  firstRelatedChecker != lastRelatedChecker; ++firstRelatedChecker) {
   if (opt.second) {
+
 if (!firstRelatedChecker->ShouldRegister(LangOpts))
   continue;
 
Index: lib/StaticAnalyzer/Checkers/ValistChec

[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Woohoo! Thanks!




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:515-516
   // class in which the encountered method defined.
   while (const auto *BR = dyn_cast(ThisRegion))
 ThisRegion = BR->getSuperRegion();
 

How about `MemRegion::getMostDerivedObjectRegion()`? @baloghadamsoftware 
recently commited it,


Repository:
  rC Clang

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

https://reviews.llvm.org/D55387



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55429: [analyzer] Add CheckerManager::getChecker, make sure that a registry function registers no more than 1 checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177245.
Szelethus retitled this revision from "[analyzer] Add 
CheckerManager::getChecker that asserts on non-registered checkers, assert on 
registering already registered checkers" to "[analyzer] Add 
CheckerManager::getChecker, make sure that a registry function registers no 
more than 1 checker".
Szelethus added a comment.

Actually, ensuring that a registry function registers no more than one checker 
can be pulled off very easily -- the change required is so little, I added it 
to this patch. With that, the checker name bug is "officially" fixed.


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

https://reviews.llvm.org/D55429

Files:
  include/clang/StaticAnalyzer/Core/CheckerManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/analyzer-checker-config.c
  test/Analysis/free.c
  test/Analysis/outofbound.c
  test/Analysis/undef-buffers.c

Index: test/Analysis/undef-buffers.c
===
--- test/Analysis/undef-buffers.c
+++ test/Analysis/undef-buffers.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,core.uninitialized -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=core.uninitialized \
+// RUN:   -analyzer-config unix:Optimistic=true
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: test/Analysis/outofbound.c
===
--- test/Analysis/outofbound.c
+++ test/Analysis/outofbound.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,unix,alpha.security.ArrayBound -analyzer-store=region -verify -analyzer-config unix:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=alpha.security.ArrayBound \
+// RUN:   -analyzer-config unix:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
Index: test/Analysis/free.c
===
--- test/Analysis/free.c
+++ test/Analysis/free.c
@@ -1,5 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-store=region -analyzer-checker=core,unix.Malloc -fblocks -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc
+//
+// RUN: %clang_analyze_cc1 -fblocks -verify %s -analyzer-store=region \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void free(void *);
 void *alloca(size_t);
Index: test/Analysis/analyzer-checker-config.c
===
--- test/Analysis/analyzer-checker-config.c
+++ test/Analysis/analyzer-checker-config.c
@@ -4,7 +4,7 @@
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config ..:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.:Optimistic=true 2>&1 | FileCheck %s
 // RUN: not %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unrelated:Optimistic=true 2>&1 | FileCheck %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.Malloc:Optimistic=true
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 // Just to test clang is working.
 # foo
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -196,6 +196,7 @@
 for (auto lastRelatedChecker = firstRelatedChecker+size;
  firstRelatedChecker !=

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54438#1315953 , @Szelethus wrote:

> - ❌ Move `CheckerManager::registerChecker` out of the registry functions.
>   - ❌ Since `shouldRegister##CHECKERNAME` is a thing, we can move everything 
> to the checker's constructor, supply a `CheckerManager`, eliminating the 
> function entirely.
>   - ❌ At long last, get rid of `CheckerManager::setCurrentCheckerName` and 
> `CheckerManager::getCurrentCheckerName`.
> - ❌ It was discussed multiple times (D48285#inline-423172 
> , D49438#inline-433993 
> ), that acquiring the options 
> for the checker isn't too easy, as it's name will be assigned only later on, 
> so currently all checkers initialize their options past construction. This 
> can be solved either by supplying the checker's name to every constructor, or 
> simply storing all enabled checkers in `AnalyzerOptions`, and acquire it from 
> there. I'll see.


Pulling this off is not only difficult, certainly super-invasive, but also 
unnecessary -- in the final patch (D55429 ) I 
used a far simpler (~7 lines of code) solution, that still ensures that the 
checker naming problem never comes up again.

Thank you so much @NoQ for all the feedback! This project has been a super fun. 
I still expect some skeletons to fall out of the closet, but I'm fairly 
confident that the overall direction is set and is good.


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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I recently had trouble with ignoring the return type, I would certainly 
appreciate a tool like this. Can you test it on LLVM+Clang? Maybe some methods 
in the AST library could also benefit from `LLVM_NODISCARD` (containers with 
factories is my no1 thought), and it's also a large enough codebase that it 
would be representative as to how the check behaves.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55425: [analyzer] Split unix.API up to UnixAPIMisuseChecker and UnixAPIPortabilityChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: test/Analysis/Inputs/expected-plists/unix-fns.c.plist:3018
  
-  /clang/test/Analysis/unix-fns.c
+  
/home/szelethus/Documents/analyzer_opts/clang/test/Analysis/unix-fns.c
  

NoQ wrote:
> Does this actually work?
Uhh, didnt catch this one. It does on my own pc and in the office as well, 
where I use a completely different directory structure.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55425



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55429: [analyzer] Add CheckerManager::getChecker, make sure that a registry function registers no more than 1 checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D55429#1324035 , @NoQ wrote:

> Aha, ok, so what's the final procedure




In D55429#1324035 , @NoQ wrote:

> Aha, ok, so what's the final procedure now to register a checker that's 
> artificially split in two? Something like this, right?
>
>   def CommonModel : Checker<"Common">,
> HelpText<"Doesn't emit warnings but models common stuff.">;
>  
>   def SubChecker : Checker<"Sub">,
> HelpText<"Emits common warnings for the sub-stuff.">,
> Dependencies<[CommonModel]>;
>
>
>
>
>   void registerCommonModel(CheckerManager &Mgr) {
> Mgr.registerChecker();
>   }
>  
>   void registerSubChecker(CheckerManager &Mgr) {
> CommonModel *Model = Mgr.getChecker();
> Model->EnableSubChecker = true;
>   }
>
>
> This looks quite usable to me.


Correct! But since I spent so much time with these files, and I don't expect 
maaajor changes to them in the foreseeable future, I'll take the time to 
properly document how the frontend of the analyzer (especially how checker 
registration) works. Maybe with the new sphinx format if it goes through by 
then, but any format is better than none.


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

https://reviews.llvm.org/D55429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I guess the solution would be to check whether there are any user supplied 
flags with "analyze" substring, and add the compatibility flag then. It is 
possible if not probable that a non-static-analyzer related flag with a name 
like that will eventually be added, but I guess we can live with it.

The deadline is around the creation of the 8.0.0. release branch, right?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53280



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I can't fix these right away, but I don't want myself to forget it before 
commiting. Pay no attention.




Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:51-56
+/// relies on information MallocBase gathers.
+/// Example:
+///   def InnerPointerChecker : Checker<"InnerPointer">,
+/// HelpText<"Check for inner pointers of C++ containers used after "
+///  "re/deallocation">,
+/// Dependencies<[MallocBase]>;

`MallocBase` was renamed to `DynamicMemoryModeling`



Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:104
 StringRef Desc;
+llvm::SmallVector Dependencies;
 

Should be `ConstCheckerInfoList`.



Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:139
 
+  /// Makes the checker with the full name \p fullName depends on the checker
+  /// called \p dependency.

typo: depend



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:174
+
+assert(std::is_sorted(Checkers.cbegin(), Checkers.cend(), checkerNameLT));
+

The same assert in there just a couple lines above.



Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:102-103
+  //   alpha.cplusplus.UninitializedObject
+  //   - CLASS: The name of the checker, with "Checker" appended, e.g.:
+  //UninitializedObjectChecker
+  //   - HELPTEXT: The description of the checker.

This is incorrect.


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

https://reviews.llvm.org/D54438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55566: [analyzer] LiveVariables: Fix a zombie expression problem, add testing infrastructure.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Makes sense! I like the summary a lot, and the fact that you added a new debug 
checker. I feel like I'm not yet qualified to give meaningful feedback though, 
but if you are not in a hurry, I'll happily play around with this patch next 
week, and both learn a bit and possibly give a better review.

But looking at it right now, this looks great.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55566



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58777: [analyzer] Fix an assertation failurure for invalid sourcelocation, add a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, baloghadamsoftware, 
Charusso.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

For a rather short code snippet, if `debug.ReportStmts` (added in this patch) 
was enabled, a bug reporter visitor crashed:

  struct h {
operator int();
  };
  
  int k() {
return h();
  }

  include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:472: 
clang::ento::PathDiagnosticSpotPiece::PathDiagnosticSpotPiece(const 
clang::ento::PathDiagnosticLocation &, llvm::StringRef, 
PathDiagnosticPiece::Kind, bool): Assertion `Pos.isValid() && 
Pos.asLocation().isValid() && "PathDiagnosticSpotPiece's must have a valid 
location."' failed.

Ultimately, this originated from `PathDiagnosticLocation::createMemberLoc`, as 
it didn't handle the case where it's `MemberExpr` typed parameter returned and 
invalid `SourceLocation` for `MemberExpr::getMemberLoc`. The solution was to 
find any related valid `SourceLocaion`, and `Stmt::getBeginLoc` happens to be 
just that.


Repository:
  rC Clang

https://reviews.llvm.org/D58777

Files:
  docs/analyzer/developer-docs/DebugChecks.rst
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/invalid-pos-fix.cpp
  test/Analysis/plist-html-macros.c

Index: test/Analysis/plist-html-macros.c
===
--- test/Analysis/plist-html-macros.c
+++ test/Analysis/plist-html-macros.c
@@ -3,7 +3,10 @@
 
 // RUN: rm -rf %t.dir
 // RUN: mkdir -p %t.dir
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-html -o %t.dir/index.plist %s
+//
+// RUN: %clang_analyze_cc1 -o %t.dir/index.plist %s \
+// RUN:   -analyzer-checker=core -analyzer-output=plist-html
+//
 // RUN: ls %t.dir | grep '\.html' | count 1
 // RUN: grep '\.html' %t.dir/index.plist | count 1
 
Index: test/Analysis/invalid-pos-fix.cpp
===
--- /dev/null
+++ test/Analysis/invalid-pos-fix.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-output=plist -o %t.plist \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ReportStmts
+
+struct h {
+  operator int();
+};
+
+int k() {
+  return h(); // expected-warning{{Statement}}
+  // expected-warning@-1{{Statement}}
+  // expected-warning@-2{{Statement}}
+}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -571,6 +571,8 @@
 } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
@@ -671,7 +673,13 @@
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
 const SourceManager &SM) {
-  return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  assert(ME->getMemberLoc().isValid() || ME->getBeginLoc().isValid());
+
+  if (ME->getMemberLoc().isValid())
+return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  return PathDiagnosticLocation(ME->getBeginLoc(), SM, SingleLocK);
 }
 
 PathDiagnosticLocation
Index: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
===
--- lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
+++ lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
@@ -266,3 +266,36 @@
 bool ento::shouldRegisterExplodedGraphViewer(const LangOptions &LO) {
   return true;
 }
+
+//===--===//
+// Emits a report for each and every Stmt.
+//===--===//
+
+namespace {
+
+class ReportStmts : public Checker> {
+  std::unique_ptr BT_stmtLoc;
+
+public:
+  ReportStmts() : BT_stmtLoc(new BuiltinBug(this, "Statement")) {}
+
+  void checkPreStmt(const Stmt *S, CheckerContext &C) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode();
+if (!Node)
+  return;
+
+auto Report = llvm::make_unique(*BT_stmtLoc, "Statement", Node);
+
+C.emitReport(std::move(Report));
+  }
+};
+
+} // end of anonymous namespace
+
+void ento::registerReportStmts(CheckerManager &mgr) {
+  mgr.registerChecker();
+}
+
+bool ento::shouldRegisterReportStmts(const LangOptions &LO) {
+  return true;
+}
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1017,6 +1017,10 @@
   HelpText<"View Exploded 

[PATCH] D58777: [analyzer] Fix an assertation failurure for invalid sourcelocation, add a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 188713.
Szelethus added a comment.

Added context.


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

https://reviews.llvm.org/D58777

Files:
  docs/analyzer/developer-docs/DebugChecks.rst
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/invalid-pos-fix.cpp
  test/Analysis/plist-html-macros.c

Index: test/Analysis/plist-html-macros.c
===
--- test/Analysis/plist-html-macros.c
+++ test/Analysis/plist-html-macros.c
@@ -3,7 +3,10 @@
 
 // RUN: rm -rf %t.dir
 // RUN: mkdir -p %t.dir
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-html -o %t.dir/index.plist %s
+//
+// RUN: %clang_analyze_cc1 -o %t.dir/index.plist %s \
+// RUN:   -analyzer-checker=core -analyzer-output=plist-html
+//
 // RUN: ls %t.dir | grep '\.html' | count 1
 // RUN: grep '\.html' %t.dir/index.plist | count 1
 
Index: test/Analysis/invalid-pos-fix.cpp
===
--- /dev/null
+++ test/Analysis/invalid-pos-fix.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-output=plist -o %t.plist \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ReportStmts
+
+struct h {
+  operator int();
+};
+
+int k() {
+  return h(); // expected-warning{{Statement}}
+  // expected-warning@-1{{Statement}}
+  // expected-warning@-2{{Statement}}
+}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -571,6 +571,8 @@
 } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
@@ -671,7 +673,13 @@
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
 const SourceManager &SM) {
-  return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  assert(ME->getMemberLoc().isValid() || ME->getBeginLoc().isValid());
+
+  if (ME->getMemberLoc().isValid())
+return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  return PathDiagnosticLocation(ME->getBeginLoc(), SM, SingleLocK);
 }
 
 PathDiagnosticLocation
Index: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
===
--- lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
+++ lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
@@ -266,3 +266,36 @@
 bool ento::shouldRegisterExplodedGraphViewer(const LangOptions &LO) {
   return true;
 }
+
+//===--===//
+// Emits a report for each and every Stmt.
+//===--===//
+
+namespace {
+
+class ReportStmts : public Checker> {
+  std::unique_ptr BT_stmtLoc;
+
+public:
+  ReportStmts() : BT_stmtLoc(new BuiltinBug(this, "Statement")) {}
+
+  void checkPreStmt(const Stmt *S, CheckerContext &C) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode();
+if (!Node)
+  return;
+
+auto Report = llvm::make_unique(*BT_stmtLoc, "Statement", Node);
+
+C.emitReport(std::move(Report));
+  }
+};
+
+} // end of anonymous namespace
+
+void ento::registerReportStmts(CheckerManager &mgr) {
+  mgr.registerChecker();
+}
+
+bool ento::shouldRegisterReportStmts(const LangOptions &LO) {
+  return true;
+}
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1017,6 +1017,10 @@
   HelpText<"View Exploded Graphs using GraphViz">,
   Documentation;
 
+def ReportStmts : Checker<"ReportStmts">,
+  HelpText<"Emits a warning for every statement.">,
+  Documentation;
+
 } // end "debug"
 
 
Index: docs/analyzer/developer-docs/DebugChecks.rst
===
--- docs/analyzer/developer-docs/DebugChecks.rst
+++ docs/analyzer/developer-docs/DebugChecks.rst
@@ -285,3 +285,10 @@
 statistics within the analyzer engine. Note the Stats checker (which produces at
 least one bug report per function) may actually change the values reported by
 -analyzer-stats.
+
+Output testing checkers
+===
+
+- debug.ReportStmts reports a warning at **every** statement, making it a very
+  useful tool for testing thoroughly bug report construction and output
+  emission.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58828: [analyzer] Fix taint propagation in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57855: [analyzer] Reimplement checker options

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 189088.
Szelethus added a comment.
Herald added a subscriber: Charusso.

- Capitalized some variable names
- Added more comments
- Preferring binary searches to linear searches wherever possible


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

https://reviews.llvm.org/D57855

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/disable-all-checks.c
  test/Analysis/invalid-checker-option.c
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -90,6 +90,26 @@
   .str();
 }
 
+/// Retrieves the type from a CmdOptionTypeEnum typed Record object. Note that
+/// the class itself has to be modified for adding a new option type in
+/// CheckerBase.td.
+static std::string getCheckerOptionType(const Record &R) {
+  if (BitsInit *BI = R.getValueAsBitsInit("Type")) {
+switch(getValueFromBitsInit(BI, R)) {
+case 0:
+  return "int";
+case 1:
+  return "string";
+case 2:
+  return "bool";
+}
+  }
+  PrintFatalError(R.getLoc(),
+  "unable to parse command line option type for "
+  + getCheckerFullName(&R));
+  return "";
+}
+
 static void printChecker(llvm::raw_ostream &OS, const Record &R) {
 OS << "CHECKER(" << "\"";
 OS.write_escaped(getCheckerFullName(&R)) << "\", ";
@@ -134,6 +154,45 @@
   OS << "#endif // GET_PACKAGES\n"
 "\n";
 
+  // Emit a package option.
+  //
+  // PACKAGE_OPTION(OPTIONTYPE, PACKAGENAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting this out compile-time.
+  //   - PACKAGENAME: Name of the package.
+  //   - OPTIONNAME: Name of the option.
+  //   - DESCRIPTION
+  //   - DEFAULT: The default value for this option.
+  //
+  // The full option can be specified in the command like like this:
+  //   -analyzer-config PACKAGENAME:OPTIONNAME=VALUE
+  OS << "\n"
+"#ifdef GET_PACKAGE_OPTIONS\n";
+  for (const Record *Package : packages) {
+
+if (Package->isValueUnset("PackageOptions"))
+  continue;
+
+std::vector PackageOptions = Package
+   ->getValueAsListOfDefs("PackageOptions");
+for (Record *PackageOpt : PackageOptions) {
+  OS << "PACKAGE_OPTION(\"";
+  OS.write_escaped(getCheckerOptionType(*PackageOpt)) << "\", \"";
+  OS.write_escaped(getPackageFullName(Package)) << "\", ";
+  OS << '\"' << getStringValue(*PackageOpt, "CmdFlag") << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "Desc")) << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "DefaultVal")) << "\"";
+  OS << ")\n";
+}
+  }
+  OS << "#endif // GET_PACKAGE_OPTIONS\n"
+"\n";
+
   // Emit checkers.
   //
   // CHECKER(FULLNAME, CLASS, HELPTEXT)
@@ -160,15 +219,15 @@
   //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
   OS << "\n"
 "#ifdef GET_CHECKER_DEPENDENCIES\n";
-  for (const Record *checker : checkers) {
-if (checker->isValueUnset("Dependencies"))
+  for (const Record *Checker : checkers) {
+if (Checker->isValueUnset("Dependencies"))
   continue;
 
 for (const Record *Dependency :
-checker->getValueAsListOfDefs("Dependencies")) {
+Checker->getValueAsListOfDefs("Dependencies")) {
   OS << "CHECKER_DEPENDENCY(";
   OS << '\"';
-  OS.write_escaped(getCheckerFullName(checker)) << "\", ";
+  OS.write_escaped(getCheckerFullName(Checker)) << "\", ";
   OS << '\"';
   OS.write_escaped(getCheckerFullName(Dependency)) << '\"';
   OS << ")\n";
@@ -176,5 +235,45 @@
   }
   OS << "\n"
 "#endif // GET_CHECKER_DEPENDENCIES\n";
+
+  // Emit a package option.
+  //
+  // CHECKER_OPTION(OPTIONTYPE, CHECKERNAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting 

[PATCH] D57855: [analyzer] Reimplement checker options

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 189090.

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

https://reviews.llvm.org/D57855

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/disable-all-checks.c
  test/Analysis/invalid-checker-option.c
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -90,6 +90,26 @@
   .str();
 }
 
+/// Retrieves the type from a CmdOptionTypeEnum typed Record object. Note that
+/// the class itself has to be modified for adding a new option type in
+/// CheckerBase.td.
+static std::string getCheckerOptionType(const Record &R) {
+  if (BitsInit *BI = R.getValueAsBitsInit("Type")) {
+switch(getValueFromBitsInit(BI, R)) {
+case 0:
+  return "int";
+case 1:
+  return "string";
+case 2:
+  return "bool";
+}
+  }
+  PrintFatalError(R.getLoc(),
+  "unable to parse command line option type for "
+  + getCheckerFullName(&R));
+  return "";
+}
+
 static void printChecker(llvm::raw_ostream &OS, const Record &R) {
 OS << "CHECKER(" << "\"";
 OS.write_escaped(getCheckerFullName(&R)) << "\", ";
@@ -134,6 +154,45 @@
   OS << "#endif // GET_PACKAGES\n"
 "\n";
 
+  // Emit a package option.
+  //
+  // PACKAGE_OPTION(OPTIONTYPE, PACKAGENAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting this out compile-time.
+  //   - PACKAGENAME: Name of the package.
+  //   - OPTIONNAME: Name of the option.
+  //   - DESCRIPTION
+  //   - DEFAULT: The default value for this option.
+  //
+  // The full option can be specified in the command like like this:
+  //   -analyzer-config PACKAGENAME:OPTIONNAME=VALUE
+  OS << "\n"
+"#ifdef GET_PACKAGE_OPTIONS\n";
+  for (const Record *Package : packages) {
+
+if (Package->isValueUnset("PackageOptions"))
+  continue;
+
+std::vector PackageOptions = Package
+   ->getValueAsListOfDefs("PackageOptions");
+for (Record *PackageOpt : PackageOptions) {
+  OS << "PACKAGE_OPTION(\"";
+  OS.write_escaped(getCheckerOptionType(*PackageOpt)) << "\", \"";
+  OS.write_escaped(getPackageFullName(Package)) << "\", ";
+  OS << '\"' << getStringValue(*PackageOpt, "CmdFlag") << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "Desc")) << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "DefaultVal")) << "\"";
+  OS << ")\n";
+}
+  }
+  OS << "#endif // GET_PACKAGE_OPTIONS\n"
+"\n";
+
   // Emit checkers.
   //
   // CHECKER(FULLNAME, CLASS, HELPTEXT)
@@ -160,15 +219,15 @@
   //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
   OS << "\n"
 "#ifdef GET_CHECKER_DEPENDENCIES\n";
-  for (const Record *checker : checkers) {
-if (checker->isValueUnset("Dependencies"))
+  for (const Record *Checker : checkers) {
+if (Checker->isValueUnset("Dependencies"))
   continue;
 
 for (const Record *Dependency :
-checker->getValueAsListOfDefs("Dependencies")) {
+Checker->getValueAsListOfDefs("Dependencies")) {
   OS << "CHECKER_DEPENDENCY(";
   OS << '\"';
-  OS.write_escaped(getCheckerFullName(checker)) << "\", ";
+  OS.write_escaped(getCheckerFullName(Checker)) << "\", ";
   OS << '\"';
   OS.write_escaped(getCheckerFullName(Dependency)) << '\"';
   OS << ")\n";
@@ -176,5 +235,45 @@
   }
   OS << "\n"
 "#endif // GET_CHECKER_DEPENDENCIES\n";
+
+  // Emit a package option.
+  //
+  // CHECKER_OPTION(OPTIONTYPE, CHECKERNAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting this out compile-time.
+  //   - CHECKERNAME: Name of the package.
+  //   - OPTIONNAME: Name of the option.
+  //   - DESCRIPTION
+  //   - DEFAULT: The default value for this option.
+  

[PATCH] D58573: [analyzer] Move UninitializedObject out of alpha

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.
Herald added a subscriber: Charusso.

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D58573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57579: [analyzer][WIP] Enable subcheckers to possess checker options

[Kristóf Umann via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC355297: [analyzer] Enable subcheckers to possess checker 
options (authored by Szelethus, committed by ).
Herald added a subscriber: Charusso.

Changed prior to commit:
  https://reviews.llvm.org/D57579?vs=184720&id=189103#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57579

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp

Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -272,54 +272,74 @@
   /// interpreted as true and the "false" string is interpreted as false.
   ///
   /// If an option value is not provided, returns the given \p DefaultVal.
-  /// @param [in] Name Name for option to retrieve.
+  /// @param [in] CheckerName The *full name* of the checker. One may retrieve
+  /// this from the checker object's field \c Name, or through \c
+  /// CheckerManager::getCurrentCheckName within the checker's registry
+  /// function.
+  /// Checker options are retrieved in the following format:
+  /// `-analyzer-config CheckerName:OptionName=Value.
+  /// @param [in] OptionName Name for option to retrieve.
   /// @param [in] DefaultVal Default value returned if no such option was
   /// specified.
-  /// @param [in] C The checker object the option belongs to. Checker options
-  /// are retrieved in the following format:
-  /// `-analyzer-config :OptionName=Value.
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
-  bool getCheckerBooleanOption(StringRef Name, bool DefaultVal,
-const ento::CheckerBase *C,
-bool SearchInParents = false) const;
-
+  bool getCheckerBooleanOption(StringRef CheckerName, StringRef OptionName,
+   bool DefaultVal,
+   bool SearchInParents = false) const;
+
+  bool getCheckerBooleanOption(const ento::CheckerBase *C, StringRef OptionName,
+   bool DefaultVal,
+   bool SearchInParents = false) const;
 
   /// Interprets an option's string value as an integer value.
   ///
   /// If an option value is not provided, returns the given \p DefaultVal.
-  /// @param [in] Name Name for option to retrieve.
+  /// @param [in] CheckerName The *full name* of the checker. One may retrieve
+  /// this from the checker object's field \c Name, or through \c
+  /// CheckerManager::getCurrentCheckName within the checker's registry
+  /// function.
+  /// Checker options are retrieved in the following format:
+  /// `-analyzer-config CheckerName:OptionName=Value.
+  /// @param [in] OptionName Name for option to retrieve.
   /// @param [in] DefaultVal Default value returned if no such option was
   /// specified.
-  /// @param [in] C The checker object the option belongs to. Checker options
-  /// are retrieved in the following format:
-  /// `-analyzer-config :OptionName=Value.
   /// @param [in] SearchInParents If set to true and the searched option was not
   /// specified for the given checker the options for the parent packages will
   /// be searched as well. The inner packages take precedence over the outer
   /// ones.
-  int getCheckerIntegerOption(StringRef Name, int DefaultVal,
- const ento::CheckerBase *C,
- bool SearchInParents = false) const;
+  int getCheckerIntegerOption(StringRef CheckerName, StringRef OptionName,
+  int DefaultVal,
+  bool SearchInParents = false) const;
+
+  int getCheckerIntegerOption(const ento::CheckerBase *C, StringRef OptionName,
+  int DefaultVal,
+  bool SearchInParents = false) const;
 
   /// Query an option's str

[PATCH] D57855: [analyzer] Reimplement checker options

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 189105.
Szelethus added a comment.

Remembered that we can't use use `llvm::binary_search`, as we'll need the 
iterators in a later patch. Should've rebased all my branches before updating 
:^)


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

https://reviews.llvm.org/D57855

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/disable-all-checks.c
  test/Analysis/invalid-checker-option.c
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -90,6 +90,26 @@
   .str();
 }
 
+/// Retrieves the type from a CmdOptionTypeEnum typed Record object. Note that
+/// the class itself has to be modified for adding a new option type in
+/// CheckerBase.td.
+static std::string getCheckerOptionType(const Record &R) {
+  if (BitsInit *BI = R.getValueAsBitsInit("Type")) {
+switch(getValueFromBitsInit(BI, R)) {
+case 0:
+  return "int";
+case 1:
+  return "string";
+case 2:
+  return "bool";
+}
+  }
+  PrintFatalError(R.getLoc(),
+  "unable to parse command line option type for "
+  + getCheckerFullName(&R));
+  return "";
+}
+
 static void printChecker(llvm::raw_ostream &OS, const Record &R) {
 OS << "CHECKER(" << "\"";
 OS.write_escaped(getCheckerFullName(&R)) << "\", ";
@@ -134,6 +154,45 @@
   OS << "#endif // GET_PACKAGES\n"
 "\n";
 
+  // Emit a package option.
+  //
+  // PACKAGE_OPTION(OPTIONTYPE, PACKAGENAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting this out compile-time.
+  //   - PACKAGENAME: Name of the package.
+  //   - OPTIONNAME: Name of the option.
+  //   - DESCRIPTION
+  //   - DEFAULT: The default value for this option.
+  //
+  // The full option can be specified in the command like like this:
+  //   -analyzer-config PACKAGENAME:OPTIONNAME=VALUE
+  OS << "\n"
+"#ifdef GET_PACKAGE_OPTIONS\n";
+  for (const Record *Package : packages) {
+
+if (Package->isValueUnset("PackageOptions"))
+  continue;
+
+std::vector PackageOptions = Package
+   ->getValueAsListOfDefs("PackageOptions");
+for (Record *PackageOpt : PackageOptions) {
+  OS << "PACKAGE_OPTION(\"";
+  OS.write_escaped(getCheckerOptionType(*PackageOpt)) << "\", \"";
+  OS.write_escaped(getPackageFullName(Package)) << "\", ";
+  OS << '\"' << getStringValue(*PackageOpt, "CmdFlag") << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "Desc")) << "\", ";
+  OS << '\"';
+  OS.write_escaped(getStringValue(*PackageOpt, "DefaultVal")) << "\"";
+  OS << ")\n";
+}
+  }
+  OS << "#endif // GET_PACKAGE_OPTIONS\n"
+"\n";
+
   // Emit checkers.
   //
   // CHECKER(FULLNAME, CLASS, HELPTEXT)
@@ -160,15 +219,15 @@
   //   - DEPENDENCY: The full name of the checker FULLNAME depends on.
   OS << "\n"
 "#ifdef GET_CHECKER_DEPENDENCIES\n";
-  for (const Record *checker : checkers) {
-if (checker->isValueUnset("Dependencies"))
+  for (const Record *Checker : checkers) {
+if (Checker->isValueUnset("Dependencies"))
   continue;
 
 for (const Record *Dependency :
-checker->getValueAsListOfDefs("Dependencies")) {
+Checker->getValueAsListOfDefs("Dependencies")) {
   OS << "CHECKER_DEPENDENCY(";
   OS << '\"';
-  OS.write_escaped(getCheckerFullName(checker)) << "\", ";
+  OS.write_escaped(getCheckerFullName(Checker)) << "\", ";
   OS << '\"';
   OS.write_escaped(getCheckerFullName(Dependency)) << '\"';
   OS << ")\n";
@@ -176,5 +235,45 @@
   }
   OS << "\n"
 "#endif // GET_CHECKER_DEPENDENCIES\n";
+
+  // Emit a package option.
+  //
+  // CHECKER_OPTION(OPTIONTYPE, CHECKERNAME, OPTIONNAME, DESCRIPTION, DEFAULT)
+  //   - OPTIONTYPE: Type of the option, whether it's integer or boolean etc.
+  // This is important for validating user input. Note that
+  // it's a string, rather than an actual type: since we can
+  // load checkers runtime, we can't use template hackery for
+  // sorting t

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 189107.
Szelethus added a comment.
Herald added a reviewer: teemperor.
Herald added a subscriber: Charusso.
This revision now requires review to proceed.

Remove the default value parameters from `getChecker*Option`, as they are no 
longer necessary. Note the changes to the unit test file: since the current 
thinking is that all options must've been validated by `CheckerRegistry`, if 
any irregularities are detected within those methods, we no longer return 
default values or even dream of a peaceful termination, as it clearly indicates 
a programming error, hence the removal of some test cases as asserts can't be 
tested.


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

https://reviews.llvm.org/D57922

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/analyzer-config.c
  unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp

Index: unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp
===
--- unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp
+++ unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp
@@ -51,25 +51,24 @@
   // CheckerTwo one has Option specified as true. It should read true regardless
   // of search mode.
   CheckerOneMock CheckerOne;
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", false));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option"));
   // The package option is overridden with a checker option.
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", false,
-   true));
+  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option", true));
   // The Outer package option is overridden by the Inner package option. No
   // package option is specified.
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", false,
-   true));
-  // No package option is specified and search in packages is turned off. The
-  // default value should be returned.
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", false));
   EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerOne, "Option2", true));
+  // No package option is specified and search in packages is turned off. We
+  // should assert here, but we can't test that.
+  //Opts.getCheckerBooleanOption(&CheckerOne, "Option2");
+  //Opts.getCheckerBooleanOption(&CheckerOne, "Option2");
 
-  // Checker true has no option specified. It should get the default value when
-  // search in parents turned off and false when search in parents turned on.
+  // Checker true has no option specified. It should get false when search in
+  // parents turned on.
   CheckerTwoMock CheckerTwo;
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", false));
-  EXPECT_TRUE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true));
-  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true, true));
+  EXPECT_FALSE(Opts.getCheckerBooleanOption(&CheckerTwo, "Option", true));
+  // In any other case, we should assert, that we cannot test unfortunately.
+  //Opts.getCheckerBooleanOption(&CheckerTwo, "Option");
+  //Opts.getCheckerBooleanOption(&CheckerTwo, "Option");
 }
 
 TEST(StaticAnalyzerOptions, StringOptions) {
@@ -84,16 +83,14 @@
 
   CheckerOneMock CheckerOne;
   EXPECT_TRUE("StringValue" ==
-Opts.getCheckerStringOption(&CheckerOne, "Option", "DefaultValue"));
-  EXPECT_TRUE("DefaultValue" ==
-   Opts.getCheckerStringOption(&CheckerOne, "Option2", "DefaultValue"));
+Opts.getCheckerStringOption(&CheckerOne, "Option"));
 }
 
 TEST(StaticAnalyzerOptions, SubCheckerOptions) {
   AnalyzerOptions Opts;
   Opts.Config["Outer.Inner.CheckerOne:Option"] = "StringValue";
   EXPECT_TRUE("StringValue" == Opts.getCheckerStringOption(
-"Outer.Inner.CheckerOne", "Option", "DefaultValue"));
+"Outer.Inner.CheckerOne", "Option"));
 }
 
 } // end namespace ento
Index: test/Analysis/analyzer-config.c
===
--- test/Analysis/analyzer-config.c
+++ test/Analysis/analyzer-config.c
@@ -3,6 +3,16 @@
 
 // CHECK: [config]
 // CHECK-NEXT: aggressive-binary-operation-simplification = false

[PATCH] D58828: [analyzer] Fix taint propagation in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355396: [analyzer] Fix taint propagation in 
GenericTaintChecker (authored by Szelethus, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D58828

Files:
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  test/Analysis/taint-generic.c


Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -458,7 +458,7 @@
   ProgramStateRef State = C.getState();
 
   // Check for taint in arguments.
-  bool IsTainted = false;
+  bool IsTainted = true;
   for (unsigned ArgNum : SrcArgs) {
 if (ArgNum >= CE->getNumArgs())
   return State;
Index: test/Analysis/taint-generic.c
===
--- test/Analysis/taint-generic.c
+++ test/Analysis/taint-generic.c
@@ -2,6 +2,7 @@
 // RUN: %clang_analyze_cc1  -DFILE_IS_STRUCT 
-analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 
-Wno-format-security -verify %s
 
 int scanf(const char *restrict format, ...);
+char *gets(char *str);
 int getchar(void);
 
 typedef struct _FILE FILE;
@@ -142,6 +143,12 @@
   system(buffern2); // expected-warning {{Untrusted data is passed to a system 
call}}
 }
 
+void testGets() {
+  char str[50];
+  gets(str);
+  system(str); // expected-warning {{Untrusted data is passed to a system 
call}}
+}
+
 void testTaintedBufferSize() {
   size_t ts;
   scanf("%zd", &ts);


Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -458,7 +458,7 @@
   ProgramStateRef State = C.getState();
 
   // Check for taint in arguments.
-  bool IsTainted = false;
+  bool IsTainted = true;
   for (unsigned ArgNum : SrcArgs) {
 if (ArgNum >= CE->getNumArgs())
   return State;
Index: test/Analysis/taint-generic.c
===
--- test/Analysis/taint-generic.c
+++ test/Analysis/taint-generic.c
@@ -2,6 +2,7 @@
 // RUN: %clang_analyze_cc1  -DFILE_IS_STRUCT -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -Wno-format-security -verify %s
 
 int scanf(const char *restrict format, ...);
+char *gets(char *str);
 int getchar(void);
 
 typedef struct _FILE FILE;
@@ -142,6 +143,12 @@
   system(buffern2); // expected-warning {{Untrusted data is passed to a system call}}
 }
 
+void testGets() {
+  char str[50];
+  gets(str);
+  system(str); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
 void testTaintedBufferSize() {
   size_t ts;
   scanf("%zd", &ts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57850: [analyzer] Emit an error rather than assert on invalid checker option input

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.
Herald added a subscriber: Charusso.



Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:352-355
+  if (Checker->AllowedPad < 0)
+
Mgr.getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input)
+<< (llvm::Twine() + Checker->getTagDescription() + ":AllowedPad").str()
+<< "a non-negative";

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > 
> > > I passively wish for a certain amount of de-duplication that wouldn't 
> > > require every checker to obtain a diagnostics engine every time it tries 
> > > to read an option. Eg.,
> > > ```lang=c++
> > >   auto *Checker = Mgr.registerChecker();
> > >   Checker->AllowedPad = Mgr.getAnalyzerOptions()
> > >   .getCheckerIntegerOption(Checker, "AllowedPad", 24);
> > >   if (Checker->AllowedPad < 0)
> > > Mgr.reportInvalidOptionValue(Checker, "AllowedPad", "a non-negative");
> > > ```
> > > 
> > > Or maybe even something like that:
> > > 
> > > ```lang=c++
> > >   auto *Checker = Mgr.registerChecker();
> > >   Checker->AllowedPad = Mgr.getAnalyzerOptions()
> > >   .getCheckerIntegerOption(Checker, "AllowedPad", 24,
> > >   [](int x) -> Option {
> > >   if (x < 0) {
> > > // Makes getCheckerIntegerOption() emit a 
> > > diagnostic
> > > // and return the default value.
> > > return "a non-negative";
> > >   }
> > >   // Makes getCheckerIntegerOption() successfully 
> > > return
> > >   // the user-specified value.
> > >   return None;
> > >   });
> > > ```
> > > I.e., a validator lambda.
> > First one, sure. I'm a little unsure about the second: No other 
> > "options"-like classes have access to a `DiagnosticsEngine` in clang, as 
> > far as I'm aware, and I guess keeping `AnalyzerOptions` as simple is 
> > possible is preferred. Not only that, but a validator lambda seems an to be 
> > an overkill (though really-really cool) solution. Your first bit of code is 
> > far more readable IMO.
> Hmm, in the first example we'll also have to manually reset the option to the 
> default value if it is invalid, which is also annoying - even if easy to 
> understand, it's also easy to forget.
> 
> And with that and a bit of polish, the lambda approach isn't really much more 
> verbose, and definitely involves less duplication:
> 
> ```lang=c++
> auto *Checker = Mgr.registerChecker();
> Checker->AllowedPad = Mgr.getAnalyzerOptions()
> .getCheckerIntegerOption(Checker, "AllowedPad", 24);
> if (Checker->AllowedPad < 0) {
>   Mgr.reportInvalidOptionValue(Checker, "AllowedPad", "a non-negative value");
>   Checker->AllowedPad = 24;
> }
> ```
> vs.
> ```lang=c++
> auto *Checker = Mgr.registerChecker();
> Checker->AllowedPad = Mgr.getAnalyzerOptions()
> .getCheckerIntegerOption(Checker, "AllowedPad", /*Default=*/ 24,
>  /*Validate=*/ [](int x) { return x >= 0; },
>  /*ValidMsg=*/ "a non-negative value");
> ```
Alright, so I've given this a lot of thought, here's where I'm standing on the 
issue:

* I would prefer not to add `DiagnosticsEngine` to `AnalyzerOptions`. In fact, 
I'd prefer not to add it even as a parameter to one of it's methods -- 
designwise, it should be a simple mapping of the command line parameters, not 
doing any complicated hackery.
* You got me convinced the validator lambda thing ;). However, a nice 
implementation of this (emphasis on //nice//) is most definitely a bigger 
undertaking.
* Once we're at the topic of "easy to forget", we could also verify 
compile-time whether checker options are actually used -- what I'm thinking 
here, is something like this:

```
auto *Checker = Mgr.registerChecker();
Mgr.initFieldWithOption(Checker, "AllowedPad",
// Note that we should be able
// to know the default value.
Checker->AllowedPad,
// We could make this optional by defining a
// default validator...
/*Validate=*/ [](int x) { return x >= 0; },
// ...nd a default error message.
/*ValidMsg=*/ "a non-negative value");
```
`CheckerManager` later (once all checker registry functions finished) could 
validate, with the help of `CheckerRegistry`, whether
* All options for a given checker were queried for,
* The supplied checker options is valid, if not, restore them in compatibility 
mode, emit an error otherwise,
* No list is complete without a third item.

For now, I admit, I have little interest in this. Would you mind me committing 
as is?


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

https://reviews.llvm.org

[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Well, yea, getting rid of a crash is great, it's kind of bummer that can't 
expand the macro properly in the testfile. I really fear that we need a far 
greater overhaul of this entire effort to support them properly -- which should 
be the task of another patch.

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57891



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59054: [analyzer] C++17: PR40022: Support aggregate initialization with compound values in presence of base classes.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

At first glance this looks OK, but I'll take the time to understand the 
infrastructure behind it and give a proper review.




Comment at: clang/test/Analysis/array-struct-region.cpp:3
 // RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c++ 
-analyzer-config c++-inlining=constructors %s
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c++ 
-std=c++17 -analyzer-config c++-inlining=constructors %s
 // RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -DINLINE -verify -x c %s

I didn't see `-analyzer-config c++-inlining=constructors` in the bugreport -- 
why is it needed?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59054: [analyzer] C++17: PR40022: Support aggregate initialization with compound values in presence of base classes.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ooooh you explained that very well, I'm clear on what's happening then :D 
Still, I'll poke the code here and there to learn about this part of the 
analyzer, I might even come up with some meaningful feedback (or a meaningful 
patch acceptance).


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

https://reviews.llvm.org/D59054



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a reviewer: JonasToth.
Szelethus added a comment.

Is this patch good to go? :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D48866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus planned changes to this revision.
Szelethus added a comment.

Uhh, we still need to restore the default value -- I might end up splitting 
this patch a little further.


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

https://reviews.llvm.org/D57922



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59055: [analyzer] Prepare generic taint checker for new sources

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

Cheers!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Uhh, it's such a chore to work on these things -- You can really feel that the 
preprocessor must've been the first thing implemented in clang. Unfortunately, 
you really have to counterweight it's shortcomings with excessive amount of 
comments. I can see an infinite loop and a lot of string comparisons, but 
someone without having this particular test case as a context, it's very hard 
to follow what's happening.

Could you please add some code examples in the comments too?




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1997-1999
 LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+if (!LocStart.isMacroID())
+  break;

> Gets the location of the immediate macro caller, one level up the stack 
> toward the initial macro typed into the source.
Hmm, is there a guarantee that at the end of the stack is not a macro location?



Comment at: clang/test/Analysis/diagnostics/macros.cpp:53
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to 
UINT32_MAX}}
+// expected-note@-1 {{Taking false branch}}

Ah, that looks pretty :D


Repository:
  rC Clang

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

https://reviews.llvm.org/D59121



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59055: [analyzer] Prepare generic taint checker for new sources

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355703: [analyzer] Use the new infrastructure of expressing 
taint propagation, NFC (authored by Szelethus, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D59055

Files:
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -62,9 +62,6 @@
   /// Propagate taint generated at pre-visit.
   bool propagateFromPre(const CallExpr *CE, CheckerContext &C) const;
 
-  /// Add taint sources on a post visit.
-  void addSourcesPost(const CallExpr *CE, CheckerContext &C) const;
-
   /// Check if the region the expression evaluates to is the standard input,
   /// and thus, is tainted.
   static bool isStdin(const Expr *E, CheckerContext &C);
@@ -72,16 +69,6 @@
   /// Given a pointer argument, return the value it points to.
   static Optional getPointedToSVal(CheckerContext &C, const Expr *Arg);
 
-  /// Functions defining the attack surface.
-  using FnCheck = ProgramStateRef (GenericTaintChecker::*)(
-  const CallExpr *, CheckerContext &C) const;
-  ProgramStateRef postScanf(const CallExpr *CE, CheckerContext &C) const;
-  ProgramStateRef postSocket(const CallExpr *CE, CheckerContext &C) const;
-  ProgramStateRef postRetTaint(const CallExpr *CE, CheckerContext &C) const;
-
-  /// Taint the scanned input if the file is tainted.
-  ProgramStateRef preFscanf(const CallExpr *CE, CheckerContext &C) const;
-
   /// Check for CWE-134: Uncontrolled Format String.
   static const char MsgUncontrolledFormatString[];
   bool checkUncontrolledFormatString(const CallExpr *CE,
@@ -118,6 +105,9 @@
   struct TaintPropagationRule {
 enum class VariadicType { None, Src, Dst };
 
+using PropagationFuncType = bool (*)(bool IsTainted, const CallExpr *,
+ CheckerContext &C);
+
 /// List of arguments which can be taint sources and should be checked.
 ArgVector SrcArgs;
 /// List of arguments which should be tainted on function return.
@@ -127,16 +117,21 @@
 /// Show when a function has variadic parameters. If it has, it marks all
 /// of them as source or destination.
 VariadicType VarType;
+/// Special function for tainted source determination. If defined, it can
+/// override the default behavior.
+PropagationFuncType PropagationFunc;
 
 TaintPropagationRule()
-: VariadicIndex(InvalidArgIndex), VarType(VariadicType::None) {}
+: VariadicIndex(InvalidArgIndex), VarType(VariadicType::None),
+  PropagationFunc(nullptr) {}
 
 TaintPropagationRule(std::initializer_list &&Src,
  std::initializer_list &&Dst,
  VariadicType Var = VariadicType::None,
- unsigned VarIndex = InvalidArgIndex)
+ unsigned VarIndex = InvalidArgIndex,
+ PropagationFuncType Func = nullptr)
 : SrcArgs(std::move(Src)), DstArgs(std::move(Dst)),
-  VariadicIndex(VarIndex), VarType(Var) {}
+  VariadicIndex(VarIndex), VarType(Var), PropagationFunc(Func) {}
 
 /// Get the propagation rule for a given function.
 static TaintPropagationRule
@@ -170,6 +165,10 @@
 /// Pre-process a function which propagates taint according to the
 /// taint rule.
 ProgramStateRef process(const CallExpr *CE, CheckerContext &C) const;
+
+// Functions for custom taintedness propagation.
+static bool postSocket(bool IsTainted, const CallExpr *CE,
+   CheckerContext &C);
   };
 };
 
@@ -206,25 +205,42 @@
   // Check for exact name match for functions without builtin substitutes.
   TaintPropagationRule Rule =
   llvm::StringSwitch(Name)
+  // Source functions
+  // TODO: Add support for vfscanf & family.
+  .Case("fdopen", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("fopen", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("freopen", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("getch", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("getchar", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("getchar_unlocked", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("getenv", TaintPropagationRule({}, {ReturnValueIndex}))
+  .Case("gets", TaintPropagationRule({}, {0, ReturnValueIndex}))
+  .Case("scanf", TaintPropagationRule({}, {}, VariadicType::Dst, 1))
+  .Case("socket",
+TaintPropagationRule({}, {ReturnValueIndex}, VariadicType::None,
+ InvalidArgIndex,
+ &

[PATCH] D57850: [analyzer] Emit an error rather than assert on invalid checker option input

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355704: [analyzer] Emit an error rather than assert on 
invalid checker option input (authored by Szelethus, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D57850

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Core/CheckerManager.h
  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  lib/StaticAnalyzer/Core/CheckerManager.cpp
  test/Analysis/copypaste/suspicious-clones.cpp
  test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
  test/Analysis/outofbound.c
  test/Analysis/padding_c.c
  test/Analysis/undef-buffers.c
  test/Analysis/use-after-move.cpp

Index: include/clang/StaticAnalyzer/Core/CheckerManager.h
===
--- include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -136,6 +136,12 @@
   AnalyzerOptions &getAnalyzerOptions() { return AOptions; }
   ASTContext &getASTContext() { return Context; }
 
+  /// Emits an error through a DiagnosticsEngine about an invalid user supplied
+  /// checker option value.
+  void reportInvalidCheckerOptionValue(const CheckerBase *C,
+   StringRef OptionName,
+   StringRef ExpectedValueDesc);
+
   using CheckerRef = CheckerBase *;
   using CheckerTag = const void *;
   using CheckerDtor = CheckerFn;
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -303,6 +303,8 @@
 def err_analyzer_config_invalid_input : Error<
   "invalid input for analyzer-config option '%0', that expects %1 value">;
 def err_analyzer_config_unknown : Error<"unknown analyzer-config '%0'">;
+def err_analyzer_checker_option_invalid_input : Error<
+  "invalid input for checker option '%0', that expects %1">;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
Index: test/Analysis/undef-buffers.c
===
--- test/Analysis/undef-buffers.c
+++ test/Analysis/undef-buffers.c
@@ -2,7 +2,7 @@
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=unix \
 // RUN:   -analyzer-checker=core.uninitialized \
-// RUN:   -analyzer-config unix:Optimistic=true
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
Index: test/Analysis/padding_c.c
===
--- test/Analysis/padding_c.c
+++ test/Analysis/padding_c.c
@@ -1,4 +1,16 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=optin.performance \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=2
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=optin.performance.Padding \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=-10 \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PAD-NEGATIVE-VALUE
+
+// CHECK-PAD-NEGATIVE-VALUE: (frontend): invalid input for checker option
+// CHECK-PAD-NEGATIVE-VALUE-SAME: 'optin.performance.Padding:AllowedPad', that
+// CHECK-PAD-NEGATIVE-VALUE-SAME: expects a non-negative value
 
 #if __has_include()
 #include 
Index: test/Analysis/outofbound.c
===
--- test/Analysis/outofbound.c
+++ test/Analysis/outofbound.c
@@ -2,7 +2,7 @@
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=unix \
 // RUN:   -analyzer-checker=alpha.security.ArrayBound \
-// RUN:   -analyzer-config unix:Optimistic=true
+// RUN:   -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
Index: test/Analysis/copypaste/suspicious-clones.cpp
===
--- test/Analysis/copypaste/suspicious-clones.cpp
+++ test/Analysis/copypaste/suspicious-clones.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.clone.CloneChecker -analyzer-config alpha.clone.CloneChecker:ReportSuspiciousClones=true  -analyzer-config alpha.clone.CloneChecker:ReportNormalClones=false -analyzer-config alpha.clone.CloneChecker:MinimumCloneComplexity=10 -verify %s
+// RUN: %clang_analyze_cc1 -v