Skip to content

[alpha.webkit.UncountedCallArgsChecker] Allow ASSERT and atomic<T> operations in a trivial function #82063

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 2 commits into from
Feb 20, 2024
Merged

[alpha.webkit.UncountedCallArgsChecker] Allow ASSERT and atomic<T> operations in a trivial function #82063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c46f11b
[alpha.webkit.UncountedCallArgsChecker] Allow ASSERT and atomic<T> op…
rniwa Feb 16, 2024
18f0910
Merge branch 'main' into treat-assert-and-atomic-operations-as-trivial
rniwa Feb 19, 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
4 changes: 4 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
E = tempExpr->getSubExpr();
continue;
}
if (auto *tempExpr = dyn_cast<CXXBindTemporaryExpr>(E)) {
E = tempExpr->getSubExpr();
continue;
}
if (auto *cast = dyn_cast<CastExpr>(E)) {
if (StopAtFirstRefCountedObj) {
if (auto *ConversionFunc =
Expand Down
16 changes: 16 additions & 0 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 @@ -315,6 +315,8 @@ class TrivialFunctionAnalysisVisitor
return false;
}

bool VisitAtomicExpr(const AtomicExpr *E) { return VisitChildren(E); }

bool VisitStaticAssertDecl(const StaticAssertDecl *SAD) {
// Any static_assert is considered trivial.
return true;
Expand All @@ -330,12 +332,18 @@ class TrivialFunctionAnalysisVisitor
const auto &Name = safeGetName(Callee);

if (Name == "WTFCrashWithInfo" || Name == "WTFBreakpointTrap" ||
Name == "WTFReportAssertionFailure" ||
Name == "compilerFenceForCrash" || Name == "__builtin_unreachable")
return true;

return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
}

bool VisitPredefinedExpr(const PredefinedExpr *E) {
// A predefined identifier such as "func" is considered trivial.
return true;
}

bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
if (!checkArguments(MCE))
return false;
Expand All @@ -356,6 +364,14 @@ class TrivialFunctionAnalysisVisitor
return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
}

bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In theory this may need to be cached because it may cause default argument expressions to be visited multiple times, i.e. every time a function is called with the given argument omitted. And these expressions can get quite complicated.

I haven't ever seen it matter though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm adding more general caching mechanism for Stmt in #82229.

if (auto *Expr = E->getExpr()) {
if (!Visit(Expr))
return false;
}
return true;
}

