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

[Artem Dergachev via Phabricator via cfe-commits]

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

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 for `reg_$1<p.x>` is lost, and when we're entering `get()` for the second time, it is re-assumed to be null. 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.


Repository:
  rC Clang

https://reviews.llvm.org/D56042

Files:
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/diagnostics/dtors.cpp


Index: test/Analysis/diagnostics/dtors.cpp
===================================================================
--- /dev/null
+++ test/Analysis/diagnostics/dtors.cpp
@@ -0,0 +1,25 @@
+// 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
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -723,6 +723,8 @@
   } 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>()) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56042.179391.patch
Type: text/x-patch
Size: 1426 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181222/1a64e557/attachment.bin>