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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347953: [analyzer] Fix the Zombie Symbols bug. 
(authored by dergachev, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D18860

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/MisusedMovedObject.cpp
  test/Analysis/keychainAPI.m
  test/Analysis/loop-block-counts.c
  test/Analysis/pr22954.c
  test/Analysis/retain-release-cpp-classes.cpp
  test/Analysis/self-assign.cpp
  test/Analysis/simple-stream-checks.c
  test/Analysis/unions.cpp

Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -90,9 +90,8 @@
 char str[] = "abc";
 vv.s = str;
 
-// FIXME: This is a leak of uu.s.
 uu = vv;
-  }
+  } // expected-warning{{leak}}
 
   void testIndirectInvalidation() {
 IntOrString uu;
Index: test/Analysis/keychainAPI.m
===
--- test/Analysis/keychainAPI.m
+++ test/Analysis/keychainAPI.m
@@ -212,7 +212,7 @@
 if (st == noErr)
   SecKeychainItemFreeContent(ptr, outData[3]);
   }
-  if (length) { // TODO: We do not report a warning here since the symbol is no longer live, but it's not marked as dead.
+  if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
 length++;
   }
   return 0;
@@ -454,3 +454,15 @@
   }
   return 0;
 }
+int radar_19196494_v2() {
+  @autoreleasepool {
+AuthorizationValue login_password = {};
+OSStatus err = SecKeychainFindGenericPassword(0, 0, "", 0, "", (UInt32 *)_password.length, (void**)_password.data, 0);
+if (!login_password.data) return 0;
+cb.SetContextVal(_password);
+if (err == noErr) {
+  SecKeychainItemFreeContent(0, login_password.data);
+}
+  }
+  return 0;
+}
Index: test/Analysis/pr22954.c
===
--- test/Analysis/pr22954.c
+++ test/Analysis/pr22954.c
@@ -585,7 +585,7 @@
   m28[j].s3[k] = 1;
   struct ll * l28 = (struct ll*)([1]);
   l28->s1[l] = 2;
-  char input[] = {'a', 'b', 'c', 'd'};
+  char input[] = {'a', 'b', 'c', 'd'}; // expected-warning{{Potential leak of memory pointed to by field 's4'}}
   memcpy(l28->s1, input, 4);
   clang_analyzer_eval(m28[0].s3[0] == 1); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(m28[0].s3[1] == 1); // expected-warning{{UNKNOWN}}
Index: test/Analysis/MisusedMovedObject.cpp
===
--- test/Analysis/MisusedMovedObject.cpp
+++ test/Analysis/MisusedMovedObject.cpp
@@ -688,3 +688,25 @@
   c.foo(); // expected-warning {{Method call on a 'moved-from' object 'c'}} expected-note {{Method call on a 'moved-from' object 'c'}}
   C c2 = c;// no-warning
 }
+
+struct Empty {};
+
+Empty inlinedCall() {
+  // Used to warn because region 'e' failed to be cleaned up because no symbols
+  // have ever died during the analysis and the checkDeadSymbols callback
+  // was skipped entirely.
+  Empty e{};
+  return e; // no-warning
+}
+
+void checkInlinedCallZombies() {
+  while (true)
+inlinedCall();
+}
+
+void checkLoopZombies() {
+  while (true) {
+Empty e{};
+Empty f = std::move(e); // no-warning
+  }
+}
Index: test/Analysis/self-assign.cpp
===
--- test/Analysis/self-assign.cpp
+++ test/Analysis/self-assign.cpp
@@ -32,13 +32,14 @@
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
   str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
+// expected-note@-1{{Memory is allocated}}
   return *this;
 }
 
 StringUsed& StringUsed::operator=(StringUsed &) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> I'd very strongly prefer to have this goldnugget copy-pasted even to a simple 
> txt file, and have it commited with this patch.

I guess i'll do it a bit later because it needs to be cleaned up after the 
behavior changes with this patch. There are a few other such discussions that i 
wanted to add to our docs.


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

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] 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] 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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 173932.
NoQ added a comment.

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

> `RetainCountChecker` is affected, as demonstrated by the newly added test.


`RetainCountChecker` is affected due to temporary insanity in the checker that 
was induced by trusting summaries of inlined calls, which was reverted in 
https://reviews.llvm.org/D53902. I'll keep the FP/TN test, but i'll need to 
come up with a new test in order to test whatever i wanted to test, which will 
most likely be a unit test.


