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>()) {
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>()) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
