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

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

Szelethus added a comment.

Also, `AnalyzerOptions.def` was recently clan-formatted, feel free to run it 
again after the changes you make in it.


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] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

Szelethus added a comment.

Okay. I was wrong, and you were right.

`MallocChecker` seriously suffers from overcrowding. Especially with the 
addition of `InnerPointerChecker`, it should be split up, but doing that is 
very much avoidable for the purposes of this effort while still achieving every 
goal of it (which I'll detail in the next couple points). But, I spent so much 
time with it, it kinda grew on me, so I'll probably tackle the problem on my 
downtime.

The change of course as follows, checkmarks indicate that I already have a 
working solution locally: (largely copied from D54438#1296695 
)

- ✔️ 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. Add an 
assert when `getChecker` is called on a checker that isn't already registered.
  - ❌ What I didn't do just yet but might be a good idea, is to rename 
`register##CHECKERNAME` to `enable##CHECKERNAME`, and let's reserve the term 
"registering" to `CheckerRegistry`.
  - This patch will //not// feature any functional change, despite the 
observation I just made.

- ✔️ 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.

- ✔️ `UnixAPI` contains a dual checker, but they actually don't interact at 
all, split them up.

- ✖️ Introduce dependencies, all checkers register themselves and only 
themselves. On my local branch I also like that it's super obvious which 
checker is interacting with which one, for example, `IteratorChecker` also has 
multiple parts, but with this patch, it's super obvious from a glance at 
`Checkers.td`.
  - ✔️ We can actually ensure that dependencies are registered before the 
checkers that depend on them, thanks to asserts added in the first part.
  - ❌ Add an assert when `CheckerManager::registerChecker` is called on an 
already registered checker.

- ❌ 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.

- ✖️ Properly document why `CheckerRegistry` and `CheckerManager` are separate 
entities, how are the tblgen files processed, how are dependencies handled, and 
so on.

- ✖️ Rebase my local checker option-related branches, and celebrate. I wouldn't 
like to go in any more detail, who knows what lies ahead.


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] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

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

Szelethus added a comment.

Yup, I agree with everything that was said.


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

https://reviews.llvm.org/D55125



___
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

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

Szelethus added a reviewer: alexfh.
Szelethus added a comment.

*advanced summoning*


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] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

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

Szelethus added a subscriber: donat.nagy.
Szelethus added a comment.

I see your point, but here's why I think it isn't a bug: I like to see macros 
as `constexpr` variables, and if I used those instead, I personally wouldn't 
like to get a warning just because they have the same value. In C, silencing 
such a warning isn't even really possible.

Another interesting thought, @donat.nagy's check works by comparing actual 
nodes of the AST, while this one would work with `Lexer`, but they almost want 
to do the same thing, the only difference is that the first checks whether two 
pieces of code are equivalent, and the second checks whether they are the same. 
Maybe it'd be worth to extract the logic into a simple `areStmtsEqual(const 
Stmt *S1, const Stmt *S2, bool ShouldCompareLexically = false)` function, that 
would do AST based comparison if the last param is set to false, and would use 
`Lexer` if set to true. After that, we could just add command line options to 
both of these checks, to leave it up to the user to choose in between them. 
Maybe folks who suffer from heavily macro-infested code would rather bear the 
obvious performance hit `Lexer` causes for little more precise warnings, and 
(for example) users of C++11 (because there are few excuses not to prefer 
`constexpr` there) could run in AST mode.

But I'm just thinking aloud really.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55125



___
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

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

This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.
Closed by commit rC348038: [analyzer] Emit an error for invalid 
-analyzer-config inputs (authored by Szelethus, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53280?vs=174495&id=176191#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D53280

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Analysis/invalid-analyzer-config-value.c

Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2368,6 +2368,9 @@
   // Treat blocks as analysis entry points.
   CmdArgs.push_back("-analyzer-opt-analyze-nested-blocks");
 
+  // Enable compatilibily mode to avoid analyzer-config related errors.
+  CmdArgs.push_back("-analyzer-config-compatibility-mode=true");
+
   // Add default argument set.
   if (!Args.hasArg(options::OPT__analyzer_no_default_checks)) {
 CmdArgs.push_back("-analyzer-checker=core");
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -181,8 +181,10 @@
   }
 }
 
+// Parse the Static Analyzer configuration. If \p Diags is set to nullptr,
+// it won't verify the input.
 static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
- DiagnosticsEngine &Diags);
+ DiagnosticsEngine *Diags);
 
 static void getAllNoBuiltinFuncValues(ArgList &Args,
   std::vector &Funcs) {
@@ -284,6 +286,12 @@
   Opts.ShowCheckerHelp = Args.hasArg(OPT_analyzer_checker_help);
   Opts.ShowConfigOptionsList = Args.hasArg(OPT_analyzer_config_help);
   Opts.ShowEnabledCheckerList = Args.hasArg(OPT_analyzer_list_enabled_checkers);
+  Opts.ShouldEmitErrorsOnInvalidConfigValue =
+  /* negated */!llvm::StringSwitch(
+   Args.getLastArgValue(OPT_analyzer_config_compatibility_mode))
+.Case("true", true)
+.Case("false", false)
+.Default(false);
   Opts.DisableAllChecks = Args.hasArg(OPT_analyzer_disable_all_checks);
 
   Opts.visualizeExplodedGraphWithGraphViz =
@@ -320,7 +328,7 @@
 
   // Go through the analyzer configuration options.
   for (const auto *A : Args.filtered(OPT_analyzer_config)) {
-A->claim();
+
 // We can have a list of comma separated config names, e.g:
 // '-analyzer-config key1=val1,key2=val2'
 StringRef configList = A->getValue();
@@ -342,11 +350,24 @@
 Success = false;
 break;
   }
+
+  // TODO: Check checker options too, possibly in CheckerRegistry.
+  // Leave unknown non-checker configs unclaimed.
+  if (!key.contains(":") && Opts.isUnknownAnalyzerConfig(key)) {
+if (Opts.ShouldEmitErrorsOnInvalidConfigValue)
+  Diags.Report(diag::err_analyzer_config_unknown) << key;
+continue;
+  }
+
+  A->claim();
   Opts.Config[key] = val;
 }
   }
 
-  parseAnalyzerConfigs(Opts, Diags);
+  if (Opts.ShouldEmitErrorsOnInvalidConfigValue)
+parseAnalyzerConfigs(Opts, &Diags);
+  else
+parseAnalyzerConfigs(Opts, nullptr);
 
   llvm::raw_string_ostream os(Opts.FullCompilerInvocation);
   for (unsigned i = 0; i < Args.getNumInputArgStrings(); ++i) {
@@ -365,56 +386,82 @@
 }
 
 static void initOption(AnalyzerOptions::ConfigTable &Config,
+   DiagnosticsEngine *Diags,
StringRef &OptionField, StringRef Name,
StringRef DefaultVal) {
+  // String options may be known to invalid (e.g. if the expected string is a
+  // file name, but the file does not exist), those will have to be checked in
+  // parseConfigs.
   OptionField = getStringOption(Config, Name, DefaultVal);
 }
 
 static void initOption(AnalyzerOptions::ConfigTable &Config,
+   DiagnosticsEngine *Diags,
bool &OptionField, StringRef Name, bool DefaultVal) {
-  // FIXME: We should emit a warning here if the value is something other than
-  // "true", "false", or the empty string (meaning the default value),
-  // but the AnalyzerOptions doesn't have access to a diagnostic engine.
-  OptionField = llvm::StringSwitch(getStringOption(Config, Name,
-   (DefaultVal ? "true" : "false")))
+  auto PossiblyInvalidVal = llvm::StringSwitch>(
+ getStringOption(Config, Name, (DefaultVal ? "true" : "false")))
   .Case("true", true)
   .Case("false", false)
-  .Default(DefaultVal);
+  .Default(None);
+
+  if (!PossiblyInvalidVal) {

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

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

Szelethus marked 6 inline comments as done.
Szelethus added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:367
 
-  parseAnalyzerConfigs(Opts, Diags);
+  if (Opts.ShouldEmitErrorsOnInvalidConfigValue)
+parseAnalyzerConfigs(Opts, &Diags);

xazax.hun wrote:
> Do we actually need the branching here? It would be perfectly fine to always 
> pass a pointer to `Diags` but sometimes just ignore it.
I'm not sure what would be the point of that, if we don't branch here, we'll 
have to somewhere else, and I'm not sure whether polluting 
`parseAnalyzerConfigs` with that would help on readability.

Or, if you mean that I should just check 
`Opts.ShouldEmitErrorsOnInvalidConfigValue` inside there, that could work, but 
I think it makes more sense that if we don't to validate configs, don't even 
supply the tool for it. Less room for error.


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] D53692: [analyzer] Evaluate all non-checker config options before analysis

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348031: [analyzer] Evaluate all non-checker config options 
before analysis (authored by Szelethus, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53692?vs=172856&id=176183#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D53692

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/Frontend/CompilerInvocation.cpp
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Core/AnalysisManager.cpp
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/BugReporter.cpp
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SValBuilder.cpp
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  lib/StaticAnalyzer/Frontend/ModelInjector.cpp
  test/Analysis/analyzer-config.c
  test/Analysis/analyzer-config.cpp

Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -204,7 +204,7 @@
 PP(CI.getPreprocessor()), OutDir(outdir), Opts(std::move(opts)),
 Plugins(plugins), Injector(injector), CTU(CI) {
 DigestAnalyzerOptions();
-if (Opts->PrintStats || Opts->shouldSerializeStats()) {
+if (Opts->PrintStats || Opts->ShouldSerializeStats) {
   AnalyzerTimers = llvm::make_unique(
   "analyzer", "Analyzer timers");
   TUTotalTimer = llvm::make_unique(
@@ -739,7 +739,7 @@
 
   // Execute the worklist algorithm.
   Eng.ExecuteWorkList(Mgr->getAnalysisDeclContextManager().getStackFrame(D),
-  Mgr->options.getMaxNodesPerTopLevelFunction());
+  Mgr->options.MaxNodesPerTopLevelFunction);
 
   if (!Mgr->options.DumpExplodedGraphTo.empty())
 Eng.DumpGraph(Mgr->options.TrimGraph, Mgr->options.DumpExplodedGraphTo);
Index: lib/StaticAnalyzer/Frontend/ModelInjector.cpp
===
--- lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -48,7 +48,7 @@
   FileID mainFileID = SM.getMainFileID();
 
   AnalyzerOptionsRef analyzerOpts = CI.getAnalyzerOpts();
-  llvm::StringRef modelPath = analyzerOpts->getModelPath();
+  llvm::StringRef modelPath = analyzerOpts->ModelPath;
 
   llvm::SmallString<128> fileName;
 
Index: lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -385,7 +385,7 @@
   // instead of generating an Unknown value and propagate the taint info to it.
   const unsigned MaxComp = StateMgr.getOwningEngine()
->getAnalysisManager()
-   .options.getMaxSymbolComplexity();
+   .options.MaxSymbolComplexity;
 
   if (symLHS && symRHS &&
   (symLHS->computeComplexity() + symRHS->computeComplexity()) <  MaxComp)
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -676,8 +676,8 @@
 bool EnableNullFPSuppression, BugReport &BR,
 const SVal V) {
 AnalyzerOptions &Options = N->getState()->getAnalysisManager().options;
-if (EnableNullFPSuppression && Options.shouldSuppressNullReturnPaths()
-  && V.getAs())
+if (EnableNullFPSuppression &&
+Options.ShouldSuppressNullReturnPaths && V.getAs())
   BR.addVisitor(llvm::make_unique(
   R->getAs(), V));
   }
@@ -808,7 +808,8 @@
 AnalyzerOptions &Options = State->getAnalysisManager().options;
 
 bool EnableNullFPSuppression = false;
-if (InEnableNullFPSuppression && Options.shouldSuppressNullReturnPaths())
+if (InEnableNullFPSuppression &&
+Options.ShouldSuppressNullReturnPaths)
   if (Optional RetLoc = RetVal.getAs())
 EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
@@ -877,7 +878,7 @@
 // future nodes. We want to emit a path note as well, in case
 // the report is resurrected as valid later on.
 if (EnableNullFPSuppression &&
-Options.shouldAvoidSuppressingNullArgumentPaths())
+   

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

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

Szelethus added a comment.

Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be 
an issue there?




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:59
+}
+}
+

`// end of namespace llvm`



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:210
+  if (!hasEqualKnownFields(TripleTo, TripleFrom)) {
+// TODO pass the SourceLocation of the CallExpression for more precise
+// diagnostics

`TODO: `



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:214
+<< Unit->getMainFileName() << TripleTo.str() << TripleFrom.str();
+// TODO Add statistics here
+return llvm::make_error(index_error_code::triple_mismatch);

Use punctuation.


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] D52988: [analyzer][PlistMacroExpansion] Part 5.: Support for # and ##

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348025: [analyzer][PlistMacroExpansion] Part 5.: Support for 
# and ## (authored by Szelethus, committed by ).
Herald added subscribers: llvm-commits, gamesh411.

Changed prior to commit:
  https://reviews.llvm.org/D52988?vs=170073&id=176170#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D52988

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp

Index: cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
===
--- cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
+++ cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
@@ -345,9 +345,17 @@
   *ptr = 5; // expected-warning{{Dereference of null pointer}}
 }
 
-// TODO: Should expand correctly.
 // CHECK: nameDECLARE_FUNC_AND_SET_TO_NULL
-// CHECK-NEXT: expansionvoid generated_##whatever(); ptr = nullptr;
+// CHECK-NEXT: expansionvoid generated_whatever(); ptr = nullptr;
+
+void macroArgContainsHashHashInStringTest() {
+  int *a;
+  TO_NULL_AND_PRINT(a, "Will this ## cause a crash?");
+  *a = 5; // expected-warning{{Dereference of null pointer}}
+}
+
+// CHECK: nameTO_NULL_AND_PRINT
+// CHECK-NEXT: expansiona = 0; print( "Will this ## cause a crash?")
 
 #define PRINT_STR(str, ptr) \
   print(#str);  \
@@ -359,9 +367,17 @@
   *ptr = 5; // expected-warning{{Dereference of null pointer}}
 }
 
-// TODO: Should expand correctly.
 // CHECK: namePRINT_STR
-// CHECK-NEXT: expansionprint(#Hello); ptr = nullptr
+// CHECK-NEXT: expansionprint("Hello"); ptr = nullptr
+
+void macroArgContainsHashInStringTest() {
+  int *a;
+  TO_NULL_AND_PRINT(a, "Will this # cause a crash?");
+  *a = 5; // expected-warning{{Dereference of null pointer}}
+}
+
+// CHECK: nameTO_NULL_AND_PRINT
+// CHECK-NEXT: expansiona = 0; print( "Will this # cause a crash?")
 
 //===--===//
 // Tests for more complex macro expansions.
Index: cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -4555,7 +4555,7 @@
   file0
  
  nameDECLARE_FUNC_AND_SET_TO_NULL
- expansionvoid generated_##whatever(); ptr = nullptr;
+ expansionvoid generated_whatever(); ptr = nullptr;
 

descriptionDereference of null pointer (loaded from variable 'ptr')
@@ -4595,12 +4595,12 @@
 start
  
   
-   line357
+   line352
col3
file0
   
   
-   line357
+   line352
col5
file0
   
@@ -4608,12 +4608,181 @@
 end
  
   
-   line358
+   line353
col3
file0
   
   
-   line358
+   line353
+   col19
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line353
+  col3
+  file0
+ 
+ ranges
+ 
+   
+
+ line353
+ col3
+ file0
+
+
+ line353
+ col53
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Null pointer value stored to 'a'
+ message
+ Null pointer value stored to 'a'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line354
+   col3
+   file0
+  
+  
+   line354
+   col3
+   file0
+  
+ 
+end
+ 
+  
+   line354
+   col6
+   file0
+  
+  
+   line354
+   col6
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line354
+  col6
+  file0
+ 
+ ranges
+ 
+   
+
+ line354
+ col4
+ file0
+
+
+ line354
+ col4
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Dereference of null pointer (loaded from variable 'a')
+ message
+ Dereference of null pointer (loaded from variable 'a')
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line353
+  col3
+  file0
+ 
+ nameTO_NULL_AND_PRINT
+ expansiona = 0; print( "Will this ## cause a crash?")
+
+   
+   descriptionDereference of null pointer (loaded from variable 'a')
+   

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

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

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

