Skip to content

[analyzer] Treat break, continue, goto, and label statements as trivial in WebKit checkers. #91873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 15, 2024
Merged

[analyzer] Treat break, continue, goto, and label statements as trivial in WebKit checkers. #91873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
433852b
[analyzer] Treat break, continue, goto, and label statements as trivi…
May 11, 2024
8c387f1
Fix formatting.
May 11, 2024
7812c33
Check triviality of destructor in CXXBindTemporaryExpr.
May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -445,6 +456,14 @@ class TrivialFunctionAnalysisVisitor
return Visit(VMT->getSubExpr());
}

bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE) {
if (auto *Temp = BTE->getTemporary()) {
if (!TrivialFunctionAnalysis::isTrivialImpl(Temp->getDestructor(), Cache))
return false;
}
return Visit(BTE->getSubExpr());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Will add a check.

Copy link
Contributor Author

@rniwa rniwa May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added (with a test case).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I guess we need to do that in TrivialFunctionAnalysis::isTrivialImpl? Will follow up.

}

bool VisitExprWithCleanups(const ExprWithCleanups *EWC) {
return Visit(EWC->getSubExpr());
}
Expand Down
78 changes: 73 additions & 5 deletions clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -161,16 +175,42 @@ 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 ObjectWithMutatingDestructor {
public:
ObjectWithMutatingDestructor() : n(0) { }
ObjectWithMutatingDestructor(int n) : n(n) { }
~ObjectWithMutatingDestructor() { n.someMethod(); }

unsigned value() const { return n.value(); }

private:
Number n;
};

class RefCounted {
Expand Down Expand Up @@ -248,14 +288,29 @@ 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()); }
void trivial47() { ASSERT(isMainThreadOrGCThread()); }
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;
Expand Down Expand Up @@ -335,6 +390,7 @@ class RefCounted {
ComplexNumber nonTrivial17() { return complex << 2; }
ComplexNumber nonTrivial18() { return +complex; }
ComplexNumber* nonTrivial19() { return new ComplexNumber(complex); }
unsigned nonTrivial20() { return ObjectWithMutatingDestructor { 7 }.value(); }

static unsigned s_v;
unsigned v { 0 };
Expand Down Expand Up @@ -413,6 +469,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
Expand Down Expand Up @@ -457,6 +523,8 @@ class UnrelatedClass {
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial19();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial20();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
}
};

Expand Down
Loading