https://reviews.llvm.org/D18860

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/MisusedMovedObject.cpp
  test/Analysis/keychainAPI.m
  test/Analysis/loop-block-counts.c
  test/Analysis/pr22954.c
  test/Analysis/retain-release-cpp-classes.cpp
  test/Analysis/self-assign.cpp
  test/Analysis/simple-stream-checks.c
  test/Analysis/unions.cpp

Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -90,9 +90,8 @@
 char str[] = "abc";
 vv.s = str;
 
-// FIXME: This is a leak of uu.s.
 uu = vv;
-  }
+  } // expected-warning{{leak}}
 
   void testIndirectInvalidation() {
 IntOrString uu;
Index: test/Analysis/simple-stream-checks.c
===
--- test/Analysis/simple-stream-checks.c
+++ test/Analysis/simple-stream-checks.c
@@ -89,3 +89,8 @@
   fs.p = fopen("myfile.txt", "w");
   fakeSystemHeaderCall(); // invalidates fs, making fs.p unreachable
 }  // no-warning
+
+void testOverwrite() {
+  FILE *fp = fopen("myfile.txt", "w");
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}
Index: test/Analysis/self-assign.cpp
===
--- test/Analysis/self-assign.cpp
+++ test/Analysis/self-assign.cpp
@@ -32,13 +32,14 @@
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
   str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
+// expected-note@-1{{Memory is allocated}}
   return *this;
 }
 
 StringUsed& StringUsed::operator=(StringUsed &) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   str = rhs.str;
-  rhs.str = nullptr; // FIXME: An improved leak checker should warn here
+  rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}}
   return *this;
 }
 
@@ -83,7 +84,7 @@
 
 int main() {
   StringUsed s1 ("test"), s2;
-  s2 = s1;
-  s2 = std::move(s1);
+  s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} // expected-note{{Returned allocated memory}}
+  s2 = std::move(s1); // expected-note{{Calling move assignment operator for 'StringUsed'}}
   return 0;
 }
Index: test/Analysis/retain-release-cpp-classes.cpp
===
--- /dev/null
+++ test/Analysis/retain-release-cpp-classes.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx -analyzer-output=text -verify %s
+
+// expected-no-diagnostics
+
+typedef void *CFTypeRef;
+typedef struct _CFURLCacheRef *CFURLCacheRef;
+
+CFTypeRef CustomCFRetain(CFTypeRef);
+void invalidate(void *);
+struct S1 {
+  CFTypeRef s;
+  CFTypeRef returnFieldAtPlus0() {
+return s;
+  }
+};
+struct S2 {
+  S1 *s1;
+};
+void foo(S1 *s1) {
+  invalidate(s1);
+  S2 s2;
+  s2.s1 = s1;
+  CustomCFRetain(s1->returnFieldAtPlus0());
+
+  // Definitely no leak end-of-path note here. The retained pointer
+  // is still accessible through s1 and s2.
+  ((void) 0); // no-warning
+
+  // FIXME: Ideally we need to warn after this invalidate(). The per-function
+  // retain-release contract is violated: the programmer should release
+  // the symbol after it was retained, within the same function.
+  

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

The manual region cleanup is removed from `MisusedMovedObjectChecker` in 
https://reviews.llvm.org/D54372.

> Another thing, since we're directly testing the functionality of 
> `SymbolReaper`, maybe it'd be worth to also include unit tests.

Sounds like a great idea, and/or i should add `ExprInspection`-based tests into 
`test/Analysis/symbol-reaper.c`.


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 173487.
NoQ added a comment.

Add an interesting test for the `MisusedMovedObject` checker that demonstrates 
one more potential source of false positives caused by the zombie symbol 
problem. In this test there are, well, //no symbols//. Therefore, there are no 
dead symbols or zombie symbols. Therefore `SymReaper.hasDeadSymbols()` is 
always `false`. Therefore `checkDeadSymbols()` is never called at all. However, 
`MisusedMovedObject` checker is not interested in symbols; it is only 
interested in regions, including concrete regions that aren't based on symbols. 
So it was missing the `checkDeadSymbols()` callback that would have unmarked 
the region for variable `e` (in inlined function or not in inlined function - 
doesn't matter). And next time it sees variable `e` in that function within the 
same stack frame, it thinks it's the same variable that has just been moved.

This problem was already discussed in D24246?id=82469#inline-249803 
.

Add tests in `loop-block-counts.c` that demonstrate the other source of the 
problem in `MisusedMovedObject`: in fact, variable `e` should not be the same 
variable on different iterations of the loop. In case of the inlined function, 
the problem is caused by how our `StackFrameContext` doesn't contain "block 
count" for the entrance - which is a hack to discriminate between different 
iterations of the loop that is used for, eg., conjured symbols, but, 
unfortunately, not for addresses of variables / temporaries. In case of 
non-inlined functions, the problem is deeper: we simply don't have a 
`LocationContext` for a single loop iteration, so there's no way we can 
discriminate between loop locals on different loop iterations by their memory 
spaces.


https://reviews.llvm.org/D18860

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/MisusedMovedObject.cpp
  test/Analysis/keychainAPI.m
  test/Analysis/loop-block-counts.c
  test/Analysis/pr22954.c
  test/Analysis/retain-release-cpp-classes.cpp
  test/Analysis/self-assign.cpp
  test/Analysis/simple-stream-checks.c
  test/Analysis/unions.cpp

Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -90,9 +90,8 @@
 char str[] = "abc";
 vv.s = str;
 
-// FIXME: This is a leak of uu.s.
 uu = vv;
-  }
+  } // expected-warning{{leak}}
 
   void testIndirectInvalidation() {
 IntOrString uu;
Index: test/Analysis/simple-stream-checks.c
===
--- test/Analysis/simple-stream-checks.c
+++ test/Analysis/simple-stream-checks.c
@@ -89,3 +89,8 @@
   fs.p = fopen("myfile.txt", "w");
   fakeSystemHeaderCall(); // invalidates fs, making fs.p unreachable
 }  // no-warning