The idea is great!

I think this should rather be an -analyzer-config flag, since the actual 
analysis changes with a new output (refer to `AnalyzerOption`'s doxygen 
comments). Please note that I'm about to land some patches that modifies 
`AnalyzerOptions.def` quite a bit, but rebasing shouldn't be too bad anyways.

Also, almost everywhere CTU is capitalized, so I guess it should be in the 
field name too.




Comment at: test/Analysis/ctu-main.cpp:6
 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir 
-verify %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir 
-analyzer-display-ctu-progress 2>&1 %s | FileCheck %s
+

I think these RUN lines would really benefit from introducing line breaks.


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] D55131: [CTU] Add more lit tests and better error handling

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

Szelethus added a comment.

Also, maybe it'd be worth making a CTU directory under `test/Analysis` for CTU 
related test files.


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] D55131: [CTU] Add more lit tests and better error handling

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

Szelethus added inline comments.



Comment at: test/Analysis/ctu-main.c:3-5
+// 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

Could you please break these lines?
```
// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu %S/Inputs/ctu-other.c \
// RUN:-emit-pch -o %t/ctudir2/ctu-other.c.ast
//
// RUN: cp %S/Inputs/externalFnMap2.txt %t/ctudir2/externalFnMap.txt
//
// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze 
-verify %s
// RUN:   -analyzer-checker=core \
// RUN:   -analyzer-checker=debug.ExprInspection \
// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
// RUN:   -analyzer-config ctu-dir=%t/ctudir2
```



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

This is a question rather than anything else, why do we have both 
externalFnMap2.txt and externalFnMap.txt?


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] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

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

Szelethus added a comment.

@JonasToth this is the `Lexer` based expression equality check I talked about 
in D54757#1311516 .  The point of this 
patch is that the definition is a macro sure could be build dependent, but the 
macro name is not, so it wouldn't warn on the case I showed. What do you think?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55125



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

[PATCH] D53692: [analyzer] Evaluate all non-checker config options before analysis

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

Szelethus added a comment.

In D53692#1313956 , @NoQ wrote:

> In D53692#1293781 , @NoQ wrote:
>
> > In D53692#1293778 , @Szelethus 
> > wrote:
> >
> > > Did you know that clang unit tests do not run with check-clang-analysis?
> >
> >
> > Well, *some* unit tests definitely do run under check-clang-analysis.
>
>
> I think they're actually //compiled// but indeed not //run//. Ugh. I guess it 
> should be fixed.


Uhh, yea, forgot to send my comment with the same observation. I tried to fix 
it myself, but a day and a half of CMake made me appreciate even preprocessor 
macros more then ever.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53692



___
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

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

Szelethus marked 2 inline comments as done.
Szelethus added a comment.

Thanks!




Comment at: lib/Frontend/CompilerInvocation.cpp:363
+  A->claim();
   Opts.Config[key] = val;
 }

NoQ wrote:
> Maybe we can eventually turn this into an array and address by index that's 
> auto-generated from the options table.
> 
> But that's pretty unimportant; even though we can query options pretty often, 
> i don't think they are a performance bottleneck.
Actually, `ConfigTable` will probably end up on the chopping board by the end 
of the refactoring.


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] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

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

Szelethus accepted this revision.
Szelethus added a comment.

In D54557#1313382 , @NoQ wrote:

> In D54557#1301896 , @Szelethus wrote:
>
> > Wasn't the point of this patch to turn off part of this checkers 
> > functionality because it's immature just yet? From what I understand it is 
> > desired, but the FP rate is a little too high. I guess fixing that is the 
> > project.
> >
> > Hmm, actually, tinkering with HTML files might be overkill, especially 
> > since sphinx will hopefully end that era. Let's just add a TODO and let me 
> > deal with it later when I reorganize the checker options. Sorry for talking 
> > nonsense :D
>
>
> I would probably fine with shipping this other part as a lint check if we 
> start shipping lint checks. I don't have any specific improvements in mind 
> for that part. If somebody is willing to enforce "don't ever use things after 
> move" policy in his code, this will be a good checker for such person. At the 
> same time, i don't see how to improve that part so much that it can be 
> enabled by default. I think projects that consist of "i believe it's 
> impossible but you can try to come up with something" kind aren't very good 
> for the open projects list because the person who will try to work on them 
> may get disappointed when he realizes that his few at-a-glance solutions are 
> bad and in fact nobody ever had any better solutions in mind.


Yea, that makes sense.


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

https://reviews.llvm.org/D54557



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

[PATCH] D52986: [analyzer][PlistMacroExpansion] Part 4.: Support for __VA_ARGS__

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347888: [analyzer][PlistMacroExpansion] Part 4.: Support for 
__VA_ARGS__ (authored by Szelethus, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D52986

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  test/Analysis/plist-macros-with-expansion.cpp

Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -4217,7 +4217,7 @@
   file0
  
  nameVARIADIC_SET_TO_NULL
- expansionptr = nullptr; variadicFunc( 1)
+ expansionptr = nullptr; variadicFunc( 1, 5, "haha!")
 

descriptionDereference of null pointer (loaded from variable 'ptr')
@@ -4257,12 +4257,12 @@
 start
  
   
-   line333
+   line324
col3
file0
   
   
-   line333
+   line324
col5
file0
   
@@ -4270,12 +4270,181 @@
 end
  
   
-   line334
+   line327
col3
file0
   
   
-   line334
+   line327
+   col22
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line327
+  col3
+  file0
+ 
+ ranges
+ 
+   
+
+ line327
+ col3
+ file0
+
+
+ line327
+ col27
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Null pointer value stored to 'ptr'
+ message
+ Null pointer value stored to 'ptr'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line328
+   col3
+   file0
+  
+  
+   line328
+   col3
+   file0
+  
+ 
+end
+ 
+  
+   line328
+   col8
+   file0
+  
+  
+   line328
+   col8
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line328
+  col8
+  file0
+ 
+ ranges
+ 
+   
+
+ line328
+ col4
+ file0
+
+
+ line328
+ col6
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Dereference of null pointer (loaded from variable 'ptr')
+ message
+ Dereference of null pointer (loaded from variable 'ptr')
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line327
+  col3
+  file0
+ 
+ nameVARIADIC_SET_TO_NULL
+ expansionptr = nullptr; variadicFunc()
+
+   
+   descriptionDereference of null pointer (loaded from variable 'ptr')
+   categoryLogic error
+   typeDereference of null pointer
+   check_namecore.NullDereference
+   
+   issue_hash_content_of_line_in_context6aa30fd6a1e997027333f16c2064d973
+  issue_context_kindfunction
+  issue_contextvariadicMacroArgumentWithoutAnyArgumentTest
+  issue_hash_function_offset5
+  location
+  
+   line328
+   col8
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+323
+324
+327
+328
+   
+  
+  
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line343
+   col3
+   file0
+  
+  
+   line343
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line344
+   col3
+   file0
+  
+  
+   line344
col30
file0
   
@@ -4287,7 +4456,7 @@
  kindevent
  location
  
-  line334
+  line344
   col3
   file0
  
@@ -4295,12 +4464,12 @@
  

 
- line334
+ line344
  col3
  file0
 
 
- line334
+ line344
  col45
  file0
 
@@ -4320,12 +4489,12 @@
 start
  
   
-   line335
+   line345
col3
file0
   
   
-   line335
+   line345
col3
file0
   
@@ -4333,12 +4502,12 @@
 end
  
   
-   line335
+   line345
col8
file0
   
   
-   line335
+   line345
col8
file0
   
@@ -4350,7 +4519,7 @@
  kindevent
  location
  
-  line335
+  l

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

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

Szelethus added a comment.

In D55051#1312666 , 
@baloghadamsoftware wrote:

> In D55051#1312660 , @Szelethus wrote:
>
> > Have you seen a crash resulting from this? Is supplying a test case 
> > feasable? I know that some of these errors are extremely hard to reproduce.
> >
> > Edit: Nevertheless, looks good to me.
>
>
> There could be probably a test provided, but the bug is very obvious if you 
> look at the code. Also if you compare it with the first version of the 
> original patch.


I'm sold on that. Let's wait for someone else then to formally accept. Please 
note that AFAIK the merge request deadline for 7.0.1 is tomorrow (nov. 30), so 
if putting this in is critical, writing a cfe-dev mail with a scary title may 
be a good idea.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55051



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

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

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

Szelethus added a comment.

Have you seen a crash resulting from this? Is supplying a test case feasable? I 
know that some of these errors are extremely hard to reproduce.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55051



___
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

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

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2348
+  // Enable compatilibily mode to avoid analyzer-config related errors.
+  CmdArgs.push_back("-analyzer-config-compatibility-mode=true");
+

xazax.hun wrote:
> I wonder if it makes more sense to not add this here but rather make the 
> default option to be true.
We resolved this IRL, but just for the record, this line will run when the 
analyzer is invoked with `--analyze` (essentially, in driver mode), in which 
case we wouldn't like to receive errors on invalid input, but with `-cc1` 
(frontend mode), it will be disabled by default .


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] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

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

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

Changes are no longer planned for the reasons explained in my earlier comments.


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] D52986: [analyzer][PlistMacroExpansion] Part 4.: Support for __VA_ARGS__

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

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:884
+  // even if we lex a tok::comma and ParanthesesDepth == 1.
+  const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__");
+

Szelethus wrote:
> xazax.hun wrote:
> > Is it possible to undef `__VA_ARGS__`? If so, can this cause problems? 
> > Should we use `findDirectiveAtLoc` instead?
> Luckily, no :)
> 
> But, even if you could, we're not looking for a `MacroInfo` (through 
> `MacroDirective`), but rather an `IdentifierInfo`, and those won't disappear 
> even if you undef a macro.
Hmmm, simply acquiring the range of tokens `__VA_ARGS__` is defined to in the 
expansion context would be neat, but it doesn't seem to be possible. It seems 
like `__VA_ARGS__` just doesn't have a `MacroDirective`.

In fact, if you CTRL+F "variadic" on `Preprocessor`'s doxygen page, you get 
basically nothing (if you look for `__VA_ARGS__`, you get //literally// 
nothing). So working with the most primitive tools available is the best I 
can do here, I'm afraid :/


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

https://reviews.llvm.org/D52986



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

[PATCH] D52986: [analyzer][PlistMacroExpansion] Part 4.: Support for __VA_ARGS__

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

Szelethus marked 2 inline comments as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:884
+  // even if we lex a tok::comma and ParanthesesDepth == 1.
+  const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__");
+

xazax.hun wrote:
> Is it possible to undef `__VA_ARGS__`? If so, can this cause problems? Should 
> we use `findDirectiveAtLoc` instead?
Luckily, no :)

But, even if you could, we're not looking for a `MacroInfo` (through 
`MacroDirective`), but rather an `IdentifierInfo`, and those won't disappear 
even if you undef a macro.


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

https://reviews.llvm.org/D52986



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

[PATCH] D52986: [analyzer][PlistMacroExpansion] Part 4.: Support for __VA_ARGS__

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

Szelethus updated this revision to Diff 175718.
Szelethus added a comment.
Herald added a subscriber: gamesh411.

- Fixed a crash where no arguments (**not** empty arguments) were supplied to 
`__VA_ARGS__`.
- Rebased to master.


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

https://reviews.llvm.org/D52986

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  test/Analysis/plist-macros-with-expansion.cpp

Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -317,9 +317,19 @@
   *ptr = 5; // expected-warning{{Dereference of null pointer}}
 }
 
-// TODO: Should correctly display the rest of the parameters.
 // CHECK: nameVARIADIC_SET_TO_NULL
-// CHECK-NEXT: expansionptr = nullptr; variadicFunc( 1)
+// CHECK-NEXT: expansionptr = nullptr; variadicFunc( 1, 5, "haha!")
+
+void variadicMacroArgumentWithoutAnyArgumentTest() {
+  int *ptr;
+  // Not adding a single parameter to ... is silly (and also causes a
+  // preprocessor warning), but is not an excuse to crash on it.
+  VARIADIC_SET_TO_NULL(ptr);
+  *ptr = 5; // expected-warning{{Dereference of null pointer}}
+}
+
+// CHECK: nameVARIADIC_SET_TO_NULL
+// CHECK-NEXT: expansionptr = nullptr; variadicFunc()
 
 //===--===//
 // Tests for # and ##.
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -4217,7 +4217,7 @@
   file0
  
  nameVARIADIC_SET_TO_NULL
- expansionptr = nullptr; variadicFunc( 1)
+ expansionptr = nullptr; variadicFunc( 1, 5, "haha!")
 

descriptionDereference of null pointer (loaded from variable 'ptr')
@@ -4257,12 +4257,12 @@
 start
  
   
-   line333
+   line324
col3
file0
   
   
-   line333
+   line324
col5
file0
   
@@ -4270,12 +4270,181 @@
 end
  
   
-   line334
+   line327
col3
file0
   
   
-   line334
+   line327
+   col22
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line327
+  col3
+  file0
+ 
+ ranges
+ 
+   
+
+ line327
+ col3
+ file0
+
+
+ line327
+ col27
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Null pointer value stored to 'ptr'
+ message
+ Null pointer value stored to 'ptr'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line328
+   col3
+   file0
+  
+  
+   line328
+   col3
+   file0
+  
+ 
+end
+ 
+  
+   line328
+   col8
+   file0
+  
+  
+   line328
+   col8
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line328
+  col8
+  file0
+ 
+ ranges
+ 
+   
+
+ line328
+ col4
+ file0
+
+
+ line328
+ col6
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Dereference of null pointer (loaded from variable 'ptr')
+ message
+ Dereference of null pointer (loaded from variable 'ptr')
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line327
+  col3
+  file0
+ 
+ nameVARIADIC_SET_TO_NULL
+ expansionptr = nullptr; variadicFunc()
+
+   
+   descriptionDereference of null pointer (loaded from variable 'ptr')
+   categoryLogic error
+   typeDereference of null pointer
+   check_namecore.NullDereference
+   
+   issue_hash_content_of_line_in_context6aa30fd6a1e997027333f16c2064d973
+  issue_context_kindfunction
+  issue_contextvariadicMacroArgumentWithoutAnyArgumentTest
+  issue_hash_function_offset5
+  location
+  
+   line328
+   col8
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+323
+324
+327
+328
+   
+  
+  
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line343
+   col3
+   file0
+  
+  
+   line343
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line344
+ 

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

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

Szelethus added a comment.

In D54757#1311468 , @donat.nagy wrote:

> **Macros:**
>
> The current implementation of the check only looks at the preprocessed code, 
> therefore it detects the situations where the duplication is created by 
> macros. I added some tests to highlight this behavior.


The only option I see to detect macro related errors is to use a `Lexer`, and 
compare the tokens one by one, but that might be overkill, and I'm aware of a 
check in the works that already implements that. I think it's perfectly fine 
not to cover those cases, but you should definitely document it somewhere, 
preferably in the rst file.

> I think that in some situations this would be useful for detecting redundant 
> or incorrect parts of macro-infected code.

How about this?

  #define PANDA_CUTE_FACTOR 9001
  #define CAT_CUTE_FACTOR 9001
  
  if (whatever())
return PANDA_CUTE_FACTOR;
  else
return CAT_CUTE_FACTOR;

Your check would warn here, but it is possible, if not likely that the 
definition of those macros will eventually change. Again, it's fine not to 
cover this, but this //is// a false positive in a sense.

I have to agree with the folks before me, this patch is amazing, great job!




Comment at: test/clang-tidy/bugprone-branch-clone.cpp:200
+
+void test_macro1(int in, int &out) {
+  PASTE_CODE(

donat.nagy wrote:
> The tests for macro handling start here.
Maybe add dividers? :) The `//===--===//` thing, you know.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend

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

Szelethus added a comment.

Ping^2


Repository:
  rC Clang

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

https://reviews.llvm.org/D54436



___
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

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

Szelethus added a comment.

Ping^2


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] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

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

Szelethus added a comment.

Thanks! I'll commit around friday (with the requested fixes) to leave enough 
time for everyone to object.


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] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

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

Szelethus added a comment.

> @george.karpenkov Matching macros is a very non-trivial job, how would you 
> feel if we shipped this patch as-is, and maybe leave a TODO about adding 
> macro `assert`s down the line?

