[alpha.webkit.UncountedCallArgsChecker] Allow ASSERT and atomic<T> operations in a trivial function #82063
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesFull diff: https://github.com/llvm/llvm-project/pull/82063.diff 6 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 94eaa81af51772..1a9d6d3127fb7f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -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 =
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 6f236db0474079..c713293924c45e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -315,6 +315,10 @@ 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;
@@ -330,12 +334,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;
@@ -356,6 +366,14 @@ class TrivialFunctionAnalysisVisitor
return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache);
}
+ bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr* E) {
+ 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))
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 17a64e1b1b8e04..1bed21d18ed74d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -53,6 +53,15 @@ class UncountedCallArgsChecker
bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return false; }
+ bool VisitCXXMethodDecl(const CXXMethodDecl *D) {
+ if (auto* Class = D->getParent()) {
+ auto name = safeGetName(Class);
+ if (isRefCounted(Class))
+ return false; // Don't visit contents of Ref/RefPtr methods.
+ }
+ return true;
+ }
+
bool VisitCallExpr(const CallExpr *CE) {
Checker->visitCallExpr(CE);
return true;
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-protected-return-value.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-protected-return-value.cpp
index 1c4b3df211b1e3..6a8b7464845a26 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-protected-return-value.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-protected-return-value.cpp
@@ -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&);
};
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index d08a997aa8c043..82db67bb031dd6 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -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; }
@@ -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; }
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index 957dff74ffce46..22886102578c16 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -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()
{
@@ -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); }
@@ -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));
}
@@ -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;
@@ -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 };
@@ -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
@@ -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}}
}
};
@@ -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 test() {
+ someFunction(*object());
+}
|
rniwa
force-pushed
the
treat-assert-and-atomic-operations-as-trivial
branch
from
ddaf7bf to
69a593f
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
rniwa
force-pushed
the
treat-assert-and-atomic-operations-as-trivial
branch
from
69a593f to
8d30496
Compare
…erations in a trivial function This PR permits the use of WebKit's ASSERT macros as well as std::atomic<T> operations to appear within a trivial function. Also exempt ref() and deref() member function calls.
rniwa
force-pushed
the
treat-assert-and-atomic-operations-as-trivial
branch
from
8d30496 to
c46f11b
Compare
haoNoQ
approved these changes
Feb 19, 2024
| @@ -356,6 +364,14 @@ class TrivialFunctionAnalysisVisitor | |||
| return TrivialFunctionAnalysis::isTrivialImpl(Callee, Cache); | |||
| } | |||
|
|
|||
| bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, I'm adding more general caching mechanism for Stmt in #82229.
This was referenced Mar 7, 2024
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Mar 8, 2024
…erations in a trivial function (llvm#82063)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.