NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus,
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, szepet.
Herald added a project: clang.
This is slightly controversial, please hold me back / bikeshed if you don't
like it.
The `optin.cplusplus.VirtualCall` checker in its current shape was implemented
by @wangxindsb during his GSoC'2017 project - see D34275
<https://reviews.llvm.org/D34275>. It is a path-sensitive checker that finds
calls to virtual functions during construction and destruction. This is a
problem because virtual dispatch won't occur during construction or
destruction. In particular, if the function is pure virtual, it is outright
undefined behavior; in other cases it is simply an unexpected behavior. The
checker has an option `PureOnly` to control whether the user is only interested
in calls to pure virtual functions. It defaults to false, i.e. warn on
non-pure-virtual calls as well.
I propose the following changes:
- Remove the option.
- Instead, split the checker in two:
- `cplusplus.PureVirtualCall` will only check pure virtual calls and it
//will be on by default//.
- `optin.cplusplus.VirtualCall` will check all calls and will remain opt-in
under the old name. It would automatically load `cplusplus.PureVirtualCall`.
- Additionally, remove the bug visitor. The reasons for that have been
described in D34275 <https://reviews.llvm.org/D34275>?id=111862#inline-321253:
essentially, the visitor doesn't add any new interesting information to the
report. It's also currently broken: it places its note in a really weird spot.
I'd like to point out that it breaks backwards compatibility. I'm not going to
be hurt by this and i haven't heard a lot about users of this opt-in check but
if you know them, please let me know. If we agree to break backwards
compatibility, we should make sure that the result is the best, so i'd like to
hear @Szelethus's opinion, as he usually has strong opinions on checker options
and dependencies :)
See also http://lists.llvm.org/pipermail/cfe-dev/2018-December/060558.html (the
thread was bumped recently)
Repository:
rC Clang
https://reviews.llvm.org/D64274
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
clang/test/Analysis/analyzer-config.c
clang/test/Analysis/virtualcall.cpp
clang/test/Analysis/virtualcall.h
Index: clang/test/Analysis/virtualcall.h
===================================================================
--- clang/test/Analysis/virtualcall.h
+++ clang/test/Analysis/virtualcall.h
@@ -2,12 +2,7 @@
class Z {
public:
Z() {
- foo();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'Z' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ foo(); // impure-warning {{Call to virtual function during construction}}
}
virtual int foo();
};
Index: clang/test/Analysis/virtualcall.cpp
===================================================================
--- clang/test/Analysis/virtualcall.cpp
+++ clang/test/Analysis/virtualcall.cpp
@@ -1,6 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-output=text -verify -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN: -verify=expected,impure -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -analyzer-output=text -verify -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
+// RUN: -verify=expected -std=c++11 %s
#include "virtualcall.h"
@@ -13,54 +15,31 @@
virtual int foo() = 0;
virtual void bar() = 0;
void f() {
- foo();
- // expected-warning-re@-1 {{{{^}}Call to pure virtual function during construction}}
- // expected-note-re@-2 {{{{^}}Call to pure virtual function during construction}}
+ foo(); // expected-warning{{Call to pure virtual function during construction}}
}
};
class B : public A {
public:
- B() { // expected-note {{Calling default constructor for 'A'}}
- foo();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'B' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ B() {
+ foo(); // impure-warning {{Call to virtual function during construction}}
}
~B();
virtual int foo();
virtual void bar() {
- foo();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during destruction}}
- // expected-note-re@-3 {{{{^}}Call to virtual function during destruction}}
-#endif
- }
+ foo(); // impure-warning{{Call to virtual function during destruction}}
+ }
};
-A::A() {
- f();
-// expected-note-re@-1 {{{{^}}This constructor of an object of type 'A' has not returned when the virtual method was called}}
-// expected-note-re@-2 {{{{^}}Calling 'A::f'}}
+A::A() {
+ f();
}
B::~B() {
this->B::foo(); // no-warning
this->B::bar();
-#if !PUREONLY
- // expected-note-re@-2 {{{{^}}This destructor of an object of type '~B' has not returned when the virtual method was called}}
- // expected-note-re@-3 {{{{^}}Calling 'B::bar'}}
-#endif
- this->foo();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during destruction}}
- // expected-note-re@-3 {{{{^}}This destructor of an object of type '~B' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during destruction}}
-#endif
-
+ this->foo(); // impure-warning {{Call to virtual function during destruction}}
}
class C : public B {
@@ -73,12 +52,7 @@
};
C::C() {
- f(foo());
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'C' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ f(foo()); // impure-warning {{Call to virtual function during construction}}
}
class D : public B {
@@ -97,9 +71,6 @@
foo(); // no-warning
}
~E() { bar(); }
-#if !PUREONLY
- // expected-note-re@-2 2{{{{^}}Calling '~B'}}
-#endif
int foo() override;
};
@@ -135,52 +106,23 @@
G g;
g.foo();
g.bar(); // no warning
- f();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'H' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ f(); // impure-warning {{Call to virtual function during construction}}
H &h = *this;
- h.f();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'H' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ h.f(); // impure-warning {{Call to virtual function during construction}}
}
};
class X {
public:
X() {
- g();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'X' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ g(); // impure-warning {{Call to virtual function during construction}}
}
X(int i) {
if (i > 0) {
-#if !PUREONLY
- // expected-note-re@-2 {{{{^}}'i' is > 0}}
- // expected-note-re@-3 {{{{^}}Taking true branch}}
- // expected-note-re@-4 {{{{^}}'i' is <= 0}}
- // expected-note-re@-5 {{{{^}}Taking false branch}}
-#endif
X x(i - 1);
-#if !PUREONLY
- // expected-note-re@-2 {{{{^}}Calling constructor for 'X'}}
-#endif
x.g(); // no warning
}
- g();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}This constructor of an object of type 'X' has not returned when the virtual method was called}}
- // expected-note-re@-4 {{{{^}}Call to virtual function during construction}}
-#endif
+ g(); // impure-warning {{Call to virtual function during construction}}
}
virtual void g();
};
@@ -197,19 +139,11 @@
N n;
n.virtualMethod(); // no warning
n.callFooOfM(this);
-#if !PUREONLY
- // expected-note-re@-2 {{{{^}}This constructor of an object of type 'M' has not returned when the virtual method was called}}
- // expected-note-re@-3 {{{{^}}Calling 'N::callFooOfM'}}
-#endif
}
virtual void foo();
};
void N::callFooOfM(M *m) {
- m->foo();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}Call to virtual function during construction}}
-#endif
+ m->foo(); // impure-warning {{Call to virtual function during construction}}
}
class Y {
@@ -217,65 +151,27 @@
virtual void foobar();
void fooY() {
F f1;
- foobar();
-#if !PUREONLY
- // expected-warning-re@-2 {{{{^}}Call to virtual function during construction}}
- // expected-note-re@-3 {{{{^}}Call to virtual function during construction}}
-#endif
+ foobar(); // impure-warning {{Call to virtual function during construction}}
}
Y() { fooY(); }
-#if !PUREONLY
- // expected-note-re@-2 {{{{^}}This constructor of an object of type 'Y' has not returned when the virtual method was called}}
- // expected-note-re@-3 {{{{^}}Calling 'Y::fooY'}}
-#endif
};
int main() {
B b;
-#if PUREONLY
- //expected-note-re@-2 {{{{^}}Calling default constructor for 'B'}}
-#else
- //expected-note-re@-4 2{{{{^}}Calling default constructor for 'B'}}
-#endif
C c;
-#if !PUREONLY
- //expected-note-re@-2 {{{{^}}Calling default constructor for 'C'}}
-#endif
D d;
E e;
F f;
G g;
H h;
H h1(1);
-#if !PUREONLY
- //expected-note-re@-2 {{{{^}}Calling constructor for 'H'}}
- //expected-note-re@-3 {{{{^}}Calling constructor for 'H'}}
-#endif
X x;
-#if !PUREONLY
- //expected-note-re@-2 {{{{^}}Calling default constructor for 'X'}}
-#endif
X x1(1);
-#if !PUREONLY
- //expected-note-re@-2 {{{{^}}Calling constructor for 'X'}}
-#endif
M m;
-#if !PUREONLY
- //expected-note-re@-2 {{{{^}}Calling default constructor for 'M'}}
-#endif
Y *y = new Y;
-#if !PUREONLY
- //expected-note-re@-2 {{{{^}}Calling default constructor for 'Y'}}
-#endif
delete y;
header::Z z;
-#if !PUREONLY
- // expected-note-re@-2 {{{{^}}Calling default constructor for 'Z'}}
-#endif
}
-#if !PUREONLY
- //expected-note-re@-2 2{{{{^}}Calling '~E'}}
-#endif
namespace PR34451 {
struct a {
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -70,7 +70,6 @@
// CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreRecordsWithField = ""
// CHECK-NEXT: optin.cplusplus.UninitializedObject:NotesAsWarnings = false
// CHECK-NEXT: optin.cplusplus.UninitializedObject:Pedantic = false
-// CHECK-NEXT: optin.cplusplus.VirtualCall:PureOnly = false
// CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false
// CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
// CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
@@ -90,4 +89,4 @@
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: widen-loops = false
// CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 87
+// CHECK-NEXT: num-entries = 86
Index: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -44,7 +44,7 @@
public:
// The flag to determine if pure virtual functions should be issued only.
- DefaultBool IsPureOnly;
+ bool IsPureOnly = true;
void checkBeginFunction(CheckerContext &C) const;
void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
@@ -55,84 +55,12 @@
CheckerContext &C) const;
void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg,
CheckerContext &C) const;
-
- class VirtualBugVisitor : public BugReporterVisitor {
- private:
- const MemRegion *ObjectRegion;
- bool Found;
-
- public:
- VirtualBugVisitor(const MemRegion *R) : ObjectRegion(R), Found(false) {}
-
- void Profile(llvm::FoldingSetNodeID &ID) const override {
- static int X = 0;
- ID.AddPointer(&X);
- ID.AddPointer(ObjectRegion);
- }
-
- std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
- BugReporterContext &BRC,
- BugReport &BR) override;
- };
};
} // end namespace
// GDM (generic data map) to the memregion of this for the ctor and dtor.
REGISTER_MAP_WITH_PROGRAMSTATE(CtorDtorMap, const MemRegion *, ObjectState)
-std::shared_ptr<PathDiagnosticPiece>
-VirtualCallChecker::VirtualBugVisitor::VisitNode(const ExplodedNode *N,
- BugReporterContext &BRC,
- BugReport &) {
- // We need the last ctor/dtor which call the virtual function.
- // The visitor walks the ExplodedGraph backwards.
- if (Found)
- return nullptr;
-
- ProgramStateRef State = N->getState();
- const LocationContext *LCtx = N->getLocationContext();
- const CXXConstructorDecl *CD =
- dyn_cast_or_null<CXXConstructorDecl>(LCtx->getDecl());
- const CXXDestructorDecl *DD =
- dyn_cast_or_null<CXXDestructorDecl>(LCtx->getDecl());
-
- if (!CD && !DD)
- return nullptr;
-
- ProgramStateManager &PSM = State->getStateManager();
- auto &SVB = PSM.getSValBuilder();
- const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl());
- if (!MD)
- return nullptr;
- auto ThiSVal =
- State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame()));
- const MemRegion *Reg = ThiSVal.castAs<loc::MemRegionVal>().getRegion();
- if (!Reg)
- return nullptr;
- if (Reg != ObjectRegion)
- return nullptr;
-
- const Stmt *S = PathDiagnosticLocation::getStmt(N);
- if (!S)
- return nullptr;
- Found = true;
-
- std::string InfoText;
- if (CD)
- InfoText = "This constructor of an object of type '" +
- CD->getNameAsString() +
- "' has not returned when the virtual method was called";
- else
- InfoText = "This destructor of an object of type '" +
- DD->getNameAsString() +
- "' has not returned when the virtual method was called";
-
- // Generate the extra diagnostic.
- PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
- N->getLocationContext());
- return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
-}
-
// The function to check if a callexpr is a virtual function.
static bool isVirtualCall(const CallExpr *CE) {
bool CallIsNonVirtual = false;
@@ -271,15 +199,20 @@
"C++ Object Lifecycle"));
auto Reporter = llvm::make_unique<BugReport>(*BT, Msg, N);
- Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(Reg));
C.emitReport(std::move(Reporter));
}
+void ento::registerPureVirtualCallChecker(CheckerManager &mgr) {
+ mgr.registerChecker<VirtualCallChecker>();
+}
+
void ento::registerVirtualCallChecker(CheckerManager &mgr) {
VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
+ checker->IsPureOnly = false;
+}
- checker->IsPureOnly =
- mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "PureOnly");
+bool ento::shouldRegisterPureVirtualCallChecker(const LangOptions &LO) {
+ return true;
}
bool ento::shouldRegisterVirtualCallChecker(const LangOptions &LO) {
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -507,6 +507,9 @@
]>,
Documentation<HasDocumentation>;
+def PureVirtualCallChecker : Checker<"PureVirtualCall">,
+ HelpText<"Check pure virtual function calls during construction/destruction">,
+ Documentation<HasDocumentation>;
} // end: "cplusplus"
let ParentPackage = CplusplusOptIn in {
@@ -555,14 +558,7 @@
Documentation<HasAlphaDocumentation>;
def VirtualCallChecker : Checker<"VirtualCall">,
- HelpText<"Check virtual function calls during construction or destruction">,
- CheckerOptions<[
- CmdLineOption<Boolean,
- "PureOnly",
- "Whether to only report calls to pure virtual methods.",
- "false",
- Released>
- ]>,
+ HelpText<"Check virtual function calls during construction/destruction">,
Documentation<HasDocumentation>;
} // end: "optin.cplusplus"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