The only solution I saw in clang-tidy was to match binary expressions as a 
heuristic, and check whether they are inside a macro named assert. That is 
hacky at best, any objection against the current state of this patch?

I'm planning to evaluate the checker in a variety of projects after this one, 
see how things are looking, and push it out of alpha.


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] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

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

Szelethus added a comment.

Polite ping. :) I'd be most comfortable landing D53692 
 together with this patch.


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] D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry

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

Szelethus added a comment.

Polite ping :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D54437



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

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

Szelethus added a comment.

I think the reason why the printed message was either along the lines of "this 
is 0" and "this is non-0", is that we don't necessarily know what constraint 
solver we're using, and adding more non-general code `BugReporter` is most 
probably a bad approach. How about we tinker with `ContraintManager` a little 
more, maybe add an `explainValueOfVarDecl`, that would return the value of a 
variable in a given `ProgramState`? We could implement it for 
`RangeConstraintSolver` (essentially move the code you already wrote there), 
and leave a `TODO` with a super generic implementation for Z3.

Although, I can't quite write an essay on top of my head about golden rules of 
constraint solving, so take my advice with a grain of salt.




Comment at: 
include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:164
 class ConditionBRVisitor final : public BugReporterVisitor {
+  bool IsAssuming;
+

We only get to know what this field is for while reading the actual 
implementation, please add comments.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1892-1920
+  const auto CurrentCR = N->getState()->get();
+  const auto PredCR = PredState->get();
+  const auto PredPredCR = PredPredState->get();
+
+  // We only want to check BlockEdges once so we have to determine if the 
change
+  // of the range information is not happened because of dead symbols.
+  //

`ConstraintRange`, as far as I know, is the data  `RangedConstraintManager` 
stores in the GDM. What if we're using a different constraint solver? I think 
it'd be better to take the more abstract approach, extend `ConstraintManager`'s 
interface with an `isEqual` or `operator==` pure virtual method, and implement 
it within it's descendants.


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

https://reviews.llvm.org/D53076



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

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

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

Szelethus added a comment.

Hmmm, I was wrong a little bit: I realized that if I tinker with with 
`initializeManager` and `getEnabledCheckers` just a bit more, register checkers 
straight away, don't need to collect them first, and this would make sure that 
dependencies are registered before the checkers that depend on them. That means 
I can get away this patch without even touching the MallocChecker family. 
`CheckerManager` would receive the `getChecker` function that would assert if 
the checker isn't already registered, so we could be extra sure this system 
works.

Bt since I spent a lot of time with MallocChecker, I might tickle the 
splitting problem later on as a weekend project or something.


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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

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

Szelethus marked 3 inline comments 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();

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.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
   if (!State)

MTC wrote:
> Szelethus wrote:
> > MTC wrote:
> > > `const` qualifier missing?
> > This method was made `static`.
> You are right, sorry for my oversight!
Actually, I forgot about it 10 times when I re-read the patch, no shame in that 
:)



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) {
 

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.


Repository:
  rC Clang

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] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347513: [analyzer] INT50-CPP. Do not cast to an out-of-range 
enumeration checker (authored by Szelethus, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D33672?vs=172516&id=175154#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D33672

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  test/Analysis/enum-cast-out-of-range.cpp
  www/analyzer/alpha_checks.html

Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -34,6 +34,7 @@
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
+  EnumCastOutOfRangeChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
   GCDAntipatternChecker.cpp
Index: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -0,0 +1,128 @@
+//===- EnumCastOutOfRangeChecker.cpp ---*- C++ -*--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// The EnumCastOutOfRangeChecker is responsible for checking integer to
+// enumeration casts that could result in undefined values. This could happen
+// if the value that we cast from is out of the value range of the enumeration.
+// Reference:
+// [ISO/IEC 14882-2014] ISO/IEC 14882-2014.
+//   Programming Languages — C++, Fourth Edition. 2014.
+// C++ Standard, [dcl.enum], in paragraph 8, which defines the range of an enum
+// C++ Standard, [expr.static.cast], paragraph 10, which defines the behaviour
+//   of casting an integer value that is out of range
+// SEI CERT C++ Coding Standard, INT50-CPP. Do not cast to an out-of-range
+//   enumeration value
+//===--===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+// This evaluator checks two SVals for equality. The first SVal is provided via
+// the constructor, the second is the parameter of the overloaded () operator.
+// It uses the in-built ConstraintManager to resolve the equlity to possible or
+// not possible ProgramStates.
+class ConstraintBasedEQEvaluator {
+  const DefinedOrUnknownSVal CompareValue;
+  const ProgramStateRef PS;
+  SValBuilder &SVB;
+
+public:
+  ConstraintBasedEQEvaluator(CheckerContext &C,
+ const DefinedOrUnknownSVal CompareValue)
+  : CompareValue(CompareValue), PS(C.getState()), SVB(C.getSValBuilder()) {}
+
+  bool operator()(const llvm::APSInt &EnumDeclInitValue) {
+DefinedOrUnknownSVal EnumDeclValue = SVB.makeIntVal(EnumDeclInitValue);
+DefinedOrUnknownSVal ElemEqualsValueToCast =
+SVB.evalEQ(PS, EnumDeclValue, CompareValue);
+
+return static_cast(PS->assume(ElemEqualsValueToCast, true));
+  }
+};
+
+// This checker checks CastExpr statements.
+// If the value provided to the cast is one of the values the enumeration can
+// represent, the said value matches the enumeration. If the checker can
+// establish the impossibility of matching it gives a warning.
+// Being conservative, it does not warn if there is slight possibility the
+// value can be matching.
+class EnumCastOutOfRangeChecker : public Checker> {
+  mutable std::unique_ptr EnumValueCastOutOfRange;
+  void reportWarning(CheckerContext &C) const;
+
+public:
+  void checkPreStmt(const CastExpr *CE, CheckerContext &C) const;
+};
+
+using EnumValueVector = llvm::SmallVector;
+
+// Collects all of the values an enum can represent (as SVals).
+EnumValueVector getDeclValuesForEnum(const EnumDecl *ED) {
+  EnumValueVector DeclValues(
+  std::distance(ED->enumerator_begin(), ED->enumerator_end()));
+  llvm::transform(ED->enumerators(), DeclValues.begin(),
+ [](const EnumConstantDecl *D) { return D->getInitVal(); });
+  return DeclValues;
+}
+} // namespace
+
+void EnumCastOutOfRangeChecker::reportWarning(CheckerContext &C) const {
+  if (const ExplodedNode *N = C.generateNonFatalErrorNode()) {
+if (!EnumValueCastOutOfRange)
+  EnumValueCastOutOfRange.reset(
+  new BuiltinBug(this, "Enum cast out of range",
+ "The value provided to

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

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

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

I'll commit tomorrow, when I have time to keep an eye out for buildbots. Thanks!


https://reviews.llvm.org/D33672



___
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

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

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:

Szelethus wrote:
> Szelethus wrote:
> > MTC wrote:
> > > I can't say that abstracting `MemFunctionInfo` is a perfect solution, for 
> > > example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` 
> > > are close to each other in the object layout, but they do the same thing, 
> > > which makes me feel weird. And there are so many `MemFunctionInfo.` :)
> > Well the thinking here was that `MallocChecker` is seriously overcrowded -- 
> > it's a one-tool-to-solve-everything class, and moving these 
> > `IdentifierInfos` in their separate class was pretty much the first step I 
> > took: I think it encapsulates a particular task well and eases a lot on 
> > `MallocChecker`. Sure `MemFunctionInfo.` is reduntant, but each time you 
> > see it, it indicates that we're calling a function or using a data member 
> > that has no effect on the actual analysis, that we're inquiring about more 
> > information about a given function. and nothing more. For a checker that 
> > emits warning at seemingly random places wherever, I think this is valuable.
> > 
> > By the way, it also forces almost all similar conditions in a separate 
> > line, which is an unexpected, but pleasant help to the untrained eye:
> > ```
> > if (Fun == MemFunctionInfo.II_malloc ||
> > Fun == MemFunctionInfo.II_whatever)
> > ```
> > Nevertheless, this is the only change I'm not 100% confident about, if you 
> > and/or others disagree, I'll happily revert it.
> (leaving a separate inline for a separate topic)
> The was this checker abuses checker options isn't nice at all, I agree. I 
> could just rename `MallocChecker::IsOptimistic` to 
> `MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it passed 
> around as a parameter in `MemFunctionInfoTy`. Would you prefer that, should 
> you be okay with `MemFunctionInfoTy` in general?
The way* this checker abuses 


Repository:
  rC Clang

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

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

Szelethus added a comment.

And thank you for helping me along the way! :D




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:

MTC wrote:
> I can't say that abstracting `MemFunctionInfo` is a perfect solution, for 
> example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` are 
> close to each other in the object layout, but they do the same thing, which 
> makes me feel weird. And there are so many `MemFunctionInfo.` :)
Well the thinking here was that `MallocChecker` is seriously overcrowded -- 
it's a one-tool-to-solve-everything class, and moving these `IdentifierInfos` 
in their separate class was pretty much the first step I took: I think it 
encapsulates a particular task well and eases a lot on `MallocChecker`. Sure 
`MemFunctionInfo.` is reduntant, but each time you see it, it indicates that 
we're calling a function or using a data member that has no effect on the 
actual analysis, that we're inquiring about more information about a given 
function. and nothing more. For a checker that emits warning at seemingly 
random places wherever, I think this is valuable.

By the way, it also forces almost all similar conditions in a separate line, 
which is an unexpected, but pleasant help to the untrained eye:
```
if (Fun == MemFunctionInfo.II_malloc ||
Fun == MemFunctionInfo.II_whatever)
```
Nevertheless, this is the only change I'm not 100% confident about, if you 
and/or others disagree, I'll happily revert it.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:

Szelethus wrote:
> MTC wrote:
> > I can't say that abstracting `MemFunctionInfo` is a perfect solution, for 
> > example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` are 
> > close to each other in the object layout, but they do the same thing, which 
> > makes me feel weird. And there are so many `MemFunctionInfo.` :)
> Well the thinking here was that `MallocChecker` is seriously overcrowded -- 
> it's a one-tool-to-solve-everything class, and moving these `IdentifierInfos` 
> in their separate class was pretty much the first step I took: I think it 
> encapsulates a particular task well and eases a lot on `MallocChecker`. Sure 
> `MemFunctionInfo.` is reduntant, but each time you see it, it indicates that 
> we're calling a function or using a data member that has no effect on the 
> actual analysis, that we're inquiring about more information about a given 
> function. and nothing more. For a checker that emits warning at seemingly 
> random places wherever, I think this is valuable.
> 
> By the way, it also forces almost all similar conditions in a separate line, 
> which is an unexpected, but pleasant help to the untrained eye:
> ```
> if (Fun == MemFunctionInfo.II_malloc ||
> Fun == MemFunctionInfo.II_whatever)
> ```
> Nevertheless, this is the only change I'm not 100% confident about, if you 
> and/or others disagree, I'll happily revert it.
(leaving a separate inline for a separate topic)
The was this checker abuses checker options isn't nice at all, I agree. I could 
just rename `MallocChecker::IsOptimistic` to 
`MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it passed 
around as a parameter in `MemFunctionInfoTy`. Would you prefer that, should you 
be okay with `MemFunctionInfoTy` in general?



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

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.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
   if (!State)

MTC wrote:
> `const` qualifier missing?
This method was made `static`.



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) {
 
--

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

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

Szelethus added a comment.

Let's also have the link to your most recent mail about this issue here: 
http://lists.llvm.org/pipermail/cfe-dev/2018-November/060038.html

I re-read the mailing list conversation from 2016, @szepet's 
`MisusedMoveChecker` patch, so I have a better graps on whats happening.

I think this really is non-trivial stuff. When I initially read your workbook, 
`SymbolReaper` (along with what inlining really is) was among the biggest 
unsolved mysteries for me. The explanation of it you wrote back in 2016[1] is 
//awesome//, I feel enlightened after reading it, it's very well constructed, 
but I find it unfortunate that it took me almost a year of working in the 
analyzer to find it. After looking at `SymbolManager.cpp`'s history, I can see 
that the logic didn't change since, I'd very strongly prefer to have this 
goldnugget copy-pasted even to a simple txt file, and have it commited with 
this patch.

[1] http://lists.llvm.org/pipermail/cfe-dev/2016-March/048142.html

In https://reviews.llvm.org/D18860#1306359, @Szelethus wrote:

> In https://reviews.llvm.org/D18860#1297742, @NoQ wrote:
>
> > Nope, unit tests aren't quite useful here. I can't unit-test the behavior 
> > of API that i'm about to //remove//... right?
>
>
> Hmmm, can't really argue with that, can I? :D


Well, we probably should implement unit tests for these anyways, `SymbolReaper` 
is not only performance-critical, but is also among the few data structures 
that's an essential part of the analysis, and it's change in functionality 
could be very easily shown in dedicated test files. But that's an issue for 
another time, and until then, `debug.ExprInspection` still does the job well 
enough. Let's not delay this patch longer just because of that, and enjoy the 
post zombie apocalypse analyzer.


https://reviews.llvm.org/D18860



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

[PATCH] D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions.

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

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2383-2384
 
+  if (RS == OldRS)
+return;
+

Hmmm, I guess we return here because if `RegionState` is unchanged, so should 
be `ReallocPairs` and `FreeReturnValue`, correct?


https://reviews.llvm.org/D54013



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

[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

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

Szelethus added a comment.

In https://reviews.llvm.org/D18860#1297742, @NoQ wrote:

> Nope, unit tests aren't quite useful here. I can't unit-test the behavior of 
> API that i'm about to //remove//... right?


Hmmm, can't really argue with that, can I? :D


https://reviews.llvm.org/D18860



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

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

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

Szelethus added a comment.

In https://reviews.llvm.org/D54563#1301893, @NoQ wrote:

> In https://reviews.llvm.org/D54563#1299918, @Szelethus wrote:
>
> > How about `std::list::splice`?
>
>
> Hmm, you mean that it leaves its argument empty? Interesting, i guess we may 
> need a separate facility for that.


I looked it up, and this is what I found: (source: 
http://eel.is/c++draft/list.ops)

> `list` provides three `splice` operations that destructively move elements 
> from one list to another. The behavior of splice operations is undefined if 
> `get_­allocator() != x.get_­allocator()`.
> 
>   void splice(const_iterator position, list& x);
>   void splice(const_iterator position, list&& x);
> 
> **Effects:** Inserts the contents of `x` before position and `x` becomes 
> empty. Pointers and references to the moved elements of `x` now refer to 
> those same elements but as members of *this. Iterators referring to the moved 
> >elements will continue to refer to their elements, but they now behave as 
> iterators into `*this`, not into `x`.

This suggests to me that `list1.splice(list1.begin(), std::move(list2))` leaves 
 `list` in a valid, well-defined, but empty state.

>   I wonder how many other state-reset methods are we forgetting.

`merge` does something similar too for `map`, `multimap`, `set`, `multiset`, as 
well as their unordered counterparts.

> As a TODO, we might want to warn anyway when the value of the argument is not 
> 0.

Maybe leave a `TODO` in the code as well? ^-^


Repository:
  rC Clang

https://reviews.llvm.org/D54563



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

[PATCH] D54834: [analyzer][MallocChecker] Improve warning messages on double-delete errors

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

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

Repository:
  rC Clang

https://reviews.llvm.org/D54834

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
  test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/NewDelete-path-notes.cpp
  test/Analysis/dtor.cpp

Index: test/Analysis/dtor.cpp
===
--- test/Analysis/dtor.cpp
+++ test/Analysis/dtor.cpp
@@ -528,7 +528,7 @@
 return *this;
   }
   ~NonTrivial() {
-delete[] p; // expected-warning {{free released memory}}
+delete[] p; // expected-warning {{delete released memory}}
   }
 };
 
Index: test/Analysis/NewDelete-path-notes.cpp
===
--- test/Analysis/NewDelete-path-notes.cpp
+++ test/Analysis/NewDelete-path-notes.cpp
@@ -11,8 +11,8 @@
 delete p;
 // expected-note@-1 {{Memory is released}}
 
-  delete p; // expected-warning {{Attempt to free released memory}}
-  // expected-note@-1 {{Attempt to free released memory}}
+  delete p; // expected-warning {{Attempt to delete released memory}}
+  // expected-note@-1 {{Attempt to delete released memory}}
 }
 
 struct Odd {
@@ -24,7 +24,7 @@
 void test(Odd *odd) {
 	odd->kill(); // expected-note{{Calling 'Odd::kill'}}
// expected-note@-1 {{Returning; memory was released}}
-	delete odd; // expected-warning {{Attempt to free released memory}}
-  // expected-note@-1 {{Attempt to free released memory}}
+	delete odd; // expected-warning {{Attempt to delete released memory}}
+  // expected-note@-1 {{Attempt to delete released memory}}
 }
 
Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -182,7 +182,7 @@
 void testDoubleDelete() {
   int *p = new int;
   delete p;
-  delete p; // expected-warning{{Attempt to free released memory}}
+  delete p; // expected-warning{{Attempt to delete released memory}}
 }
 
 void testExprDeleteArg() {
Index: test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
===
--- test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
+++ test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
@@ -46,7 +46,7 @@
 void testNewDoubleFree() {
   int *p = new int;
   delete p;
-  delete p; // expected-warning{{Attempt to free released memory}}
+  delete p; // expected-warning{{Attempt to delete released memory}}
 }
 
 void testNewLeak() {
Index: test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
+++ test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
@@ -194,17 +194,17 @@
  
  depth0
  extended_message
- Attempt to free released memory
+ Attempt to delete released memory
  message
- Attempt to free released memory
+ Attempt to delete released memory
 

-   descriptionAttempt to free released memory
+   descriptionAttempt to delete released memory
categoryMemory error
-   typeDouble free
+   typeDouble delete
check_namecplusplus.NewDelete

-   issue_hash_content_of_line_in_contextbd8e324d09c70b9e2be6f824a4942e5a
+   issue_hash_content_of_line_in_context593b185245106bed5175ccf2753039b2
   issue_context_kindfunction
   issue_contexttest
   issue_hash_function_offset8
@@ -423,17 +423,17 @@
  
  depth0
  extended_message
- Attempt to free released memory
+ Attempt to delete released memory
  message
- Attempt to free released memory
+ Attempt to delete released memory
 

-   descriptionAttempt to free released memory
+   descriptionAttempt to delete released memory
categoryMemory error
-   typeDouble free
+   typeDouble delete
check_namecplusplus.NewDelete

-   issue_hash_content_of_line_in_context8bf1a5b9fdae9d86780aa6c4cdce2605
+   issue_hash_content_of_line_in_context6484e9b006ede7362edef2187ba6eb37
   issue_context_kindfunction
   issue_contexttest
   issue_hash_function_offset3
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1435,7 +1435,8 @@
 
 void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE,
  CheckerContext &C) const {
-
+  // This will regard deleting freed() regions as a use-after-fr

[PATCH] D54466: [Analyzer] Iterator Checkers - Use the region of the topmost base class for iterators stored in a region

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

Szelethus added a comment.

In https://reviews.llvm.org/D54466#1305305, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D54466#1297887, @NoQ wrote:
>
> > > Hmmm, shouldn't we add this to `MemRegion`'s interface instead?
>
>
> This:
>
> > I wouldn't insist, but this does indeed sound useful. I suggest 
> > `MemRegion::getMostDerivedObjectRegion()` or something like that.
>
> vs
>
> > Also, ugh, that nomenclature: the base region of `CXXBaseObjectRegion` in 
> > fact represents the //derived// object.
>
> So if `CXXBaseObjectRegion` is the derived object, then 
> `MemRegion::getMostDerivedObjectRegion()` gets the least derived object 
> region now?


Hmm, are there any particular reasons against renaming it to `CXXDerivedRegion`?


https://reviews.llvm.org/D54466



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

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

Szelethus added inline comments.



Comment at: lib/AST/DeclBase.cpp:1469
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||

balazske wrote:
> martong wrote:
> > Szelethus wrote:
> > > Contained in?
> > Indeed, `containedInVector` sounds better, so I renamed.
> For me, `containsInVector` is the better name, or `hasInVector` ("contains" 
> is already used at other places but not "contained" and it sounds like it is 
> not contained any more).
Sorry about the confusion, my inline was only about the comment above it, that 
it isn't obvious enough that //what// decl is contained in. But after taking a 
second look, it's very clear that only `Map` is mutated in this context, so I 
don't insist on any modification :)


Repository:
  rC Clang

https://reviews.llvm.org/D53655



___
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

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

Szelethus added a comment.

I also have some very neat ideas how the split up should go, but I'd like to 
mature the idea in my head for just a bit longer.




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:24-27
+//   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.

Source: https://reviews.llvm.org/D7905



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:242-245
+/// This is important because realloc may fail, and that needs special 
modeling.
+/// Whether reallocation failed or not will not be known until later, so we'll
+/// store whether upon failure 'fromPtr' will be freed, or needs to be freed
+/// later, etc.

Source: http://lists.llvm.org/pipermail/cfe-dev/2018-November/060216.html



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:435-443
+  /// User-defined function may have the ownership_returns attribute, which
+  /// annotates that the function returns with an object that was allocated on
+  /// the heap, and passes the ownertship to the callee.
+  ///
+  ///   void __attribute((ownership_returns(malloc, 1))) *my_malloc(size_t);
+  ///
+  /// It has two parameters:

Source:
http://lists.llvm.org/pipermail/cfe-dev/2010-June/009600.html
https://github.com/llvm-mirror/clang/commit/dd0e490c24aeade2c59ca4cae171199f6af9f02e



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1359-1363
+  /// Non-trivial constructors have a chance to escape 'this', but marking all
+  /// invocations of trivial constructors as escaped would cause too great of
+  /// reduction of true positives, so let's just do that for constructor that
+  /// have an argument of a pointer-to-record type.
+  if (!PM.isConsumedExpr(NE) && hasNonTrivialConstructorCall(NE))

Source:
https://bugs.llvm.org/show_bug.cgi?id=19102
https://reviews.llvm.org/D4025


Repository:
  rC Clang

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

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

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

I'm in the process of splitting this checker into multiple other ones, and 
while I was there, I provided documentation to most functions, and made some 
static, and in some cases moved them out of class.

This patch merely reorganizes some things, and features no functional change.

In detail:

- Provided documentation, or moved existing documentation in more obvious 
places.
- Added dividers. (the `//===--===//` thing).
- Moved `getAllocationFamily`, `printAllocDeallocName`, 
`printExpectedAllocName` and `printExpectedDeallocName` in the global namespace 
on top of the file where `AllocationFamily` is declared, as they are very 
strongly related.
- Realloc modeling was very poor in terms of variable and structure naming, as 
well as documentation, so I renamed some of them and added much needed docs.
- Moved function `IdentifierInfo`s to a separate struct, and moved 
`isMemFunction`, `isCMemFunction` adn `isStandardNewDelete` inside it. This 
makes the patch affect quite a lot of lines, should I extract it to a separate 
one?
- Moved `MallocBugVisitor` out of `MallocChecker`.
- Preferred switches to long else-if branches in some places.
- Neatly organized some `RUN:` lines.


Repository:
  rC Clang

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  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: 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 it's 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 it's 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 dealloc

[PATCH] D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

Polite ping :)


Repository:
  rC Clang

https://reviews.llvm.org/D54436



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

[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

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

Szelethus added a comment.

Ping, @xazax.hun, any objections?


https://reviews.llvm.org/D52795



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

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

Szelethus added a comment.

In the meanwhile we managed to figure out where the problem lays, so if you're 
interested, I'm going to document it here.

The problem this patch attempts to solve is that `ConditionBRVisitor` adds 
event pieces to the bugpath when the state has changed from the previous state, 
not only when the `ConstraintManager` differs in between the changes [1]. The 
patch attempts to solve this by inspecting the actual condition, and tries to 
find out whether the symbols inside that condition has changed in between the 
states, but this is overkill solution, as comparing the two `ConstraintManager` 
objects would achieve the same thing. This should probably be executed by 
extending the interface of `ConstraintManager` with a comparison method.

[1] 
https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp#L1808


https://reviews.llvm.org/D53076



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

[PATCH] D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit.

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

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:385-386
+  }
+  // Provide the caller with the classification of the object
+  // we've obtained here accidentally, for later use.
+  return OK;

NoQ wrote:
> Szelethus wrote:
> > Maybe move this in-class?
> Mmm, what do you mean?
`explain.*` sounds like it either returns a string, or writes a stream object, 
but the return type isn't `void` nor string, maybe it'd be worth to put this 
comment in-class.

But yea, this is over the top nitpicking, I don't insist :)


https://reviews.llvm.org/D54560



___
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

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

Ping


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] D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347157: [analyzer][NFC] Move CheckerOptInfo to 
CheckerRegistry.cpp, and make it local (authored by Szelethus, committed by ).
Herald added subscribers: llvm-commits, gamesh411, baloghadamsoftware.

Changed prior to commit:
  https://reviews.llvm.org/D54397?vs=173568&id=174535#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54397

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

Index: cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -17,7 +17,6 @@
 #include "clang/StaticAnalyzer/Checkers/ClangCheckers.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/CheckerOptInfo.h"
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/StaticAnalyzer/Frontend/FrontendActions.h"
 #include "llvm/ADT/SmallVector.h"
@@ -102,44 +101,23 @@
   << pluginAPIVersion;
 }
 
