[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2018-01-06 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

https://reviews.llvm.org/D41538 is superior and committed.


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-12-22 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.
Herald added subscribers: a.sidorin, rnkovacs.

In case you do not like this solution, I uploaded an alternative approach: 
https://reviews.llvm.org/D41538


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-11-24 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

@dcoughlin do you have some input on this?


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-10-31 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

In https://reviews.llvm.org/D37437#909537, @zaks.anna wrote:

> Just to be clear, since this leads to regression to the checker API, I am 
> asking to look into other ways of solving this problem. For example, is there 
> a way to ensure that the checker names are set at construction?


Checkers are instantiated here: 
https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/CheckerManager.h#L142
Checkers are created using the default constructor. After the constructor was 
run we set the name. If we want the checkers to be able to create the BugType 
with the correct name in the constructor (and not using mutable fields for the 
BugTypes), we need to add a new constructor parameter to CheckerBase and all 
the Checkers will need to forward a CheckerName to their base classes. So we 
need to break the API for every checkers basically. But if you prefer that 
solution I can rewrite this patch.


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-10-27 Thread Anna Zaks via Phabricator via cfe-commits 
zaks.anna added a comment.

Just to be clear, since this leads to regression to the checker API, I am 
asking to look into other ways of solving this problem. For example, is there a 
way to ensure that the checker names are set at construction?


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-10-16 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:308
   if (StOutBound && !StInBound) {
+if (!Filter.CheckCStringOutOfBounds)
+  return state;

zaks.anna wrote:
> This seems to be related to the change in the test, correct? In the future, 
> please, split changes like this one into their own separate commits.
Yes. I will do so in the future, thanks.


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-10-16 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun updated this revision to Diff 119141.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.
Herald added a subscriber: szepet.

- Address review comments.


https://reviews.llvm.org/D37437

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1720,13 +1720,6 @@
   }
 }
 
-char *dupstrWarn(const char *s) {
-  const int len = strlen(s);
-  char *p = (char*) smallocWarn(len + 1);
-  strcpy(p, s); // expected-warning{{String copy function overflows destination buffer}}
-  return p;
-}
-
 int *radar15580979() {
   int *data = (int *)malloc(32);
   int *p = data ?: (int*)malloc(32); // no warning
Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -64,7 +64,7 @@
  CheckerContext ) const;
   void reportLeakedVALists(const RegionVector , StringRef Msg1,
StringRef Msg2, CheckerContext , ExplodedNode *N,
-   bool ForceReport = false) const;
+   bool ReportUninit = false) const;
 
   void checkVAListStartCall(const CallEvent , CheckerContext ,
 bool IsCopy) const;
@@ -267,15 +267,15 @@
 void ValistChecker::reportLeakedVALists(const RegionVector ,
 StringRef Msg1, StringRef Msg2,
 CheckerContext , ExplodedNode *N,
-bool ForceReport) const {
+bool ReportUninit) const {
   if (!(ChecksEnabled[CK_Unterminated] ||
-(ChecksEnabled[CK_Uninitialized] && ForceReport)))
+(ChecksEnabled[CK_Uninitialized] && ReportUninit)))
 return;
   for (auto Reg : LeakedVALists) {
 if (!BT_leakedvalist) {
-  BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated],
-"Leaked va_list",
-categories::MemoryError));
+  BT_leakedvalist.reset(new BugType(
+  CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+  "Leaked va_list", categories::MemoryError));
   BT_leakedvalist->setSuppressOnSink(true);
 }
 