+
+void testOverwrite() {
+  FILE *fp = fopen("myfile.txt", "w");
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}
Index: test/Analysis/self-assign.cpp
===
--- test/Analysis/self-assign.cpp
+++ test/Analysis/self-assign.cpp
@@ -32,13 +32,14 @@
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
   str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
+// expected-note@-1{{Memory is allocated}}
   return *this;
 }
 
 StringUsed& StringUsed::operator=(StringUsed &) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   str = rhs.str;
-  rhs.str = nullptr; // FIXME: An improved leak checker should warn here
+  rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}}
   return *this;
 }
 
@@ -83,7 +84,7 @@
 
 int main() {
   StringUsed s1 ("test"), s2;
-  s2 = s1;
-  s2 = std::move(s1);
+  s2 = s1; // 

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

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

Szelethus added a comment.

I read through your patch multiple times, have read your mail on cfe-dev, and I 
still couldn't find eve minor nits, well done!

However, both in the mail, and in this discussion, there are a lot of 
invaluable information about the inner workings of the analyzer, that I fear 
will be lost once this revision is closed, even though I don't think the main 
logic will change in the near future. It would be neat to document it for the 
next generation somewhere.

Another thing, since we're directly testing the functionality of 
`SymbolReaper`, maybe it'd be worth to also include unit tests.

I'll probably revisit this revision as I read through your followup patches, 
but I can't seem to find errors in the main logic straight away. GG!


Repository:
  rC Clang

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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Just to be clear - this newly found problem is also automatically fixed by that 
patch.


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 172263.
NoQ added a comment.

Ha! Apart from Zombie symbols that are neither dead nor alive and therefore not 
buried properly, there are also Schrödinger symbols that are dead and alive at 
the same time. Moreover, asking whether a Schrödinger symbol is `isLive()` or 
`maybeDead()` would immediately collapse it into an alive-only state. Moreover, 
if a checker iterates through the dead set and asks whether a Schrödinger 
symbol is alive, undefined behavior occurs. Ah, thin ice.

As far as i understand, this only happens to derived symbols under fairly 
specific circumstances. Namely, if the derived symbol is marked as dead and 
then its parent symbol is marked as live, the derived symbol is not 
automatically removed from the "dead set". It is impossible to automatically 
add derived symbols to the dead set because there may be infinitely many 
symbols derived from the same parent symbol. Therefore there exists moment of 
time when the derived symbol is still in the dead set, but asking whether it is 
`isLive()` would yield true (because it'll check whether the parent symbol is 
alive). Additionally, when `isLive()` notices that it's alive, it automatically 
removes the symbol from the dead set in order to "memoize" the answer, so it 
becomes purely alive. In particular, it invalidates dead set iterators, which 
results in undefined behavior (which in practice boils down to past-end 
iterator access, ultimately crashing Clang).

Schrödinger symbols cause false positive leaks, unlike zombies who only cause 
false negative leaks. But those false positives are relatively rare because 
most checkers don't track derived symbols; eg., MallocChecker only tracks pure 
conjured symbols. RetainCountChecker is affected, as demonstrated by the newly 
added test. Not sure if it's possible to make CStringChecker track a derived 
symbol and forget a string length due to that effect. MacOSKeychainAPI checker 
should also be affected, i think, but i didn't try. NullabilityChecker is 
probably unaffected; its behavior on dead symbols is pretty weird, but it seems 
safe; it also has a separate bug that was exposed but i'll address in a 
separate patch.


https://reviews.llvm.org/D18860

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/keychainAPI.m
  test/Analysis/pr22954.c
  test/Analysis/retain-release-cpp-classes.cpp
  test/Analysis/self-assign.cpp
  test/Analysis/simple-stream-checks.c
  test/Analysis/unions.cpp

Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -90,9 +90,8 @@
 char str[] = "abc";
 vv.s = str;
 
-// FIXME: This is a leak of uu.s.
 uu = vv;
-  }
+  } // expected-warning{{leak}}
 
   void testIndirectInvalidation() {
 IntOrString uu;
Index: test/Analysis/simple-stream-checks.c
===
--- test/Analysis/simple-stream-checks.c
+++ test/Analysis/simple-stream-checks.c
@@ -89,3 +89,8 @@
   fs.p = fopen("myfile.txt", "w");
   fakeSystemHeaderCall(); // invalidates fs, making fs.p unreachable
 }  // no-warning