-static SmallVector
-getCheckerOptList(const AnalyzerOptions &opts) {
-  SmallVector checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-const std::pair &opt = opts.CheckersControlList[i];
-checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
-  }
-  return checkerOpts;
-}
-
 std::unique_ptr ento::createCheckerManager(
 ASTContext &context,
 AnalyzerOptions &opts,
 ArrayRef plugins,
 ArrayRef> checkerRegistrationFns,
 DiagnosticsEngine &diags) {
   auto checkerMgr = llvm::make_unique(context, opts);
 
-  SmallVector checkerOpts = getCheckerOptList(opts);
-
   ClangCheckerRegistry allCheckers(plugins, &diags);
 
   for (const auto &Fn : checkerRegistrationFns)
 Fn(allCheckers);
 
-  allCheckers.initializeManager(*checkerMgr, checkerOpts);
+  allCheckers.initializeManager(*checkerMgr, opts, diags);
   allCheckers.validateCheckerOptions(opts, diags);
   checkerMgr->finishedCheckerRegistration();
 
-  for (unsigned i = 0, e = checkerOpts.size(); i != e; ++i) {
-if (checkerOpts[i].isUnclaimed()) {
-  diags.Report(diag::err_unknown_analyzer_checker)
-  << checkerOpts[i].getName();
-  diags.Report(diag::note_suggest_disabling_all_checkers);
-}
-
-  }
-
   return checkerMgr;
 }
 
@@ -155,8 +133,7 @@
const AnalyzerOptions &opts) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  SmallVector checkerOpts = getCheckerOptList(opts);
-  ClangCheckerRegistry(plugins).printList(out, checkerOpts);
+  ClangCheckerRegistry(plugins).printList(out, opts);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: cfe/trunk/lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -12,7 +12,6 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/CheckerOptInfo.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
@@ -30,6 +29,41 @@
 
 using CheckerInfoSet = llvm::SetVector;
 
+namespace {
+/// Represents a request to include or exclude a checker or package from a
+/// specific analysis run.
+///
+/// \sa CheckerRegistry::initializeManager
+class CheckerOptInfo {
+  StringRef Name;
+  bool Enable;
+  bool Claimed;
+
+public:
+  CheckerOptInfo(StringRef name, bool enable)
+: Name(name), Enable(enable), Claimed(false) { }
+
+  StringRef getName() const { return Name; }
+  bool isEnabled() const { return Enable; }
+  bool isDisabled() const { return !isEnabled(); }
+
+  bool isClaimed() const { return Claimed; }
+  bool isUnclaimed() const { return !isClaimed(); }
+  void claim() { Claimed = true; }
+};
+
+} // end of anonymous namespace
+
+static SmallVector
+getCheckerOptList(const AnalyzerOptions &opts) {
+  SmallVector checkerOpts;
+  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
+const std::pair &opt = opts.CheckersControlList[i];
+checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
+  }
+  return checkerOpts;
+}
+
 static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
   const CheckerRegistry::CheckerInfo &b) {
   return a.FullName < b.FullName;
@@ -52,6 +86,7 @@
   return false;
 }
 
+/// Collects the checkers for the supplied \p opt opt

[PATCH] D53069: [analyzer][www] Update avaible_checks.html

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

Szelethus abandoned this revision.
Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

In https://reviews.llvm.org/D53069#1274554, @george.karpenkov wrote:

> If we want to be serious about this page, it really has to be auto-generated 
> (like clang-tidy one), but I understand that this is a larger undertaking.


Since sphinx is on the way, hopefully, let's just look for a long term solution.


https://reviews.llvm.org/D53069



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

[PATCH] D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347153: [analyzer][UninitializedObjectChecker] Uninit 
regions are only reported once (authored by Szelethus, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51531?vs=163542&id=174530#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51531

Files:
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -865,3 +865,44 @@
   ReferenceTest4::RecordType c, d{37, 38};
   ReferenceTest4(d, c);
 }
+
+//===--===//
+// Tests for objects containing multiple references to the same object.
+//===--===//
+
+struct IntMultipleReferenceToSameObjectTest {
+  int *iptr; // expected-note{{uninitialized pointee 'this->iptr'}}
+  int &iref; // no-note, pointee of this->iref was already reported
+
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntMultipleReferenceToSameObjectTest(int *i) : iptr(i), iref(*i) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fIntMultipleReferenceToSameObjectTest() {
+  int a;
+  IntMultipleReferenceToSameObjectTest Test(&a);
+}
+
+struct IntReferenceWrapper1 {
+  int &a; // expected-note{{uninitialized pointee 'this->a'}}
+
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntReferenceWrapper1(int &a) : a(a) {} // expected-warning{{1 uninitialized field}}
+};
+
+struct IntReferenceWrapper2 {
+  int &a; // no-note, pointee of this->a was already reported
+
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntReferenceWrapper2(int &a) : a(a) {} // no-warning
+};
+
+void fMultipleObjectsReferencingTheSameObjectTest() {
+  int a;
+
+  IntReferenceWrapper1 T1(a);
+  IntReferenceWrapper2 T2(a);
+}
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
@@ -153,7 +153,7 @@
 
   if (V.isUndef()) {
 return addFieldToUninits(
-LocalChain.add(LocField(FR, /*IsDereferenced*/ false)));
+LocalChain.add(LocField(FR, /*IsDereferenced*/ false)), FR);
   }
 
   if (!Opts.CheckPointeeInitialization) {
@@ -170,7 +170,7 @@
   }
 
   if (DerefInfo->IsCyclic)
-return addFieldToUninits(LocalChain.add(CyclicLocField(FR)));
+return addFieldToUninits(LocalChain.add(CyclicLocField(FR)), FR);
 
   const TypedValueRegion *R = DerefInfo->R;
   const bool NeedsCastBack = DerefInfo->NeedsCastBack;
@@ -187,8 +187,9 @@
   if (PointeeT->isUnionType()) {
 if (isUnionUninit(R)) {
   if (NeedsCastBack)
-return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
-  return addFieldToUninits(LocalChain.add(LocField(FR)));
+return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)),
+ R);
+  return addFieldToUninits(LocalChain.add(LocField(FR)), R);
 } else {
   IsAnyFieldInitialized = true;
   return false;
@@ -208,8 +209,8 @@
 
   if (isPrimitiveUninit(PointeeV)) {
 if (NeedsCastBack)
-  return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
-return addFieldToUninits(LocalChain.add(LocField(FR)));
+  return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)), R);
+return addFieldToUninits(LocalChain.add(LocField(FR)), R);
   }
 
   IsAnyFieldInitialized = true;
Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
@@ -28,18 +28,25 @@
 using namespace clang;
 using namespace clang::ento;
 