bool checkArguments(const CallExpr *CE) {
for (const Expr *Arg : CE->arguments()) {
if (Arg && !Visit(Arg))
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ class UncountedCallArgsChecker
unsigned ArgIdx = isa<CXXOperatorCallExpr>(CE) && isa_and_nonnull<CXXMethodDecl>(F);

if (auto *MemberCallExpr = dyn_cast<CXXMemberCallExpr>(CE)) {
if (auto *MD = MemberCallExpr->getMethodDecl()) {
auto name = safeGetName(MD);
if (name == "ref" || name == "deref")
return;
}
auto *E = MemberCallExpr->getImplicitObjectArgument();
QualType ArgType = MemberCallExpr->getObjectType();
std::optional<bool> IsUncounted =
Expand Down
6 changes: 4 additions & 2 deletions clang/test/Analysis/Checkers/WebKit/call-args-protected-return-value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@

class RefCounted {
public:
void ref();
void deref();
void ref() const;
void deref() const;
};

class Object {
public:
void ref() const;
void deref() const;
void someFunction(RefCounted&);
};

Expand Down
20 changes: 18 additions & 2 deletions clang/test/Analysis/Checkers/WebKit/mock-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@ template <typename T> struct Ref {
T *t;

Ref() : t{} {};
Ref(T *) {}
Ref(T &t)
: t(t) {
if (t)
t->ref();
}
~Ref() {
if (t)
t->deref();
}
T *get() { return t; }
T *ptr() { return t; }
operator const T &() const { return *t; }
Expand All @@ -16,7 +24,15 @@ template <typename T> struct RefPtr {
T *t;

RefPtr() : t(new T) {}
RefPtr(T *t) : t(t) {}
RefPtr(T *t)
: t(t) {
if (t)
t->ref();
}
~RefPtr() {
if (t)
t->deref();
}
T *get() { return t; }
T *operator->() { return t; }
const T *operator->() const { return t; }
Expand Down
39 changes: 28 additions & 11 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 @@ -4,6 +4,7 @@

void WTFBreakpointTrap();
void WTFCrashWithInfo(int, const char*, const char*, int);
void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion);

inline void compilerFenceForCrash()
{
Expand Down Expand Up @@ -31,21 +32,19 @@ void isIntegralOrPointerType(T, Types... types)
CRASH_WITH_INFO(__VA_ARGS__); \
} while (0)

#if !defined(NOT_TAIL_CALLED)
#if __has_attribute(not_tail_called)
#define NOT_TAIL_CALLED __attribute__((not_tail_called))
#else
#define NOT_TAIL_CALLED
#endif
#endif
#define NO_RETURN_DUE_TO_CRASH
#define ASSERT(assertion, ...) do { \
if (!(assertion)) { \
WTFReportAssertionFailure(__FILE__, __LINE__, __PRETTY_FUNCTION__, #assertion); \
CRASH_WITH_INFO(__VA_ARGS__); \
} \
} while (0)

#if !defined(ALWAYS_INLINE)
#define ALWAYS_INLINE inline
#endif

NO_RETURN_DUE_TO_CRASH NOT_TAIL_CALLED void WTFCrashWithInfoImpl(int line, const char* file, const char* function, int counter, unsigned long reason);
NO_RETURN_DUE_TO_CRASH NOT_TAIL_CALLED void WTFCrashWithInfo(int line, const char* file, const char* function, int counter);
void WTFCrashWithInfoImpl(int line, const char* file, const char* function, int counter, unsigned long reason);
void WTFCrashWithInfo(int line, const char* file, const char* function, int counter);

template<typename T>
ALWAYS_INLINE unsigned long wtfCrashArg(T* arg) { return reinterpret_cast<unsigned long>(arg); }
Expand All @@ -54,7 +53,7 @@ template<typename T>
ALWAYS_INLINE unsigned long wtfCrashArg(T arg) { return arg; }

template<typename T>
NO_RETURN_DUE_TO_CRASH ALWAYS_INLINE void WTFCrashWithInfo(int line, const char* file, const char* function, int counter, T reason)
void WTFCrashWithInfo(int line, const char* file, const char* function, int counter, T reason)
{
WTFCrashWithInfoImpl(line, file, function, counter, wtfCrashArg(reason));
}
Expand Down Expand Up @@ -198,6 +197,8 @@ class RefCounted {
bool trivial22() { return enumValue == Enum::Value1; }

bool trivial23() const { return OptionSet<Flags>::fromRaw(v).contains(Flags::Flag1); }
int trivial24() const { ASSERT(v); return v; }
unsigned trivial25() const { return __c11_atomic_load((volatile _Atomic(unsigned) *)&v, __ATOMIC_RELAXED); }

static RefCounted& singleton() {
static RefCounted s_RefCounted;
Expand Down Expand Up @@ -256,6 +257,11 @@ class RefCounted {
}
}

static unsigned* another();
unsigned nonTrivial10() const {
return __c11_atomic_load((volatile _Atomic(unsigned) *)another(), __ATOMIC_RELAXED);
}

unsigned v { 0 };
Number* number { nullptr };
Enum enumValue { Enum::Value1 };
Expand Down Expand Up @@ -301,6 +307,8 @@ class UnrelatedClass {
getFieldTrivial().trivial21(); // no-warning
getFieldTrivial().trivial22(); // no-warning
getFieldTrivial().trivial23(); // no-warning
getFieldTrivial().trivial24(); // no-warning
getFieldTrivial().trivial25(); // no-warning
RefCounted::singleton().trivial18(); // no-warning
RefCounted::singleton().someFunction(); // no-warning

Expand All @@ -324,6 +332,8 @@ class UnrelatedClass {
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial9();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
getFieldTrivial().nonTrivial10();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
}
};

Expand All @@ -342,3 +352,10 @@ class UnrelatedClass2 {
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
}
};

RefPtr<RefCounted> object();
void someFunction(const RefCounted&);

void test2() {
someFunction(*object());
}