+
+void testOverwrite() {
+  FILE *fp = fopen("myfile.txt", "w");
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}
Index: test/Analysis/self-assign.cpp
===
--- test/Analysis/self-assign.cpp
+++ test/Analysis/self-assign.cpp
@@ -32,13 +32,14 @@
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
   str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
+// expected-note@-1{{Memory is allocated}}
   return *this;
 }
 
 StringUsed& StringUsed::operator=(StringUsed &) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   str = rhs.str;
-  

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: test/Analysis/simple-stream-checks.c:96
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}

george.karpenkov wrote:
> Woo-hoo, were we losing this case before?
Yes, this is the whole point of the patch.


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added inline comments.
This revision is now accepted and ready to land.



Comment at: test/Analysis/self-assign.cpp:42
   str = rhs.str;
-  rhs.str = nullptr; // FIXME: An improved leak checker should warn here
+  rhs.str = nullptr; // expected-warning{{Potential memory leak}} 
expected-note{{Potential memory leak}}
   return *this;

Woo-hoo!



Comment at: test/Analysis/simple-stream-checks.c:96
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}

Woo-hoo, were we losing this case before?


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/Environment.cpp:180
-  for (; SI != SE; ++SI)
-SymReaper.maybeDead(*SI);
 }

zaks.anna wrote:
> We are removing this because the maybeDead is no longer used, correct?
Yup.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:390
-
-  } else {
-// Call checkers with the non-cleaned state so that they could query the

zaks.anna wrote:
> This removes an optimization to address the following issue: 
> "removeDeadBindings is not run right after the last reference to the object 
> is lost, which leads to imprecise error reports and failure to report the 
> leak in some cases. It's because of how hasDeadSymbols() is implemented. That 
> problem is solved if we disable the optimization, but I do not know how that 
> effects performance. Maybe we can come up with something more clever.
> "
> I suspect the removal of this optimization causes the performance regression. 
> In the patch I sent to the list, this was just a hack to demonstrate what 
> causes the issue. I am not sure we should not just remove the optimization... 
> The best proposal I have is to trigger remove dead bindings at the end of 
> every basic block. This would degrade diagnostics (you will see leaks only at 
> the end of the basic block), but should give us performance back or even 
> improve performance.
> 
> Artem and Devin, WDYT?
> 
> Artem, can you experiment with this and investigate if the diagnostics become 
> much worse? Maybe send a couple of examples? I suggest we implement this mode 
> behind a flag as a separate patch.
This cannot be kept around because `.hasDeadSymbols()` cannot be implemented 
correctly.



Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2452
-SymReaper.maybeDead(*SI);
-}
   }

zaks.anna wrote:
> Looks like we are saying that we should no longer add to maybeDead because 
> it's not used.
Yup.


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 171006.
NoQ marked 2 inline comments as done.
NoQ added a comment.
Herald added subscribers: dkrupp, donat.nagy, Szelethus, mikhail.ramalho.

Rebase!

Changes on large codebase runs still look mild and reasonable, with much less 
slowdown than before. Even if there's in fact a 5-10% slowdown, i believe we 
can live with that, as there doesn't seem to be a better solution for the 
purposes of correctness.