+/// We'll mark fields (and pointee of fields) that are confirmed to be
+/// uninitialized as already analyzed.
+REGISTER_SET_WITH_PROGRAMSTATE(AnalyzedRegions, const MemRegion *)
+
 namespace {
 
-class UninitializedObjectChecker : public Checker {
+class UninitializedObjectChecker
+: public Checker {
   std::unique_ptr BT_uninitField;
 
 public:
   // The fields of this struct will be initialized when registering the checker.
   UninitObjCheckerOptions Opts;
 
   UninitializedObjectChecker()
   : BT_uninitField(new BuiltinBug(this, "Uninitialized fiel

[PATCH] D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

In https://reviews.llvm.org/D51531#1296256, @NoQ wrote:

> In https://reviews.llvm.org/D51531#1286110, @Szelethus wrote:
>
> > Oh, and the reason why I think it would add a lot of complication: since 
> > only `ExprEngine` is avaible in the `checkEndAnalysis` callback, which, 
> > from what I can see, doesn't have a handly `isDead` method, so I'm not even 
> > sure how I'd implement it.
>
>
> Mm, what do you need `isDead` for? Everything is dead at the end of the 
> analysis, you need to clean up everything, otherwise you get use-after-free(?)


Oh, right. Silly me :)


https://reviews.llvm.org/D51531



___
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

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

Szelethus updated this revision to Diff 174495.
Szelethus retitled this revision from "[analyzer] Emit a warning for unknown 
-analyzer-config options" to "[analyzer] Emit an error for invalid 
-analyzer-config inputs".
Szelethus set the repository for this revision to rC Clang.
Szelethus added a comment.
Herald added subscribers: gamesh411, mgrang, baloghadamsoftware.

- Added the discussed compatibility flag that's off by default, but is enabled 
by default by the driver.
- Now emitting errors instead of warnings.
- For the first time, introduce errors rather then asserts for invalid input.


Repository:
  rC Clang

https://reviews.llvm.org/D53280

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Analysis/invalid-analyzer-config-value.c

Index: test/Analysis/invalid-analyzer-config-value.c
===
--- /dev/null
+++ test/Analysis/invalid-analyzer-config-value.c
@@ -0,0 +1,72 @@
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config notes-as-events=yesplease \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-BOOL-INPUT
+
+// CHECK-BOOL-INPUT: (frontend): invalid input for analyzer-config option
+// CHECK-BOOL-INPUT-SAME:'notes-as-events', that expects a boolean value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config notes-as-events=yesplease
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config max-inlinable-size=400km/h \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-UINT-INPUT
+
+// CHECK-UINT-INPUT: (frontend): invalid input for analyzer-config option
+// CHECK-UINT-INPUT-SAME:'max-inlinable-size', that expects an unsigned
+// CHECK-UINT-INPUT-SAME:value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config max-inlinable-size=400km/h
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config ctu-dir=0123012301230123 \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-FILENAME-INPUT
+
+// CHECK-FILENAME-INPUT: (frontend): invalid input for analyzer-config option
+// CHECK-FILENAME-INPUT-SAME:'ctu-dir', that expects a filename
+// CHECK-FILENAME-INPUT-SAME:value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config ctu-dir=0123012301230123
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config no-false-positives=true \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-UNKNOWN-CFG
+
+// CHECK-UNKNOWN-CFG: (frontend): unknown analyzer-config 'no-false-positives'
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config no-false-positives=true
+
+
+// Test the driver properly using "analyzer-config-compatibility-mode=true",
+// no longer causing an error on input error.
+// RUN: %clang --analyze %s \
+// RUN:   -Xclang -analyzer-config -Xclang no-false-positives=true
+
+// RUN: not %clang --analyze %s \
+// RUN:   -Xclang -analyzer-config -Xclang no-false-positives=true \
+// RUN:   -Xclang -analyzer-config-compatibility-mode=false \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NO-COMPAT
+
+// CHECK-NO-COMPAT: error: unknown analyzer-config 'no-false-positives'
+
+// expected-no-diagnostics
+
+int main() {}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -181,8 +181,10 @@
   }
 }
 
+// Parse the Static Analyzer configuration. If \p Diags is set to nullptr,
+// it won't verify the input.
 static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
- DiagnosticsEngine &Diags);
+ DiagnosticsEngine *Diags);
 
 static void getAllNoBuiltinFuncValues(ArgList &Args,
   std::vector &Funcs) {
@@ -284,6 +286,12 @@
   Opts.ShowCheckerHelp = Args.hasArg(OPT_analyzer_checker_help);
   Opts.ShowConfigOptionsList = Args.hasArg(OPT_analyzer_config_help);
   Opts.ShowEnabledCheckerList = Args.hasArg(OPT_analyzer_list_enabled_checkers);
+  Opts.ShouldEmitErrorsOnInvalidConfigValue =
+  /* negated */!llvm::StringSwitch(
+   Args.getLastArgValue(OPT_analyzer_config_compatibility_mode))
+.Case("true", true)
+.Case("false", false)

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

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

Szelethus added a comment.

In https://reviews.llvm.org/D54557#1301869, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1300671, @Szelethus wrote:
>
> > Nice catch, would you like to make an issue on their project or shall I?
>
>
> I guess it can be an outright pull request :) I'll see if i have time for 
> that soon, please feel free to take initiative><


I'll do the honors then. :)

> 
> 
> In https://reviews.llvm.org/D54557#1301829, @Szelethus wrote:
> 
>> Yup, there seems to be a desire to keep it around. Let's add an entry to the 
>> open projects maybe?
> 
> 
> Mmm, what is the project here? What changes are still necessary?

Wasn't the point of this patch to turn off part of this checkers functionality 
because it's immature just yet? From what I understand it is desired, but the 
FP rate is a little too high. I guess fixing that is the project.

Hmm, actually, tinkering with HTML files might be overkill, especially since 
sphinx will hopefully end that era. Let's just add a TODO and let me deal with 
it later when I reorganize the checker options. Sorry for talking nonsense :D


https://reviews.llvm.org/D54557



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

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

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

Szelethus added a comment.

> In https://reviews.llvm.org/D54557#1300653, @NoQ wrote:
> 
>> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>>
>> > I think we should either remove the non-default functionality (which 
>> > wouldn't be ideal), or emphasise somewhere (open projects?) that there is 
>> > still work to be done, but leaving it to be forgotten and essentially 
>> > making it an extra maintenance work would be, in my optinion, the worst 
>> > case scenario. `Aggressive` isn't `Pedantic` because it actually emits 
>> > warnings on correct code, and it's not a simple matter of too many reports 
>> > being emitted, let's also document that this is an experimental feature, 
>> > not a power-user-only thing.
>>
>>
>> I only kept the option around because i was under an impression that i'm 
>> intruding into a checker that already has some happy users, probably 
>> breaking existing workflows. If this option is unnecessary, i'd be happy to 
>> remove it :)
> 
> 
> Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the 
> near future) work on this particular checker.

Yup, there seems to be a desire to keep it around. Let's add an entry to the 
open projects maybe?


Repository:
  rC Clang

https://reviews.llvm.org/D54557



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

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

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

Szelethus added a comment.

In https://reviews.llvm.org/D54557#1300581, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>
> > Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm 
> > curious how this patch behaves on that project. I'll take a look.
>
>
> Whoops, sry, i accidentally had a look because i misread your message as if 
> you wanted me to take a look >.<
>
> (IMPORTANT) **Spoiler alert!**
>
> It finds one positive (and one duplicate of that one positive):
>
> F7553145: rtags-move-positive.html 
>
> I believe this positive is a real bug, but we can still do better here. We 
> find it as a use-after-move of a local variable, which is not a bug on its 
> own, i.e. the user intended to empty and then re-use the storage and it's 
> fine, this is not a usual kind of typo where the user uses the pre-move 
> variable instead of the post-move variable. But the real bug here is that 
> this local variable uses an `std::string` as a field under the hood, which 
> is, well, not guaranteed to be empty after move, like all other STL objects: 
> https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring
>
> NOTE: As also mentioned in this stackoverflow thread, we also need to exclude 
> smart pointers from the STL check because they don't end up in unspecified 
> state, and see if there are other cornercases here.


OMG That is cool. :D That project was fishy. Nice catch, would you like to make 
an issue on their project or shall I?

> Unfortunately, we don't find this bug as an STL use-after-move bug because 
> inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's 
> a combination of running out of budget and a specific feature that we have. 
> By flipping a single `-analyzer-config` flag that represents that feature, we 
> are able to find such bugs as STL bugs (when local variable bugs are disabled 
> in the checker). We're still not able to find the original bug, most likely 
> due to running out of budget (i didn't debug this further), but we can find 
> it in a minimal example:
> 
>   #include "rct/Rct.h"
>   
>   void foo() {
> String S1;
> String S2 = std::move(S1);
> S1 += "asdfg"; // use-after-move of a std::string
>   }
> 
> 
> Here's the report that we are able to obtain for this trivial code snippet, 
> and you can look up the answer to the exercise in the collapsed run-line :)
> 
> F7553290: rtags-move-positive-simplified.html 
> 

Thanks! :) *throws confetti*

In https://reviews.llvm.org/D54557#1300653, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>
> > I think we should either remove the non-default functionality (which 
> > wouldn't be ideal), or emphasise somewhere (open projects?) that there is 
> > still work to be done, but leaving it to be forgotten and essentially 
> > making it an extra maintenance work would be, in my optinion, the worst 
> > case scenario. `Aggressive` isn't `Pedantic` because it actually emits 
> > warnings on correct code, and it's not a simple matter of too many reports 
> > being emitted, let's also document that this is an experimental feature, 
> > not a power-user-only thing.
>
>
> I only kept the option around because i was under an impression that i'm 
> intruding into a checker that already has some happy users, probably breaking 
> existing workflows. If this option is unnecessary, i'd be happy to remove it 
> :)


Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the near 
future) work on this particular checker.


Repository:
  rC Clang

https://reviews.llvm.org/D54557



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347006: [analyzer] ConversionChecker: handle floating point 
(authored by Szelethus, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52730?vs=172922&id=174306#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52730

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
   U8 = S + 10;
 }
 
+char dontwarn6(long long x) {
+  long long y = 42;
+  y += x;
+  return y == 42;
+}
+
 
 // C library functions, handled via apiModeling.StdCLibraryFunctions
 
@@ -154,7 +160,7 @@
 # define EOF (-1)
 char reply_string[8192];
 FILE *cin;
-extern int dostuff (void);
+extern int dostuff(void);
 int libraryFunction2() {
   int c, n;
   int dig;
@@ -179,3 +185,26 @@
   }
 }
 
+double floating_point(long long a, int b) {
+  if (a > 1LL << 55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+  } else if (b > 1 << 25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+  }
+  return 137;
+}
+
+double floating_point2() {
+  int a = 1 << 24;
+  long long b = 1LL << 53;
+  float f = a; // no-warning
+  double d = b; // no-warning
+  return d - f;
+}
+
+int floating_point_3(unsigned long long a) {
+  double b = a; // no-warning
+  return 42;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
 // of expressions. A warning is reported when:
 // * a negative value is implicitly converted to an unsigned value in an
 //   assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-//   value of target
+// * assignment / initialization when the source value is greater than the max
+//   value of the target integer type
+// * assignment / initialization when the source integer is above the range
+//   where the target floating point type can represent all integers
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -28,6 +30,9 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/APFloat.h"
+
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -40,11 +45,9 @@
 private:
   mutable std::unique_ptr BT;
 
-  // Is there loss of precision
   bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
  CheckerContext &C) const;
 
-  // Is there loss of sign
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
   void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
@@ -132,19 +135,51 @@
 
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!DestType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isRealType() || !SubType->isIntegerType())
 return false;
 
-  if (C.getASTContext().getIntWidth(DestType) >=
-  C.getASTContext().getIntWidth(SubType))
+  const bool isFloat = DestType->isFloatingType();
+
+  const auto &AC = C.getASTContext();
+
+  // We will find the largest RepresentsUntilExp value such that the DestType
+  // can exactly represent all nonnegative integers below 2^RepresentsUntilExp.
+  unsigned RepresentsUntilExp;
+
+  if (isFloat) {
+const llvm::fltSemantics &Sema = AC.getFloatTypeSemantics(DestType);
+RepresentsUntilExp = llvm::APFloat::semanticsPrecision(Sema);
+  } else {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+  // This is just casting a number to bool, probably not a bug.
+  return false;
+}
+if (DestType->isSignedIntegerType())
+  RepresentsUntilExp--;
+  }
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long) * CHAR_BIT) {
+// Avoid overflow in our later calculations.
 return false;
+  }
+
+  unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+  if (SubType->isSignedIntegerType())
+CorrectedSrcWidth--;
 
-  unsigned W = C.getASTContext().getIntWidth(DestType);
-  if (W == 1 || W >= 64U)
+  if (RepresentsUntilExp >= CorrectedSrcWidth) {
+// Simple case: the destination can store all values of the source type.
 return false;
+  }
 
-  unsigned long long MaxVal = 1ULL << W;
+  unsigned long long MaxVal = 1ULL << RepresentsUntilExp;
+  if (isFloat) {
+// If this is a floating point type, it can also represent MaxVal exactly.
+MaxVal++;
+  }
   return C.isGreaterOrEqual(Cast->getSubExpr(

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

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

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

In https://reviews.llvm.org/D52730#1289989, @donat.nagy wrote:

> Could someone with commit rights commit this patch (if it is acceptable)? I 
> don't have commit rights myself.


I'll do the honors. Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

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

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

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

In https://reviews.llvm.org/D54438#1297602, @NoQ wrote:

> Hmm, makes sense. Maybe `static bool BlahBlahChecker::isApplicable(const 
> LangOpts &LO)` or something like that.
>
> Btw, maybe instead of default constructor and `->setup(Args)` method we could 
> stuff all of the `setup(Args)`'s arguments into the one-and-only constructor 
> for the checker?


Neat. Make all checkers have only a single constructor, expecting a `const 
CheckerManager &`! Since `getCurrentName` and the like will be thing of the 
past, for once, maybe we could also initialize checker options in constructors 
too. Hopefully.


Repository:
  rC Clang

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] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

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

Szelethus added a comment.

Alpha checkers, at least to developers, are clearly stating "Please finish 
me!", but a non-alpha, enabled-by-default checker with a flag that you'd never 
know about unless you looked at the source code (at least up until I fix these) 
sounds like it'll be forgotten like many other similar flags.


Repository:
  rC Clang

https://reviews.llvm.org/D54557



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

[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.

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

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

How about `std::list::splice`?


Repository:
  rC Clang

https://reviews.llvm.org/D54563



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

[PATCH] D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit.

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

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

Awesome! :D




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:385-386
+  }
+  // Provide the caller with the classification of the object
+  // we've obtained here accidentally, for later use.
+  return OK;

Maybe move this in-class?


Repository:
  rC Clang

https://reviews.llvm.org/D54560



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

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

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

Szelethus added a comment.

I think we should either remove the non-default functionality (which wouldn't 
be ideal), or emphasise somewhere (open projects?) that there is still work to 
be done, but leaving it to be forgotten and essentially making it an extra 
maintenance work would be, in my optinion, the worst case scenario. 
`Aggressive` isn't `Pedantic` because it actually emits warnings on correct 
code, and it's not a simple matter of too many reports being emitted, let's 
also document that this is an experimental feature, not a power-user-only thing.

Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm curious 
how this patch behaves on that project. I'll take a look.




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:92-95
+  bool IsAggressive = false;
+
+public:
+  void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; }

