[analyzer] Treat break, continue, goto, and label statements as trivial in WebKit checkers. #91873
Conversation
llvmbot
added
clang
…al in WebKit checkers. Also allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list.
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesAlso allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list. Full diff: https://github.com/llvm/llvm-project/pull/91873.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index ad493587affa0..e6d014e3ba8e1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -308,6 +308,12 @@ class TrivialFunctionAnalysisVisitor
bool VisitCaseStmt(const CaseStmt *CS) { return VisitChildren(CS); }
bool VisitDefaultStmt(const DefaultStmt *DS) { return VisitChildren(DS); }
+ // break, continue, goto, and label statements are always trivial.
+ bool VisitBreakStmt(const BreakStmt*) { return true; }
+ bool VisitContinueStmt(const ContinueStmt*) { return true; }
+ bool VisitGotoStmt(const GotoStmt*) { return true; }
+ bool VisitLabelStmt(const LabelStmt*) { return true; }
+
bool VisitUnaryOperator(const UnaryOperator *UO) {
// Unary operators are trivial if its operand is trivial except co_await.
return UO->getOpcode() != UO_Coawait && Visit(UO->getSubExpr());
@@ -349,12 +355,17 @@ class TrivialFunctionAnalysisVisitor
return false;
const auto &Name = safeGetName(Callee);
+ if (Callee->isInStdNamespace() && (Name == "addressof" ||
+ Name == "forward" || Name == "move"))
+ return true;
+
if (Name == "WTFCrashWithInfo" || Name == "WTFBreakpointTrap" ||
+ Name == "WTFCrashWithSecurityImplication" || Name == "WTFCrash" ||
Name == "WTFReportAssertionFailure" || Name == "isMainThread" ||
Name == "isMainThreadOrGCThread" || Name == "isMainRunLoop" ||
Name == "isWebThread" || Name == "isUIThread" ||
- Name == "compilerFenceForCrash" || Name == "bitwise_cast" ||
- Name == "addressof" || Name.find("__builtin") == 0)
+ Name == "mayBeGCThread" || Name == "compilerFenceForCrash" ||
+ Name == "bitwise_cast" || Name.find("__builtin") == 0)
return true;
return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
@@ -445,6 +456,10 @@ class TrivialFunctionAnalysisVisitor
return Visit(VMT->getSubExpr());
}
+ bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr* BTE) {
+ return Visit(BTE->getSubExpr());
+ }
+
bool VisitExprWithCleanups(const ExprWithCleanups *EWC) {
return Visit(EWC->getSubExpr());
}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index 073f3252160ee..a6589f6fad14b 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -7,6 +7,9 @@ void WTFBreakpointTrap();
void WTFCrashWithInfo(int, const char*, const char*, int);
void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion);
+void WTFCrash(void);
+void WTFCrashWithSecurityImplication(void);
+
inline void compilerFenceForCrash()
{
asm volatile("" ::: "memory");
@@ -62,14 +65,25 @@ void WTFCrashWithInfo(int line, const char* file, const char* function, int coun
template<typename ToType, typename FromType>
ToType bitwise_cast(FromType from);
+namespace std {
+
template<typename T>
T* addressof(T& arg);
+template<typename T>
+T&& forward(T& arg);
+
+template<typename T>
+T&& move( T&& t );
+
+} // namespace std
+
bool isMainThread();
bool isMainThreadOrGCThread();
bool isMainRunLoop();
bool isWebThread();
bool isUIThread();
+bool mayBeGCThread();
enum class Flags : unsigned short {
Flag1 = 1 << 0,
@@ -161,16 +175,30 @@ class Number {
class ComplexNumber {
public:
- ComplexNumber() : real(0), complex(0) { }
+ ComplexNumber() : realPart(0), complexPart(0) { }
ComplexNumber(const ComplexNumber&);
- ComplexNumber& operator++() { real.someMethod(); return *this; }
+ ComplexNumber& operator++() { realPart.someMethod(); return *this; }
ComplexNumber operator++(int);
ComplexNumber& operator<<(int);
ComplexNumber& operator+();
+ const Number& real() const { return realPart; }
+
private:
- Number real;
- Number complex;
+ Number realPart;
+ Number complexPart;
+};
+
+class ObjectWithNonTrivialDestructor {
+public:
+ ObjectWithNonTrivialDestructor() { }
+ ObjectWithNonTrivialDestructor(unsigned v) : v(v) { }
+ ~ObjectWithNonTrivialDestructor() { }
+
+ unsigned value() const { return v; }
+
+private:
+ unsigned v { 0 };
};
class RefCounted {
@@ -248,7 +276,7 @@ class RefCounted {
int trivial40() { return v << 2; }
unsigned trivial41() { v = ++s_v; return v; }
unsigned trivial42() { return bitwise_cast<unsigned long>(nullptr); }
- Number* trivial43() { return addressof(*number); }
+ Number* trivial43() { return std::addressof(*number); }
Number* trivial44() { return new Number(1); }
ComplexNumber* trivial45() { return new ComplexNumber(); }
void trivial46() { ASSERT(isMainThread()); }
@@ -256,6 +284,21 @@ class RefCounted {
void trivial48() { ASSERT(isMainRunLoop()); }
void trivial49() { ASSERT(isWebThread()); }
void trivial50() { ASSERT(isUIThread()); }
+ void trivial51() { ASSERT(mayBeGCThread()); }
+ void trivial52() { WTFCrash(); }
+ void trivial53() { WTFCrashWithSecurityImplication(); }
+ unsigned trivial54() { return ComplexNumber().real().value(); }
+ Number&& trivial55() { return std::forward(*number); }
+ unsigned trivial56() { Number n { 5 }; return std::move(n).value(); }
+ void trivial57() { do { break; } while (1); }
+ void trivial58() { do { continue; } while (0); }
+ void trivial59() {
+ do { goto label; }
+ while (0);
+ label:
+ return;
+ }
+ unsigned trivial60() { return ObjectWithNonTrivialDestructor { 5 }.value(); }
static RefCounted& singleton() {
static RefCounted s_RefCounted;
@@ -413,6 +456,16 @@ class UnrelatedClass {
getFieldTrivial().trivial48(); // no-warning
getFieldTrivial().trivial49(); // no-warning
getFieldTrivial().trivial50(); // no-warning
+ getFieldTrivial().trivial51(); // no-warning
+ getFieldTrivial().trivial52(); // no-warning
+ getFieldTrivial().trivial53(); // no-warning
+ getFieldTrivial().trivial54(); // no-warning
+ getFieldTrivial().trivial55(); // no-warning
+ getFieldTrivial().trivial56(); // no-warning
+ getFieldTrivial().trivial57(); // no-warning
+ getFieldTrivial().trivial58(); // no-warning
+ getFieldTrivial().trivial59(); // no-warning
+ getFieldTrivial().trivial60(); // no-warning
RefCounted::singleton().trivial18(); // no-warning
RefCounted::singleton().someFunction(); // no-warning
|
rniwa
force-pushed
the
treat-break-continue-goto-label-stmt-as-trivial
branch
from
5a5b65b
to
433852b
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -445,6 +456,10 @@ class TrivialFunctionAnalysisVisitor | |||
| return Visit(VMT->getSubExpr()); | |||
| } | |||
|
|
|||
| bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr* BTE) { | |||
| return Visit(BTE->getSubExpr()); | |||
There was a problem hiding this comment.
At this point you probably want to double-check that the destructor itself is trivial (in the sense of your analysis, not in the general C++ sense). Destructor calls are generally not represented in the AST at all (unless they're explicit), CodeGen just figures them out from the rest of the syntax.
CXXBindTemporaryExpr is probably the right place to look for destructors, at least according to me after reading a lot of ASTs. But at the same time nobody really understands what CXXBindTemporaryExpr stands for anymore and a few famous people have argued for removing it.
Note that CXXBindTemporaryExpr doesn't guarantee that there will be a destructor call at the end of the full-expression (lifetime extension is a thing - need to consult MaterializeTemporaryExpr to see where this is going), or even at the end of function (RVO is a thing), even when the constructor is targeting a local variable (NRVO is a thing). In case of RVO/NRVO the caller doesn't necessarily call the destructor either; it could also be RVOing/NRVOing it further up the call stack up to arbitrarily large depth.
In pre-C++17 AST, and even in C++17 and later if NRVO is involved, the AST would look as if a copy/move is being made even if it's elided in practice.
So when checking the destructor, you need to think how far do you want to go when it comes to determining if the destructor actually happens by the time the function returns. Because in order to implement this accurately you'll need to re-implement a big chunk of CodeGen.
You can also try to check the destructor inside VisitCXXConstructExpr(), as if assuming that every time an object is constructed, it'd be deleted "eventually". Except in this case you'll miss the part where you're receiving an object by value from a CallExpr (and such); then you'll also need to check the destructor for the received object even though you don't see the constructor. This isn't a problem when you're relying on CXXBindTemporaryExpr which will be present around the CallExpr normally when the object needs a destructor.
There was a problem hiding this comment.
Oh I see. Will add a check.
There was a problem hiding this comment.
Added (with a test case).
There was a problem hiding this comment.
Which reminds me, I think we're also forgetting to check CXXCtorInitializers. Because they aren't included in CXXConstructorDecl->getBody(). They also may be non-trivial.
Also need to double-check default function argument expressions. We probably don't need to handle them when we're jumping into a function from a call site. But if we're ever asking the analysis whether a function is safe outside of any caller context, we should probably traverse them manually. But I don't think we're actually asking that.
There was a problem hiding this comment.
Oh! I guess we need to do that in TrivialFunctionAnalysis::isTrivialImpl? Will follow up.
| @@ -445,6 +456,10 @@ class TrivialFunctionAnalysisVisitor | |||
| return Visit(VMT->getSubExpr()); | |||
| } | |||
|
|
|||
| bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr* BTE) { | |||
| return Visit(BTE->getSubExpr()); | |||
There was a problem hiding this comment.
Which reminds me, I think we're also forgetting to check CXXCtorInitializers. Because they aren't included in CXXConstructorDecl->getBody(). They also may be non-trivial.
Also need to double-check default function argument expressions. We probably don't need to handle them when we're jumping into a function from a call site. But if we're ever asking the analysis whether a function is safe outside of any caller context, we should probably traverse them manually. But I don't think we're actually asking that.
|
Thanks for the review! |
…al in WebKit checkers. (llvm#91873) Also allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list.
…al in WebKit checkers. (llvm#91873) Also allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list.
…al in WebKit checkers. (llvm#91873) Also allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list.
…al in WebKit checkers. (llvm#91873) Also allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list.
Also allow CXXBindTemporaryExpr, which creates a temporary object with a non-trivial destructor, and add a few more std and WTF functions to the explicitly allowed list.