@@ -375,7 +375,7 @@
 
 std::shared_ptr ValistChecker::ValistBugVisitor::VisitNode(
 const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext ,
-BugReport ) {
+BugReport &) {
   ProgramStateRef State = N->getState();
   ProgramStateRef StatePrev = PrevN->getState();
 
Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
@@ -54,8 +54,8 @@
check::PointerEscape> {
   CallDescription OpenFn, CloseFn;
 
-  std::unique_ptr DoubleCloseBugType;
-  std::unique_ptr LeakBugType;
+  mutable std::unique_ptr DoubleCloseBugType;
+  mutable std::unique_ptr LeakBugType;
 
   void reportDoubleClose(SymbolRef FileDescSym,
  const CallEvent ,
@@ -104,16 +104,7 @@
 } // end anonymous namespace
 
 SimpleStreamChecker::SimpleStreamChecker()
-: OpenFn("fopen"), CloseFn("fclose", 1) {
-  // Initialize the bug types.
-  DoubleCloseBugType.reset(
-  new BugType(this, "Double fclose", "Unix Stream API Error"));
-
-  LeakBugType.reset(
-  new BugType(this, "Resource Leak", "Unix Stream API Error"));
-  // Sinks are higher importance bugs as well as calls to assert() or exit(0).
-  LeakBugType->setSuppressOnSink(true);
-}
+: OpenFn("fopen"), CloseFn("fclose", 1) {}
 
 void SimpleStreamChecker::checkPostCall(const CallEvent ,
 CheckerContext ) const {
@@ -206,6 +197,10 @@
   if (!ErrNode)
 return;
 
+  if (!DoubleCloseBugType)
+DoubleCloseBugType.reset(
+new BugType(this, "Double fclose", "Unix Stream API Error"));

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-10-13 Thread Anna Zaks via Phabricator via cfe-commits 
zaks.anna added a comment.

Looks like the need to have the checker name in BugType along with the checker 
names not being initialized early enough, leads to worse checker-writer 
experience. Is there a way to ensure that the checker names are set at 
construction?




Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:131
  << "' inside of critical section";
-  auto R = llvm::make_unique(*BlockInCritSectionBugType, os.str(), 
ErrNode);
+  if (!BlockInCritSectionBugType)
+BlockInCritSectionBugType.reset(

nit: I prefer to use '{' here since the if body spans multiple lines.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:308
   if (StOutBound && !StInBound) {
+if (!Filter.CheckCStringOutOfBounds)
+  return state;

This seems to be related to the change in the test, correct? In the future, 
please, split changes like this one into their own separate commits.


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-09-21 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

Ping.


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-09-05 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

In https://reviews.llvm.org/D37437#860311, @NoQ wrote:

> Cool. Thanks!
>
> > In the future probably it would be better to alter the signature of the 
> > checkers' constructor to set the name in the constructor so it is possible 
> > to create the BugType eagerly.
>
> Still, should we add an assertion so that we could be sure that every bug 
> type contains a checker name?


Sure! I added the assert and discovered lots of other checks that had the same 
problem. I also discovered some other bugs, e.g.: when a checker was emitting 
diagnostic even without being turned on or the wrong name is emitted to the 
plist.




Comment at: test/Analysis/malloc.c:1723
 
-char *dupstrWarn(const char *s) {
-  const int len = strlen(s);

This test is deleted because the corresponding checker is not even turned on 
for this file and this warning should not be emitted at all. Emitting this 
warning with the settings above was a bug. 


https://reviews.llvm.org/D37437



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

[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name

2017-09-05 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun updated this revision to Diff 113837.
xazax.hun retitled this revision from "[analyzer] Fix SimpleStreamChecker's 
output plist not containing the checker name" to "[analyzer] Fix some checker's 
output plist not containing the checker name".
xazax.hun edited the summary of this revision.
xazax.hun added a comment.

- Fix more checkers
- Minor style fixes along the way
- Added an assert to prevent such errors in the future


https://reviews.llvm.org/D37437

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1720,13 +1720,6 @@
   }
 }
 
-char *dupstrWarn(const char *s) {
-  const int len = strlen(s);
-  char *p = (char*) smallocWarn(len + 1);
-  strcpy(p, s); // expected-warning{{String copy function overflows destination buffer}}
-  return p;
-}
-
 int *radar15580979() {
   int *data = (int *)malloc(32);
   int *p = data ?: (int*)malloc(32); // no warning
Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -64,7 +64,7 @@
  CheckerContext ) const;
   void reportLeakedVALists(const RegionVector , StringRef Msg1,
StringRef Msg2, CheckerContext , ExplodedNode *N,
-   bool ForceReport = false) const;
+   bool ReportUninit = false) const;
 
   void checkVAListStartCall(const CallEvent , CheckerContext ,
 bool IsCopy) const;
@@ -267,15 +267,15 @@
 void ValistChecker::reportLeakedVALists(const RegionVector ,
 StringRef Msg1, StringRef Msg2,
 CheckerContext , ExplodedNode *N,
-bool ForceReport) const {
+bool ReportUninit) const {
   if (!(ChecksEnabled[CK_Unterminated] ||
-(ChecksEnabled[CK_Uninitialized] && ForceReport)))
+(ChecksEnabled[CK_Uninitialized] && ReportUninit)))
 return;
   for (auto Reg : LeakedVALists) {
 if (!BT_leakedvalist) {
-  BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated],
-"Leaked va_list",
-categories::MemoryError));
+  BT_leakedvalist.reset(new BugType(
+  CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+  "Leaked va_list", categories::MemoryError));
   BT_leakedvalist->setSuppressOnSink(true);
 }
 
@@ -375,7 +375,7 @@
 
 std::shared_ptr ValistChecker::ValistBugVisitor::VisitNode(
 const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext ,
-BugReport ) {
+BugReport &) {
   ProgramStateRef State = N->getState();
   ProgramStateRef StatePrev = PrevN->getState();
 
Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
@@ -54,8 +54,8 @@
check::PointerEscape> {
   CallDescription OpenFn, CloseFn;
 
-  std::unique_ptr DoubleCloseBugType;
-  std::unique_ptr LeakBugType;
+  mutable std::unique_ptr DoubleCloseBugType;
+  mutable std::unique_ptr LeakBugType;
 
   void reportDoubleClose(SymbolRef FileDescSym,
  const CallEvent ,
@@ -104,16 +104,7 @@
 } // end anonymous namespace
 
 SimpleStreamChecker::SimpleStreamChecker()
-: OpenFn("fopen"), CloseFn("fclose", 1) {
-  // Initialize the bug types.
-  DoubleCloseBugType.reset(
-  new BugType(this, "Double fclose", "Unix Stream API Error"));
-
-  LeakBugType.reset(
-  new BugType(this, "Resource Leak", "Unix Stream API Error"));
-  // Sinks are higher importance bugs as well as calls to assert() or exit(0).
-  LeakBugType->setSuppressOnSink(true);
-}
+: OpenFn("fopen"), CloseFn("fclose", 1) {}
 
 void SimpleStreamChecker::checkPostCall(const