How about a simple public data member? Since all callbacks are const, it 
wouldn't be modified after registration anyways.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:134
+static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) {
+  if (const auto *SR = dyn_cast_or_null(MR)) {
+SymbolRef Sym = SR->getSymbol();

You use `dyn_cast_or_null`, which to me implies that the returned value may be 
`nullptr`, but you never check it on the call sites.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:273-282
+  // In non-aggressive mode, only warn on use-after-move of local variables (or
+  // local rvalue references) and of STL objects. The former is possible 
because
+  // local variables (or local rvalue references) are not tempting their user 
to
+  // re-use the storage. The latter is possible because STL objects are known
+  // to end up in a valid but unspecified state after the move and their
+  // state-reset methods are also known, which allows us to predict
+  // precisely when use-after-move is invalid.

I've seen checker option descriptions in registry functions, in-class where 
they are declared, and on top of the file, I think any of them would be more 
ideal. Since my checker option related efforts are still far in the distance 
(which will put all of them in `Checkers.td`), let's move (or at least copy) 
this somewhere else.



Comment at: test/Analysis/use-after-move.cpp:47-52
+template 
+class vector {
+public:
+  vector();
+  void push_back(const T &t);
+};

Any particular reason for not using `cxx-system-header-simulator.h`?


Repository:
  rC Clang

https://reviews.llvm.org/D54557



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

[PATCH] D54556: [analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.

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

Szelethus accepted this revision.
Szelethus added a comment.

LGTM, both the concept and the patch! I have read your followup patches up to 
part 4, I'll leave some thoughts there.


Repository:
  rC Clang

https://reviews.llvm.org/D54556



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

[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

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

Szelethus marked 5 inline comments as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:982
+  int ParenthesesDepth = 1;
+  while (ParenthesesDepth != 0) {
 ++It;

xazax.hun wrote:
> Is the misspelling already committed? If not, better fix it in the other 
> revision to keep this smaller. Otherwise, it is fine.
Unfortunately the previous parts of this project are already commited :/


https://reviews.llvm.org/D52795



___
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

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

@george.karpenkov Matching macros is a very non-trivial job, how would you feel 
if we shipped this patch as-is, and maybe leave a TODO about adding macro 
`assert`s down the line?


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] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

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

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

Unfortunately, I found //yet another// corner case I didn't cover: if the macro 
arguments themselves are macros. I already fixed it, but it raises the question 
that what other things I may have missed? I genuinely dislike this project, and 
I fear that I can't test this enough to be be 100% it does it's job perfectly. 
Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is 
unnerving to put it lightly, but doing changes within `Preprocessor` is //most 
definitely// out of my reach.

Maybe it'd be worth to caution the users of this feature that it's 
experimental, but then again, it will always be, as long as there's no 
out-of-the-box solution from `Preprocessor`.


https://reviews.llvm.org/D52795



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

[PATCH] D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

Unfortunately, I found //yet another// corner case I didn't cover: if the macro 
arguments themselves are macros. I already fixed it, but it raises the question 
that what other things I may have missed? I genuinely dislike this project, and 
I fear that I can't test this enough to be be 100% it does it's job perfectly. 
Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is 
unnerving to put it lightly, but doing changes within `Preprocessor` is //most 
definitely// out of my reach.

Maybe it'd be worth to caution the users of this feature that it's 
experimental, but then again, it will always be, as long as there's no 
out-of-the-box solution from `Preprocessor`.


Repository:
  rC Clang

https://reviews.llvm.org/D54437



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

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

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

Szelethus added a comment.

Thanks you so much! One of the reasons why I love working on this is because 
the great feedback I get. I don't wanna seem annoyingly grateful, but it's been 
a very enjoyable experience so far.

> This should not cause a warnings because it should be fine to compile 
> different translation units with different flags within the same project, 
> while the decision to enable the checker explicitly is done once per project.

Okay, I don't insist on it :)

> Now, normally these decisions are handled by the Clang Driver - the gcc 
> compatibility wrapper for clang that converts usual gcc-compatible run-lines 
> into `-cc1` run-lines. Eg., the `unix` package is only enabled on UNIX 
> targets and the `osx` package is only enabled on macOS/iOS targets and the 
> `core` package is enabled on all targets and `security.insecureAPI` is 
> disabled on PS4 //because that's what the Driver automatically appends to the 
> -cc1 run-line//. Even though `scan-build` does not call the analyzer through 
> the Driver, it invokes a `-###` run-line to obtain flags first, so it still 
> indirectly consults the Driver.
> 
> But the problem with the Driver is that nobody knows about this layer of 
> decision-making (unlike the obvious `Checkers.td`) and therefore it's kinda 
> messy to have all checkers hardcoded separately within the Driver. It kinda 
> makes more sense to split the checkers into packages and let the Driver 
> choose which packages are enabled.
>  So, generally, i suggest not to jump onto this as long as the analyzer as a 
> whole works more or less correctly. Restricting functionality for the purpose 
> of avoiding mistakes should be done with extreme care because, well, it 
> restricts functionality. Before restricting funcitonality, we need to be more 
> or less certain that nobody will ever need such functionality or that we will 
> be able to provide a safe replacement when necessary without also introducing 
> too much complexity (aka bugs). Which is why i recommend not to bother too 
> much about it. There are much more terrible bugs all over the place, no need 
> to struggle that hard to prevent these small bugs.
> 
> Another approach to the original problem would be to replace the language 
> options check in registerBlahBlahChecker() with a set of language options 
> checks in every checker callback. It's annoying to write and clutters the 
> checker but it preserves all the functionality without messing with the 
> registration process. So if you simply want to move these checks out of the 
> way of your work, this is probably the way to go.

Do you mean something along the lines of supplying a boolean 
`shouldRegister##CHECKERNAME` function to checkers? That would essentially 
achieve the same thing, but still allows me to move 
`CheckerManager::registerChecker` ultimately out of checker registry function.


Repository:
  rC Clang

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] D54466: [Analyzer] [WIP] Iterator Checkers - Use the base region of C++ Base Object Regions (recursively) for iterators stored in a region

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

Szelethus added a comment.

Hmmm, shouldn't we add this to `MemRegion`'s interface instead?


Repository:
  rC Clang

https://reviews.llvm.org/D54466



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

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

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

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

Here's how I'm planning to approach this issue:

- Split `MallocChecker` and `CStringChecker` up properly, introduce 
`MallocCheckerBase` and `CStringCheckerBase`. I'm a little unsure about how 
I'll do this, but I'll share my thoughts when I come up with something clever.
- Some checkers do not register themselves in all cases[1], which should 
probably be handled at a higher level, preferably in `Checkers.td`. Warnings 
could be emitted when a checker is explicitly enabled but will not be 
registered.
- Once all checkers in all cases properly register themselves, refactor this 
patch to introduce dependencies in between the subcheckers of `MallocChecker` 
and `CStringChecker`. This will imply that every registry function registers 
one, and only one checker.
- Move `CheckerManager::registerChecker` out of the registry functions, 
rename them from `register##CHECKERNAME` to `setup##CHECKERNAME`, and supply 
the //mutable// checker object and the //immutable// `CheckerManager`. The 
downside is that all checkers will have to be default constructible, and their 
fields will have to be set up in `setup##CHECKERNAME`, but I see this as an 
acceptable tradeoff for the extra security.
  - This will make it impossible to affect other checkers. I'm a little unsure 
whether this is achievable considering how intertwined everything is in 
`MallocChecker`, but even if the supplied `CheckerManager` will have to 
non-const, `getCurrentCheckerName` and `setCurrentCheckerName` could be 
eliminated for sure.
- Properly document why `CheckerRegistry` and `CheckerManager` are separate 
entities, how are the tblgen files processed, how are dependencies handled, and 
so on.
- Rebase my local checker option-related branches, and celebrate. Unless 
another skeleton falls out of the closet. You never know I guess.

[1]

  void ento::registerObjCDeallocChecker(CheckerManager &Mgr) {
const LangOptions &LangOpts = Mgr.getLangOpts();
// These checker only makes sense under MRR.
if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
  return;
  
Mgr.registerChecker();
  }


Repository:
  rC Clang

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] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

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

Allow me to disagree again -- the reason why `CheckerRegistry` and the entire 
checker registration process is a mess is that suboptimal solutions were added 
instead of making the system do well on the long term. This is completely fine, 
I am guilty of this myself, but now that this problem has been highlighted, 
it's time to get things right. Some clever refactoring is long overdue.

I don't mind delaying my checker option related works, if that means that 
checker registration doesn't need hacking each time a flaw like this is 
discovered.

There are other things I need to figure out -- some checkers refuse to register 
themselves in their registry funcions,

- which makes moving `CheckerManager::registerChecker` out of registry 
funcions impossible,
- which makes passing the checkers full name directly to `CheckerManager` 
impossible,
- which makes replacing the current registration infrastructure impossible as 
`CheckerManager` has to be mutable when supplied to the registry function,
- which will always leaves toom for errors like this to arise.

Since I spent the last couple weeks lookig at these files, there's no better 
opportunity for me to fix these.


Repository:
  rC Clang

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] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

Szelethus added a comment.

Actually, no. The main problem here is that `InnerPointerChecker` doesn't only 
want to register `MallocBase`, it needs to modify the checker object. In any 
case, either `MallocChecker.cpp` needs the definition of `InnerPointerChecker`, 
or vice versa. Sure, I could separate out the part that adds a new way to 
register dependencies, but it doesn't stop anyone from making the same mistake 
again, so what would be the point of that? I think it would be just a lot 
cleaner to split those checkers up properly, and completely replace the current 
system of registering checkers to stop anyone accidentally doing the same 
mistake again.


Repository:
  rC Clang

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] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

Szelethus added a comment.

In https://reviews.llvm.org/D54438#1296239, @NoQ wrote:

> Mm, i don't understand. I mean, what prevents you from cutting it off even 
> earlier and completely omitting that part of the patch? Somebody will get to 
> this later in order to see how exactly does the separation needs to be 
> performed.


I somewhat misunderstood what you meant in your earlier comment.

Originally, I created `CStringBase`, but not `MallocBase`, hence the insane 
amount of workaround, witch eventually lead to the createion of `MallocBase`, 
but I never removed the now unnecessary stuff, but now that you've shed some 
light to it, it makes little sense for it to stay around. I guess you could say 
that I couldn't see the forest for the trees.

Thanks! There's still a lot of work to be done, as this patch supplies a new 
way of expressing dependencies, but doesn't actually stop anyone from doing 
this mistake again, but let that be an issue for another time.


Repository:
  rC Clang

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] D53982: Output "rule" information in SARIF

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

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242
+#define GET_CHECKERS
+#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN)
\
+  .Case(FULLNAME, HELPTEXT)
+#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+#undef CHECKER
+#undef GET_CHECKERS

aaron.ballman wrote:
> Szelethus wrote:
> > aaron.ballman wrote:
> > > Szelethus wrote:
> > > > Szelethus wrote:
> > > > > Hmmm, this won't handle checkers loaded from plugins.
> > > > I don't immediately know the solution is to this, but when you invoke 
> > > > clang with `-cc1 -analyzer-checker-help`, plugin checkers are also 
> > > > displayed, and [[ 
> > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp#L150
> > > >  | this is line that magically does it ]].
> > > > Maybe store the plugins in `AnalyzerOptions`, and move 
> > > > `ClangCheckerRegistry` to `include/clang/StaticAnalyzer/Frontend`?
> > > Oof, this is a good point. Thank you for bringing it up! I'll look into 
> > > this as soon as I have the chance.
> > > 
> > > Do we have any tests that use plugins (preferably on Windows) so that I 
> > > have a way to test this functionality out?
> > I'm working on the exact same issue, which is why I spotted this! There are 
> > some in `unittests/`, but I plan on enhancing them //a lot// in the coming 
> > days. I'll make sure to add you as subscriber on related paches, until 
> > then, unless users of the Saris output are likely to use the StaticAnalyzer 
> > with plugins, I think feel free to wait on my progress. I expect to come 
> > out with a patch during the week. But you never know, I guess.
> > 
> > https://github.com/llvm-mirror/clang/blob/master/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
> I'm fine with waiting. I don't expect to see heavy use of the Sarif exporter 
> until the spec is finalized, as it's hard to write a Sarif viewer against a 
> moving target. So we've got a few months to find these sort of things out.
> 
> Thank you for CCing me on the related patches!
Well, yea, skeletons are falling out of the closet left and right, so I'll need 
to clean a lot of other things up before finding a neat solution to this. 
Please let me know when you need to make progress on this.


https://reviews.llvm.org/D53982



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

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

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

Szelethus added a comment.

In https://reviews.llvm.org/D54438#1296155, @NoQ wrote:

> Can we reduce this patch to simply introducing the dependency item in 
> `Checkers.td` and using it in, like, one place (eg., 
> `MallocChecker`/`CStringChecker` inter-op) and discuss the rest of the stuff 
> later before we jump into it?


Short answer, no. The original intent was to somehow make sense out of checker 
options, and look where that got me :D There are no more shortcuts to make, 
sadly. I strongly believe it'd be better to figure out how those checkers 
should operate before moving forward with this patch, because I know I wouldn't 
like to touch any of those checkers once this patch goes through, so let's 
tough it out first.

Also, doing some checker related work might be refreshing after the never 
ending refactoring.

I'll brush the dust off some notebooks and see what I can come up with.


Repository:
  rC Clang

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] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

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

Szelethus updated this revision to Diff 173745.
Szelethus edited the summary of this revision.
Szelethus added a comment.

Restored the discussed note.


https://reviews.llvm.org/D54401

Files:
  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Core/CheckerRegistry.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  test/Analysis/disable-all-checks.c

Index: test/Analysis/disable-all-checks.c
===
--- test/Analysis/disable-all-checks.c
+++ test/Analysis/disable-all-checks.c
@@ -1,9 +1,18 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-all-checks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-all-checks -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core \
+// RUN:   -analyzer-store=region -verify %s
+//
 // RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -verify %s
-// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-checker -verify %s 2>&1 | FileCheck %s
+//
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-checker non.existant.Checker -verify %s 2>&1 \
+// RUN:   | FileCheck %s
+//
 // expected-no-diagnostics
 
+// CHECK: no analyzer checkers are associated with 'non.existant.Checker'
 // CHECK: use -analyzer-disable-all-checks to disable all static analyzer checkers
 int buggy() {
   int x = 0;
Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -130,10 +130,11 @@
 
 void ento::printEnabledCheckerList(raw_ostream &out,
ArrayRef plugins,
-   const AnalyzerOptions &opts) {
+   const AnalyzerOptions &opts,
+   DiagnosticsEngine &diags) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  ClangCheckerRegistry(plugins).printList(out, opts);
+  ClangCheckerRegistry(plugins).printList(out, opts, diags);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -9,7 +9,6 @@
 
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/Basic/Diagnostic.h"
-#include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
@@ -19,50 +18,11 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
-#include 
-#include 
 
 using namespace clang;
 using namespace ento;
 
-static const char PackageSeparator = '.';
-
-using CheckerInfoSet = llvm::SetVector;
-
-namespace {
-/// Represents a request to include or exclude a checker or package from a
-/// specific analysis run.
-///
-/// \sa CheckerRegistry::initializeManager
-class CheckerOptInfo {
-  StringRef Name;
-  bool Enable;
-  bool Claimed;
-
-public:
-  CheckerOptInfo(StringRef name, bool enable)
-: Name(name), Enable(enable), Claimed(false) { }
-
-  StringRef getName() const { return Name; }
-  bool isEnabled() const { return Enable; }
-  bool isDisabled() const { return !isEnabled(); }
-
-  bool isClaimed() const { return Claimed; }
-  bool isUnclaimed() const { return !isClaimed(); }
-  void claim() { Claimed = true; }
-};
-
-} // end of anonymous namespace
-
-static SmallVector
-getCheckerOptList(const AnalyzerOptions &opts) {
-  SmallVector checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-const std::pair &opt = opts.CheckersControlList[i];
-checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
-  }
-  return checkerOpts;
-}
+static constexpr char PackageSeparator = '.';
 
 static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
   const CheckerRegistry::CheckerInfo &b) {
@@ -86,40 +46,49 @@
   return false;
 }
 
-/// Collects the checkers for the supplied \p opt option into \p collected.
-static void collectCheckers(const CheckerRegistry::CheckerInfoList &checkers,
-const llvm::StringMap &packageSizes,
-CheckerOptInfo &opt, CheckerInfoSet &collected) {
-  // Use a binary search to find the possible start of the package.
-

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

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

Szelethus added inline comments.



Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:177
 "no analyzer checkers are associated with '%0'">;
-def note_suggest_disabling_all_checkers : Note<
-"use -analyzer-disable-all-checks to disable all static analyzer 
checkers">;

NoQ wrote:
> george.karpenkov wrote:
> > @NoQ what does this option even do? Can you think of some legitimate uses?
> rC216763 is the commit in which it was added and the commit message sounds 
> useful. I never understood how exactly are people using it, but it sounds as 
> if they are trying to skip analysis of certain files by appending a flag that 
> disables checkers to the build(?!) command of those files.
Okay, let's move this to another revision then.


Repository:
  rC Clang

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] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

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

This patch solves the problem I explained in an earlier revision[1]. It's huge, 
I'm still looking for ways to split it up, hence the WIP tag. Also, it fixes a 
major bug, but has no tests.

The solution is implemented as follows:

