[analyzer] pr38838, pr39976: Fix a crash on emitting diagnostics before destructor.

[NoQ]

Summary

All right guys, now this one's weird.

This patch looks fairly reasonable at a glance. Like, we need to be able to emit the diagnostic at PreImplicitCall, and the patch implements this functionality.

The real question, as usual, is why didn't we need that before. If you look at the test, you'll see that i'm not testing for any diagnostics. Let me explain.

The test emits two bug reports for

test/Analysis/diagnostics/dtors.cpp:23:3: note: Called C++ object pointer is null
  p.get()->foo();
  ^~~~~~~

Both of them are suppressed because by default we have -analyzer-config suppress-null-return-paths=true. If you change it to false, you'll see a warning, but it won't have any diagnostics attached to line 22, where the destructor is called. That's because you see the first warning, and the second warning gets de-duplicated out. Now, if you suppress the first warning with an early exit like this:

   21 void bar(smart_ptr p) {
+  22   if (p.x)
+  23     return;
   24   delete p.get();
   25   p.get()->foo();
   26 }

...then you'll see the second report, which would include the piece we're looking for:

test/Analysis/diagnostics/dtors.cpp:24:16: note: Assuming pointer value is null
  delete p.get();
               ^

The piece says that p.s is assumed to be non-null by the null dereference checker: otherwise we wouldn't have been able to call the destructor. This is fine.

The bad thing here is that if p.s is non-null, then the warning about calling a method on a null pointer is false! Therefore i don't want to add tests for diagnostic messages: they're incorrect and would need to be removed anyway.

Now, why do we have the false positive? That's because we have a dead symbol collection problem :/ Namely, the symbol for p.x gets garbage-collected too early, even though p is still alive as an lvalue expression. Then the constraint range reg_$1<p.x>: [0, 0] is lost, and when we're entering get() for the second time, reg_$1<p.x> is re-assumed to be non-zero. Ugh.

Finally, i've no idea how to come up with a true positive to test this note piece. The only thing i can come up with that can happen in pre-call to destructor is this null dereference assumption. But what warning would you emit over a pointer that's known to be non-null so that the pointer was an interesting symbol with respect to this warning? I just can't come up with anything. Which explains why don't we see that many crashes of that kind. It seems that they only occur on false positives of a certain kind, which of course need to be fixed.

Still, fixing a crash is better than nothing, so i propose to land this before i dive into this dead symbol problem.

cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

if (Optional<BlockEdge> BE = P.getAs<BlockEdge>()) {
     S = BSrc->getTerminatorCondition();
   } else if (Optional<StmtPoint> SP = P.getAs<StmtPoint>()) {
     S = SP->getStmt();
     if (P.getAs<PostStmtPurgeDeadSymbols>())
       return PathDiagnosticLocation::createEnd(S, SMng, P.getLocationContext());
   } else if (Optional<PostInitializer> PIP = P.getAs<PostInitializer>()) {
     return PathDiagnosticLocation(PIP->getInitializer()->getSourceLocation(),
                                   SMng);
+  } else if (Optional<PreImplicitCall> PIC = P.getAs<PreImplicitCall>()) {
+    return PathDiagnosticLocation(PIC->getLocation(), SMng);
   } else if (Optional<PostImplicitCall> PIE = P.getAs<PostImplicitCall>()) {
     return PathDiagnosticLocation(PIE->getLocation(), SMng);
   } else if (Optional<CallEnter> CE = P.getAs<CallEnter>()) {
     return getLocationForCaller(CE->getCalleeContext(),
                                 CE->getLocationContext(),
                                 SMng);
   } else if (Optional<CallExitEnd> CEE = P.getAs<CallExitEnd>()) {
     return getLocationForCaller(CEE->getCalleeContext(),

cfe/trunk/test/Analysis/diagnostics/dtors.cpp

+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus -verify %s
+
+// expected-no-diagnostics
+
+namespace no_crash_on_delete_dtor {
+// We were crashing when producing diagnostics for this code.
+struct S {
+  void foo();
+  ~S();
+};
+
+struct smart_ptr {
+  int x;
+  S *s;
+  smart_ptr(S *);
+  S *get() {
+    return (x || 0) ? nullptr : s;
+  }
+};
+
+void bar(smart_ptr p) {
+  delete p.get();
+  p.get()->foo();
+}
+} // namespace no_crash_on_delete_dtor