The most important consequence of the rebase was the conflict that lead to test 
failure in `MacOSKeychainAPIChecker` on the `radar_19196494()` test. The story 
here is that https://reviews.llvm.org/D28330 unconsciously adds a suppression 
that relies on zombie symbols. I replaced this suppression with another ugly 
suppression that should be at least a tiny bit in the right direction.


https://reviews.llvm.org/D18860

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/keychainAPI.m
  test/Analysis/pr22954.c
  test/Analysis/self-assign.cpp
  test/Analysis/simple-stream-checks.c
  test/Analysis/unions.cpp

Index: test/Analysis/unions.cpp
===
--- test/Analysis/unions.cpp
+++ test/Analysis/unions.cpp
@@ -90,9 +90,8 @@
 char str[] = "abc";
 vv.s = str;
 
-// FIXME: This is a leak of uu.s.
 uu = vv;
-  }
+  } // expected-warning{{leak}}
 
   void testIndirectInvalidation() {
 IntOrString uu;
Index: test/Analysis/simple-stream-checks.c
===
--- test/Analysis/simple-stream-checks.c
+++ test/Analysis/simple-stream-checks.c
@@ -89,3 +89,8 @@
   fs.p = fopen("myfile.txt", "w");
   fakeSystemHeaderCall(); // invalidates fs, making fs.p unreachable
 }  // no-warning
+
+void testOverwrite() {
+  FILE *fp = fopen("myfile.txt", "w");
+  fp = 0;
+} // expected-warning {{Opened file is never closed; potential resource leak}}
Index: test/Analysis/self-assign.cpp
===
--- test/Analysis/self-assign.cpp
+++ test/Analysis/self-assign.cpp
@@ -32,13 +32,14 @@
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
   str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
+// expected-note@-1{{Memory is allocated}}
   return *this;
 }
 
 StringUsed& StringUsed::operator=(StringUsed &) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
   clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
   str = rhs.str;
-  rhs.str = nullptr; // FIXME: An improved leak checker should warn here
+  rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}}
   return *this;
 }
 
@@ -83,7 +84,7 @@
 
 int main() {
   StringUsed s1 ("test"), s2;
-  s2 = s1;
-  s2 = std::move(s1);
+  s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} // expected-note{{Returned allocated memory}}
+  s2 = std::move(s1); // expected-note{{Calling move assignment operator for 'StringUsed'}}
   return 0;
 }
Index: test/Analysis/pr22954.c
===
--- test/Analysis/pr22954.c
+++ test/Analysis/pr22954.c
@@ -585,7 +585,7 @@
   m28[j].s3[k] = 1;
   struct ll * l28 = (struct ll*)([1]);
   l28->s1[l] = 2;
-  char input[] = {'a', 'b', 'c', 'd'};
+  char input[] = {'a', 'b', 'c', 'd'}; // expected-warning{{Potential leak of memory pointed to by field 's4'}}
   memcpy(l28->s1, input, 4);
   clang_analyzer_eval(m28[0].s3[0] == 1); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(m28[0].s3[1] == 1); // expected-warning{{UNKNOWN}}
Index: test/Analysis/keychainAPI.m
===
--- test/Analysis/keychainAPI.m
+++ test/Analysis/keychainAPI.m
@@ -212,7 +212,7 @@
 if (st == noErr)
   SecKeychainItemFreeContent(ptr, outData[3]);
   }
-  if (length) { // TODO: We do not 

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

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: baloghadamsoftware.

The change LGTM after nits and rebasing IF the new evaluation will not show a 
too large regression.




Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:3925
   // Update counts from autorelease pools
-  for (SymbolReaper::dead_iterator I = SymReaper.dead_begin(),
-   E = SymReaper.dead_end(); I != E; ++I) {
-SymbolRef Sym = *I;
-if (const RefVal *T = B.lookup(Sym)){
+  for (auto I = B.begin(), E = B.end(); I != E; ++I) {
+SymbolRef Sym = I->first;

for .. in loop?



Comment at: lib/StaticAnalyzer/Checkers/StreamChecker.cpp:402
+  const StreamMapTy  = state->get();
+  for (auto I = Map.begin(), E = Map.end(); I != E; ++I) {
+SymbolRef Sym = I->first;

foreach loop?


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@NoQ can we get the evaluation re-done?


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] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

What is the status of this patch? Is there a consensus?


https://reviews.llvm.org/D18860



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