- Add a new field to the `Checker` class in `CheckerBase.td` called 
`Dependencies`, which is a list of `Checker`s. I know some of us have mixed 
feelings on the use of tblgen, but I personally found it very helpful, because 
few things are more miserable then working with the preprocessor, and this 
extra complication is actually another point where things can be verified, 
properly organized, and so on. The best way for me to prove that it shouldn't 
be dropped is actually use it and document it properly, so I did just that in 
this patch.
- Move `unix` checkers before `cplusplus`, as there is no forward declaration 
in tblgen :/
- Introduce two new checkers: `CStringBase` and `MallocBase` are essentially 
checkers without any of their modules/subcheckers enabled. These are important, 
as for example `InnerPointerChecker` would like to see `MallocChecker` enabled, 
but does not add `CK_MallocChecker` to `MallocChecker::ChecksEnabled` on it's 
own. This was weird, so from now on, "barebone" versions of `CStringChecker` 
and `MallocChecker` may be enabled via the new checkers. Note that this doesn't 
change anything on the user facing interface.
- Move `InnerPointerChecker` to it's own directory, separate the checker class 
into a header file, move its registry function to `MallocChecker`.
- Clear up and registry functions in `MallocChecker`, happily remove old FIXMEs.
- Remove `lib/StaticAnalyzer/Checkers/InterCheckerAPI.h`.
- Add a new `addDependency` function to `CheckerRegistry`.
- Neatly format `RUN` lines in files I looked at while debugging.

[1]**(Copied from https://reviews.llvm.org/D54397's summary!)** While on the 
quest of fixing checker options the same way I cleaned up non-checker options, 
although I'm making good progress, I ran into a wall: In order to emit warnings 
on incorrect checker options, we need to ensure that checkers actually acquire 
their options properly -- but, I unearthed a huge bug in checker registration: 
dependencies are currently implemented in a way that breaks the already very 
fragile registration infrastructure.

Here is where the problem really originates from: this is a snippet from 
`CheckerRegistry::initializeManager`.

  // Initialize the CheckerManager with all enabled checkers.
  for (const auto *i :enabledCheckers) {
checkerMgr.setCurrentCheckName(CheckName(i->FullName));
i->Initialize(checkerMgr);
  }

Note that `Initialize` is a function pointer to `register##CHECKERNAME`, like 
`registerInnerPointerChecker`. Since for each register function call the 
current checker name is only set once, when `InnerPointerChecker` attempts to 
also register it's dependent `MallocChecker`, both it and `MallocChecker` will 
receive the `cplusplus.InnerPointer` name. This results in `MallocChecker` 
trying to acquire it's `Optimistic` option as 
`cplusplus.InnerPointerChecker:Optimistic`.

Clearly, this problem has to be solved at a higher level -- it makes no sense 
to be able to affect other checkers in a registry function. Since I'll already 
make changes to how checker registration works, I'll probably tune some things 
for the current C++ standard, add much needed documentation, and try to make 
the code //a lot less confusing//.


Repository:
  rC Clang

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/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointer/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointer/InnerPointerChecker.h
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/NewDelete+MismatchedDeallocator_intersections.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/inner-pointer.cpp
  utils/TableGen/ClangSACheckersEmitter.cpp

Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -57,14 +57,26 @@
   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(getS

[PATCH] D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry

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

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

Now that `CheckerRegistry` lies in `Frontend`, we can finally eliminate 
`ClangCheckerRegistry`. Fortunately, this also provides us with a 
`DiagnosticsEngine`, so I went ahead and removed some parameters from it's 
methods.


Repository:
  rC Clang

https://reviews.llvm.org/D54437

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
@@ -10,17 +10,73 @@
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/DynamicLibrary.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 
 using namespace clang;
 using namespace ento;
+using llvm::sys::DynamicLibrary;
+
+using RegisterCheckersFn = void (*)(CheckerRegistry &);
+
+static bool isCompatibleAPIVersion(const char *versionString) {
+  // If the version string is null, it's not an analyzer plugin.
+  if (!versionString)
+return false;
+
+  // For now, none of the static analyzer API is considered stable.
+  // Versions must match exactly.
+  return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
+}
+
+CheckerRegistry::CheckerRegistry(ArrayRef plugins,
+ DiagnosticsEngine &diags) : Diags(diags) {
+#define GET_CHECKERS
+#define CHECKER(FULLNAME, CLASS, HELPTEXT) \
+  addChecker(register##CLASS, FULLNAME, HELPTEXT);
+#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+#undef CHECKER
+#undef GET_CHECKERS
+
+  for (ArrayRef::iterator i = plugins.begin(), e = plugins.end();
+   i != e; ++i) {
+// Get access to the plugin.
+std::string err;
+DynamicLibrary lib = DynamicLibrary::getPermanentLibrary(i->c_str(), &err);
+if (!lib.isValid()) {
+  diags.Report(diag::err_fe_unable_to_load_plugin) << *i << err;
+  continue;
+}
+
+// See if it's compatible with this build of clang.
+const char *pluginAPIVersion =
+  (const char *) lib.getAddressOfSymbol("clang_analyzerAPIVersionString");
+if (!isCompatibleAPIVersion(pluginAPIVersion)) {
+  Diags.Report(diag::warn_incompatible_analyzer_plugin_api)
+  << llvm::sys::path::filename(*i);
+  Diags.Report(diag::note_incompatible_analyzer_plugin_api)
+  << CLANG_ANALYZER_API_VERSION_STRING
+  << pluginAPIVersion;
+  continue;
+}
+
+// Register its checkers.
+RegisterCheckersFn registerPluginCheckers =
+  (RegisterCheckersFn) (intptr_t) lib.getAddressOfSymbol(
+  "clang_registerCheckers");
+if (registerPluginCheckers)
+  registerPluginCheckers(*this);
+  }
+}
 
 static constexpr char PackageSeparator = '.';
 
@@ -47,8 +103,7 @@
 }
 
 CheckerRegistry::CheckerInfoSet CheckerRegistry::getEnabledCheckers(
-  const AnalyzerOptions &Opts,
-  DiagnosticsEngine &diags) const {
+const AnalyzerOptions &Opts) const {
 
   assert(std::is_sorted(Checkers.begin(), Checkers.end(), checkerNameLT) &&
  "In order to efficiently gather checkers, this function expects them "
@@ -65,7 +120,7 @@
 
 if (firstRelatedChecker == end ||
 !isInPackage(*firstRelatedChecker, opt.first)) {
-  diags.Report(diag::err_unknown_analyzer_checker) << opt.first;
+  Diags.Report(diag::err_unknown_analyzer_checker) << opt.first;
   return {};
 }
 
@@ -104,23 +159,22 @@
 }
 
 void CheckerRegistry::initializeManager(CheckerManager &checkerMgr,
-const AnalyzerOptions &Opts,
-DiagnosticsEngine &diags) const {
+const AnalyzerOptions &Opts) const {
   // Sort checkers for efficient collection.
   llvm::sort(Checkers, checkerNameLT);
 
   // Collect checkers enabled by the options.
-  CheckerInfoSet enabledCheckers = getEnabledCheckers(Opts, diags);
+

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

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

Szelethus added a comment.

In https://reviews.llvm.org/D54401#1295832, @george.karpenkov wrote:

> > Also, remove diags::note_suggest_disabling_all_checkers.
>
> Isn't that a separate revision? Given that removing existing options is often 
> questionable, I'd much rather see this patch separated.


This isn't an option, just a note :)


Repository:
  rC Clang

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] D54429: [analyzer] Creating standard Sphinx documentation

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

Szelethus added a comment.

In https://reviews.llvm.org/D54429#1295838, @george.karpenkov wrote:

> What do you propose we should do with other pages on the existing website? 
> (OpenProjects / etc.)


I think we should just move everything here, and forget about the old site, as 
there clearly isn't a desire to maintain it any longer.


Repository:
  rC Clang

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] D54436: [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend

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

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

`ClangCheckerRegistry` is a very non-obvious, poorly documented, weird concept. 
It derives from `CheckerRegistry`, and is placed in 
`lib/StaticAnalyzer/Frontend`, whereas it's base is located in 
`lib/StaticAnalyzer/Core`. It was, from what I can imagine, used to circumvent 
the problem that the registry functions of the checkers are located in the 
`clangStaticAnalyzerCheckers` library, but that library depends on 
`clangStaticAnalyzerCore`. However, `clangStaticAnalyzerFrontend` depends on 
both of those libraries.

One can make the observation however, that `CheckerRegistry` has no place in 
`Core`, it isn't used there at all! The only place where it is used is 
`Frontend`, which where it ultimately belongs.

This move implies that since 
`include/clang/StaticAnalyzer/Checkers/ClangCheckers.h` only a single function:

  class CheckerRegistry;
  
  void registerBuiltinCheckers(CheckerRegistry ®istry);

it had to re purposed, as `CheckerRegistry` is no longer avaible to 
`clangStaticAnalyzerCheckers`. It was renamed to 
`BuiltinCheckerRegistration.h`, which actually describes it -- it **does not** 
contain the registration functions for checkers, but only those generated by 
the tblgen files.


Repository:
  rC Clang

https://reviews.llvm.org/D54436

Files:
  include/clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h
  include/clang/StaticAnalyzer/Checkers/ClangCheckers.h
  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  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/CMakeLists.txt
  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/CheckerDocumentation.cpp
  lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  lib/StaticAnalyzer/Checkers/ClangCheckers.cpp
  lib/StaticAnalyzer/Checkers/ClangSACheckers.h
  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/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/

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

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

Szelethus updated this revision to Diff 173717.
Szelethus set the repository for this revision to rC Clang.
Szelethus added a comment.

Might as well make it a method.


Repository:
  rC Clang

https://reviews.llvm.org/D54401

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Core/CheckerRegistry.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  test/Analysis/disable-all-checks.c

Index: test/Analysis/disable-all-checks.c
===
--- test/Analysis/disable-all-checks.c
+++ test/Analysis/disable-all-checks.c
@@ -1,10 +1,18 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-all-checks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-all-checks -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core \
+// RUN:   -analyzer-store=region -verify %s
+//
 // RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -verify %s
-// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-checker -verify %s 2>&1 | FileCheck %s
+//
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-checker non.existant.Checker -verify %s 2>&1 \
+// RUN:   | FileCheck %s
+//
 // expected-no-diagnostics
 
-// CHECK: use -analyzer-disable-all-checks to disable all static analyzer checkers
+// CHECK: no analyzer checkers are associated with 'non.existant.Checker'
 int buggy() {
   int x = 0;
   return 5/x; // no warning
Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -130,10 +130,11 @@
 
 void ento::printEnabledCheckerList(raw_ostream &out,
ArrayRef plugins,
-   const AnalyzerOptions &opts) {
+   const AnalyzerOptions &opts,
+   DiagnosticsEngine &diags) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  ClangCheckerRegistry(plugins).printList(out, opts);
+  ClangCheckerRegistry(plugins).printList(out, opts, diags);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -9,7 +9,6 @@
 
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/Basic/Diagnostic.h"
-#include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
@@ -19,50 +18,11 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
-#include 
-#include 
 
 using namespace clang;
 using namespace ento;
 
-static const char PackageSeparator = '.';
-
-using CheckerInfoSet = llvm::SetVector;
-
-namespace {
-/// Represents a request to include or exclude a checker or package from a
-/// specific analysis run.
-///
-/// \sa CheckerRegistry::initializeManager
-class CheckerOptInfo {
-  StringRef Name;
-  bool Enable;
-  bool Claimed;
-
-public:
-  CheckerOptInfo(StringRef name, bool enable)
-: Name(name), Enable(enable), Claimed(false) { }
-
-  StringRef getName() const { return Name; }
-  bool isEnabled() const { return Enable; }
-  bool isDisabled() const { return !isEnabled(); }
-
-  bool isClaimed() const { return Claimed; }
-  bool isUnclaimed() const { return !isClaimed(); }
-  void claim() { Claimed = true; }
-};
-
-} // end of anonymous namespace
-
-static SmallVector
-getCheckerOptList(const AnalyzerOptions &opts) {
-  SmallVector checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-const std::pair &opt = opts.CheckersControlList[i];
-checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
-  }
-  return checkerOpts;
-}
+static constexpr char PackageSeparator = '.';
 
 static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
   const CheckerRegistry::CheckerInfo &b) {
@@ -86,40 +46,48 @@
   return false;
 }
 
-/// Collects the checkers for the supplied \p opt option into \p collected.
-static void collectCheckers(const CheckerRegistry::CheckerInfoList &checkers,
-const llvm::StringMap &packageSizes,
-C

[PATCH] D53995: [analyzer] Drastically simplify the tblgen files used for checkers

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346680: [analyzer] Drastically simplify the tblgen files 
used for checkers (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53995?vs=172534&id=173704#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53995

Files:
  cfe/trunk/include/clang/Driver/CC1Options.td
  cfe/trunk/include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
  cfe/trunk/lib/StaticAnalyzer/Checkers/ClangCheckers.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ClangSACheckers.h
  cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  cfe/trunk/utils/TableGen/ClangSACheckersEmitter.cpp

Index: cfe/trunk/utils/TableGen/ClangSACheckersEmitter.cpp
===
--- cfe/trunk/utils/TableGen/ClangSACheckersEmitter.cpp
+++ cfe/trunk/utils/TableGen/ClangSACheckersEmitter.cpp
@@ -11,34 +11,19 @@
 //
 //===--===//
 
-#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 #include 
 #include 
+
 using namespace llvm;
 
 //===--===//
 // Static Analyzer Checkers Tables generation
 //===--===//
 
-/// True if it is specified hidden or a parent package is specified
-/// as hidden, otherwise false.
-static bool isHidden(const Record &R) {
-  if (R.getValueAsBit("Hidden"))
-return true;
-  // Not declared as hidden, check the parent package if it is hidden.
-  if (DefInit *DI = dyn_cast(R.getValueInit("ParentPackage")))
-return isHidden(*DI->getDef());
-
-  return false;
-}
-
-static bool isCheckerNamed(const Record *R) {
-  return !R->getValueAsString("CheckerName").empty();
-}
-
 static std::string getPackageFullName(const Record *R);
 
 static std::string getParentPackageFullName(const Record *R) {
@@ -50,17 +35,19 @@
 
 static std::string getPackageFullName(const Record *R) {
   std::string name = getParentPackageFullName(R);
-  if (!name.empty()) name += ".";
+  if (!name.empty())
+name += ".";
+  assert(!R->getValueAsString("PackageName").empty());
   name += R->getValueAsString("PackageName");
   return name;
 }
 
 static std::string getCheckerFullName(const Record *R) {
   std::string name = getParentPackageFullName(R);
-  if (isCheckerNamed(R)) {
-if (!name.empty()) name += ".";
-name += R->getValueAsString("CheckerName");
-  }
+  if (!name.empty())
+name += ".";
+  assert(!R->getValueAsString("CheckerName").empty());
+  name += R->getValueAsString("CheckerName");
   return name;
 }
 
@@ -70,129 +57,12 @@
   return std::string();
 }
 
-namespace {
-struct GroupInfo {
-  llvm::DenseSet Checkers;
-  llvm::DenseSet SubGroups;
-  bool Hidden;
-  unsigned Index;
-
-  GroupInfo() : Hidden(false) { }
-};
-}
-
-static void addPackageToCheckerGroup(const Record *package, const Record *group,
-  llvm::DenseMap &recordGroupMap) {
-  llvm::DenseSet &checkers = recordGroupMap[package]->Checkers;
-  for (llvm::DenseSet::iterator
- I = checkers.begin(), E = checkers.end(); I != E; ++I)
-recordGroupMap[group]->Checkers.insert(*I);
-
-  llvm::DenseSet &subGroups = recordGroupMap[package]->SubGroups;
-  for (llvm::DenseSet::iterator
- I = subGroups.begin(), E = subGroups.end(); I != E; ++I)
-addPackageToCheckerGroup(*I, group, recordGroupMap);
-}
-
 namespace clang {
 void EmitClangSACheckers(RecordKeeper &Records, raw_ostream &OS) {
   std::vector checkers = Records.getAllDerivedDefinitions("Checker");
-  llvm::DenseMap checkerRecIndexMap;
-  for (unsigned i = 0, e = checkers.size(); i != e; ++i)
-checkerRecIndexMap[checkers[i]] = i;
-
-  // Invert the mapping of checkers to package/group into a one to many
-  // mapping of packages/groups to checkers.
-  std::map groupInfoByName;
-  llvm::DenseMap recordGroupMap;
-
   std::vector packages = Records.getAllDerivedDefinitions("Package");
-  for (unsigned i = 0, e = packages.size(); i != e; ++i) {
-Record *R = packages[i];
-std::string fullName = getPackageFullName(R);
-if (!fullName.empty()) {
-  GroupInfo &info = groupInfoByName[fullName];
-  info.Hidden = isHidden(*R);
-  recordGroupMap[R] = &info;
-}
-  }
-
-  std::vector
-  checkerGroups = Records.getAllDerivedDefinitions("CheckerGroup");
-  for (unsigned i = 0, e = checkerGroups.size(); i != e; ++i) {
-Record *R = checkerGroups[i];
-std::string name = R->getValueAsString("GroupName");
-if (!name.empty()) {
-  GroupInfo &info = groupInfoByName[name];
-  recordGroupMap[R] = &info

[PATCH] D54429: Creating standard shpinx documentation for Clang Static Analyzer

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

Szelethus added a subscriber: xazax.hun.
Szelethus added a comment.

I'll read through this as soon as possible, but a HUGE thank you for this. This 
is what the Static Analyzer desperately needed for years. This will trivialize 
documenting, so conversations about the inner workings of the analyzer will not 
be buried in many year old closed revisions.

@xazax.hun maybe your patch should be implemented with Sphinx too?

Please reupload with full context (`-U9`).


Repository:
  rC Clang

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] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

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

Szelethus added a comment.

Thanks for taking a look! ^-^




Comment at: lib/StaticAnalyzer/Core/CheckerRegistry.cpp:51
 
-/// Collects the checkers for the supplied \p opt option into \p collected.
-static void collectCheckers(const CheckerRegistry::CheckerInfoList &checkers,
-const llvm::StringMap &packageSizes,
-CheckerOptInfo &opt, CheckerInfoSet &collected) {
-  // Use a binary search to find the possible start of the package.
-  CheckerRegistry::CheckerInfo packageInfo(nullptr, opt.getName(), "");
-  auto end = checkers.cend();
-  auto i = std::lower_bound(checkers.cbegin(), end, packageInfo, 
checkerNameLT);
-
-  // If we didn't even find a possible package, give up.
-  if (i == end)
-return;
-
-  // If what we found doesn't actually start the package, give up.
-  if (!isInPackage(*i, opt.getName()))
-return;
-
-  // There is at least one checker in the package; claim the option.
-  opt.claim();
-
-  // 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 =
-packageSizes.find(opt.getName());
-  if (packageSize != packageSizes.end())
-size = packageSize->getValue();
-
-  // Step through all the checkers in the package.
-  for (auto checkEnd = i+size; i != checkEnd; ++i)
-if (opt.isEnabled())
-  collected.insert(&*i);
-else
-  collected.remove(&*i);
+static CheckerInfoSet getEnabledCheckers(
+const CheckerRegistry::CheckerInfoList &checkers,

MTC wrote:
> I just curious about the reason why you move the `CheckerInfoSet` from the 
> parameter passing by reference to the return value, keep the number of 
> parameter at a lower value? Or make the code at the caller cite cleaner?
Both, actually. From what I've seen, out-params usually lead to much harder to 
comprehend code.


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] D52984: [analyzer] Checker reviewer's checklist

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

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

LGTM! Let's continue the conversation about coding standards somewhere else.

Can you please mark inlines as done?


https://reviews.llvm.org/D52984



___
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

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

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

Also, remove `diags::note_suggest_disabling_all_checkers`.


https://reviews.llvm.org/D54401

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Core/CheckerRegistry.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  test/Analysis/disable-all-checks.c

Index: test/Analysis/disable-all-checks.c
===
--- test/Analysis/disable-all-checks.c
+++ test/Analysis/disable-all-checks.c
@@ -1,10 +1,18 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-all-checks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-all-checks -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core \
+// RUN:   -analyzer-store=region -verify %s
+//
 // RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -verify %s
-// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-checker -verify %s 2>&1 | FileCheck %s
+//
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-checker non.existant.Checker -verify %s 2>&1 \
+// RUN:   | FileCheck %s
+//
 // expected-no-diagnostics
 
-// CHECK: use -analyzer-disable-all-checks to disable all static analyzer checkers
+// CHECK: no analyzer checkers are associated with 'non.existant.Checker'
 int buggy() {
   int x = 0;
   return 5/x; // no warning
Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -130,10 +130,11 @@
 
 void ento::printEnabledCheckerList(raw_ostream &out,
ArrayRef plugins,
-   const AnalyzerOptions &opts) {
+   const AnalyzerOptions &opts,
+   DiagnosticsEngine &diags) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  ClangCheckerRegistry(plugins).printList(out, opts);
+  ClangCheckerRegistry(plugins).printList(out, opts, diags);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -9,7 +9,6 @@
 
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/Basic/Diagnostic.h"
-#include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
@@ -19,51 +18,14 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
-#include 
-#include 
 
 using namespace clang;
 using namespace ento;
 
-static const char PackageSeparator = '.';
+static constexpr char PackageSeparator = '.';
 
 using CheckerInfoSet = llvm::SetVector;
 
-namespace {
-/// Represents a request to include or exclude a checker or package from a
-/// specific analysis run.
-///
-/// \sa CheckerRegistry::initializeManager
-class CheckerOptInfo {
-  StringRef Name;
-  bool Enable;
-  bool Claimed;
-
-public:
-  CheckerOptInfo(StringRef name, bool enable)
-: Name(name), Enable(enable), Claimed(false) { }
-
-  StringRef getName() const { return Name; }
-  bool isEnabled() const { return Enable; }
-  bool isDisabled() const { return !isEnabled(); }
-
-  bool isClaimed() const { return Claimed; }
-  bool isUnclaimed() const { return !isClaimed(); }
-  void claim() { Claimed = true; }
-};
-
-} // end of anonymous namespace
-
-static SmallVector
-getCheckerOptList(const AnalyzerOptions &opts) {
-  SmallVector checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-const std::pair &opt = opts.CheckersControlList[i];
-checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
-  }
-  return checkerOpts;
-}
-
 static bool checkerNameLT(const CheckerRegistry::CheckerInfo &a,
   const CheckerRegistry::CheckerInfo &b) {
   return a.FullName < b.FullName;
@@ -86,40 +48,50 @@
   return false;
 }
 
-/// Collects the checkers for the supplied \p opt option into \p collected.
-static void collectCheckers(const CheckerRegistry::CheckerInfoList &checkers,
-const llvm::StringMap &packageSizes,
-CheckerOptInfo &opt, Ch

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

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

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

This bugged me for a long time, so it's time to put an end to it: 
`collectCheckers` was cryptic and hard to understand. This is done by

- Renaming `collectCheckers` to `getEnabledCheckers`
- Changing the functionality to acquire //all// enabled checkers, rather then 
collect checkers for a specific `CheckerOptInfo` (for example, collecting all 
checkers for `{ "core", true }`, which meant enabling all checkers from the 
core package, which was an unnecessary complication).
- Removing `CheckerOptInfo`, instead of storing whether the option was claimed 
via a field, we handle errors immediately, as `getEnabledCheckers` can now 
access a `DiagnosticsEngine`. Realize that the remaining information it stored 
is directly accessible through `AnalyzerOptions.CheckerControlList`.
- Removing the suggestion to disable all checkers when a checker option is left 
unclaimed. I personally found this super annoying, and I'd be very surprised if 
this was a welcome suggestion to anyone.
- Fix a test with `-analyzer-disable-checker -verify` accidentally left in.


Repository:
  rC Clang

https://reviews.llvm.org/D54401

Files:
  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Core/CheckerRegistry.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  test/Analysis/disable-all-checks.c

Index: test/Analysis/disable-all-checks.c
===
--- test/Analysis/disable-all-checks.c
+++ test/Analysis/disable-all-checks.c
@@ -1,10 +1,18 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-all-checks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-all-checks -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -analyzer-checker=core \
+// RUN:   -analyzer-store=region -verify %s
+//
 // RUN: %clang_analyze_cc1 -analyzer-disable-all-checks -verify %s
-// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region -analyzer-disable-checker -verify %s 2>&1 | FileCheck %s
+//
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-store=region \
+// RUN:   -analyzer-disable-checker non.existant.Checker -verify %s 2>&1 \
+// RUN:   | FileCheck %s
+//
 // expected-no-diagnostics
 
-// CHECK: use -analyzer-disable-all-checks to disable all static analyzer checkers
+// CHECK: no analyzer checkers are associated with 'non.existant.Checker'
 int buggy() {
   int x = 0;
   return 5/x; // no warning
Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -130,10 +130,11 @@
 
 void ento::printEnabledCheckerList(raw_ostream &out,
ArrayRef plugins,
-   const AnalyzerOptions &opts) {
+   const AnalyzerOptions &opts,
+   DiagnosticsEngine &diags) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  ClangCheckerRegistry(plugins).printList(out, opts);
+  ClangCheckerRegistry(plugins).printList(out, opts, diags);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -9,7 +9,6 @@
 
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/Basic/Diagnostic.h"
-#include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
@@ -19,51 +18,14 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
-#include 
-#include 
 
 using namespace clang;
 using namespace ento;
 
-static const char PackageSeparator = '.';
+static constexpr char PackageSeparator = '.';
 
 using CheckerInfoSet = llvm::SetVector;
 
-namespace {
-/// Represents a request to include or exclude a checker or package from a
-/// specific analysis run.
-///
-/// \sa CheckerRegistry::initializeManager
-class CheckerOptInfo {
-  StringRef Name;
-  bool Enable;
-  bool Claimed;
-
-public:
-  CheckerOptInfo(StringRef name, bool enable)
-: Name(name), Enable(enable), Claimed(false) { }
-
-  StringRef ge

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

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

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

vmiklos wrote:
> Szelethus wrote:
> > I'm a little confused. To me, it seems like you acquired the condition 
> > already -- doesn't `ConditionRange` actually cover the, well, condition 
> > range? This is how I imagined it:
> > 
> > ```
> > #ifdef CUTE_PANDA_CUBS
> >^~~
> >ConditionRange
> > ```
> > Why is there a need for `getCondition`? Is there any? If there is (maybe 
> > the acquired text contains other things), can you document it? I haven't 
> > played with `PPCallbacks` much, so I'm fine with being in the wrong.
> ConditionRange covers more than what you expect:
> 
> ```
> #if FOO == 4
>^
> void f();
> ~
> #endif
> ~~
> ```
> 
> to find out if the condition of the `#if` is the same as a previous one, I 
> want to extract just `FOO == 4` from that, then deal with that part similar 
> to `#ifdef` and `#ifndef`, which are easier as you have a single Token for 
> the condition. But you're right, I should add a comment explaining this.
Oh my god. There is no tool or a convenient method for this??? I had the 
displeasure of working with the preprocessor in the last couple months, and I 
get shocked by things like this almost every day.

Yea, unfortunately you will have to write excessive amount of comments to 
counterweights the shortcomings of `Preprocessor` :/


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

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

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

I'm a little confused. To me, it seems like you acquired the condition already 
-- doesn't `ConditionRange` actually cover the, well, condition range? This is 
how I imagined it:

```
#ifdef CUTE_PANDA_CUBS
   ^~~
   ConditionRange
```
Why is there a need for `getCondition`? Is there any? If there is (maybe the 
acquired text contains other things), can you document it? I haven't played 
with `PPCallbacks` much, so I'm fine with being in the wrong.


https://reviews.llvm.org/D54349



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

[PATCH] D54397: [analyzer][NFC] Move CheckerOptInfo to CheckerRegistry.cpp, and make it local

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

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

TL;DR: `CheckerOptInfo` feels very much out of place in 
`CheckerRegistration.cpp`, so I moved it to `CheckerRegistry.h`.

Details:

While on the quest of fixing checker options the same way I cleaned up 
non-checker options, although I'm making good progress, I ran into a wall: In 
order to emit warnings on incorrect checker options, we need to ensure that 
checkers actually acquire their options properly -- but, I unearthed a huge bug 
in checker registration: dependencies are currently implemented in a way that 
breaks the already very fragile registration infrastructure.

Here is where the problem really originates from: this is a snippet from 
`CheckerRegistry::initializeManager`.

  // Initialize the CheckerManager with all enabled checkers.
  for (const auto *i :enabledCheckers) {
checkerMgr.setCurrentCheckName(CheckName(i->FullName));
i->Initialize(checkerMgr);
  }

Note that `Initialize` is a function pointer to `register##CHECKERNAME`, like 
`registerInnerPointerChecker`. Since for each register function call the 
current checker name is only set once, when `InnerPointerChecker` attempts to 
also register it's dependent `MallocChecker`, both it and `MallocChecker` will 
receive the `cplusplus.InnerPointer` name. This results in `MallocChecker` 
trying to acquire it's `Optimistic` option as 
`cplusplus.InnerPointerChecker:Optimistic`.

Clearly, this problem has to be solved at a higher level -- it makes no sense 
to be affect other checkers in a registry function. Since I'll already make 
changes to how checker registration works, I'll probably tune some things for 
the current C++ standard, add much needed documentation, and try to make the 
code //a lot less confusing//.


Repository:
  rC Clang

https://reviews.llvm.org/D54397

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

Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -17,7 +17,6 @@
 #include "clang/StaticAnalyzer/Checkers/ClangCheckers.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/CheckerOptInfo.h"
 #include "clang/StaticAnalyzer/Core/CheckerRegistry.h"
 #include "clang/StaticAnalyzer/Frontend/FrontendActions.h"
 #include "llvm/ADT/SmallVector.h"
@@ -102,44 +101,23 @@
   << pluginAPIVersion;
 }
 
-static SmallVector
-getCheckerOptList(const AnalyzerOptions &opts) {
-  SmallVector checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-const std::pair &opt = opts.CheckersControlList[i];
-checkerOpts.push_back(CheckerOptInfo(opt.first, opt.second));
-  }
-  return checkerOpts;
-}
-
 std::unique_ptr ento::createCheckerManager(
 ASTContext &context,
 AnalyzerOptions &opts,
 ArrayRef plugins,
 ArrayRef> checkerRegistrationFns,
 DiagnosticsEngine &diags) {
   auto checkerMgr = llvm::make_unique(context, opts);
 
-  SmallVector checkerOpts = getCheckerOptList(opts);
-
   ClangCheckerRegistry allCheckers(plugins, &diags);
 
   for (const auto &Fn : checkerRegistrationFns)
 Fn(allCheckers);
 
-  allCheckers.initializeManager(*checkerMgr, checkerOpts);
+  allCheckers.initializeManager(*checkerMgr, opts, diags);
   allCheckers.validateCheckerOptions(opts, diags);
   checkerMgr->finishedCheckerRegistration();
 
-  for (unsigned i = 0, e = checkerOpts.size(); i != e; ++i) {
-if (checkerOpts[i].isUnclaimed()) {
-  diags.Report(diag::err_unknown_analyzer_checker)
-  << checkerOpts[i].getName();
-  diags.Report(diag::note_suggest_disabling_all_checkers);
-}
-
-  }
-
   return checkerMgr;
 }
 
@@ -155,8 +133,7 @@
const AnalyzerOptions &opts) {
   out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
 
-  SmallVector checkerOpts = getCheckerOptList(opts);
-  ClangCheckerRegistry(plugins).printList(out, checkerOpts);
+  ClangCheckerRegistry(plugins).printList(out, opts);
 }
 
 void ento::printAnalyzerConfigList(raw_ostream &out) {
Index: lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -12,7 +12,6 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/CheckerOptI

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

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

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+  SourceLocation Loc;
+  std::string MacroName;
+};

vmiklos wrote:
> Szelethus wrote:
> > This is a way too general name in my opinion. Either include comments that 
> > describe it, or rename it (preferably both).
> Renamed to `PreprocessorCondition`, hope it helps. :-)
I actually meant it for `Entry`, if you hover your mouse over an inline 
comment, you can see which lines it applies to. Sorry for the confusing 
communication :D


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

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

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+  SourceLocation Loc;
+  std::string MacroName;
+};

This is a way too general name in my opinion. Either include comments that 
describe it, or rename it (preferably both).


https://reviews.llvm.org/D54349



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