Merge Webkit checker fixes #10038
Merged
Merge Webkit checker fixes #10038
Conversation
This file contains 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
…te warnings (llvm#107676) This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to an "opaque" function; i.e. a function without definition / body.
…on. (llvm#108167) Treat WTFReportBacktrace, which prints out the backtrace, as trivial.
…8257) We should allow singleton and fooSingleton as singleton function names.
…Objective-C++ (llvm#108184) This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe.
… property access (llvm#108669) Treat a function call or property access via a Objective-C++ selector which returns a Ref/RefPtr as safe.
… warnings (llvm#108656) Improve the fix in 203a2ca by allowing variable references and more ignoring of parentheses.
This PR fixes a bug in UncountedCallArgsChecker that calling a function with a member variable which is Ref/RefPtr is erroneously treated as safe by canoniclizing the type before checking whether it's ref counted or not.
Set DeclWithIssue in alpha.webkit.UncountedCallArgsChecker and alpha.webkit.UncountedLocalVarsChecker.
…checker for CheckedPtr/CheckedRef (llvm#108352) This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr.
…dPtrOrigin. (llvm#111222) Ignore std::forward when it appears while looking for the pointer origin.
…XXInheritedCtorInitExpr. (llvm#111198)
Make isUncountedPtr take QualType as an argument instead of Type*. This simplifies some code.
…neously treated as non-trivial (llvm#110973) This PR fixes the bug that alpha.webkit.UncountedLocalVarsChecker erroneously treats a trivial recursive function as non-trivial. This was caused by TrivialFunctionAnalysis::isTrivialImpl which takes a statement as an argument populating the cache with "false" while traversing the statement to determine its triviality within a recursive function in TrivialFunctionAnalysisVisitor's WithCachedResult. Because IsFunctionTrivial honors an entry in the cache, this resulted in the whole function to be treated as non-trivial. Thankfully, TrivialFunctionAnalysisVisitor::IsFunctionTrivial already handles recursive functions correctly so this PR applies the same logic to TrivialFunctionAnalysisVisitor::WithCachedResult by sharing code between the two functions. This avoids the cache to be pre-populated with "false" while traversing statements in a recurisve function.
This PR makes WebKit checkers allow a guardian variable which is CheckedPtr or CheckedRef as in addition to RefPtr or Ref.
…r/reference when the guardian variable gets mutated. (llvm#113859) This checker has a notion of a guardian variable which is a variable and keeps the object pointed to by a raw pointer / reference in an inner scope alive long enough to "guard" it from use-after-free. But such a guardian variable fails to flawed to keep the object alive if it ever gets mutated within the scope of a raw pointer / reference. This PR fixes this bug by introducing a new AST visitor class, GuardianVisitor, which traverses the compound statements of a guarded variable (raw pointer / reference) and looks for any operator=, move constructor, or calls to "swap", "leakRef", or "releaseNonNull" functions.
…13708) This PR introduces alpha.webkit.UncheckedLocalVarsChecker which detects a raw reference or a raw pointer local, static, or global variable to a CheckedPtr capable object without a guardian variable in an outer scope.
…keCheckedPtr. (llvm#114636) The member functions that define CheckedPtr capable type is incrementCheckedPtrCount and decrementCheckedPtrCount after the rename.
…13708) (llvm#114522) This PR introduces alpha.webkit.UncheckedCallArgsChecker which detects a function argument which is a raw reference or a raw pointer to a CheckedPtr capable object.
…[[clang::noescape]]. (llvm#114897) This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration. In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests.
…operator[] as trivial (llvm#113377) TFA wasn't recognizing `__libcpp_verbose_abort` as trivial causing `std::array::operator[]` also not being recognized as trivial.
…al (llvm#115695) Treat member function calls to ref() and incrementCheckedPtrCount() as trivial so that a function which returns RefPtr/Ref out of a raw reference / pointer is also considered trivial.
…#115594) Treat const Ref, RefPtr, CheckedRef, CheckedPtr member variables as safe pointer origin in WebKit's local variable and call arguments checkers.
…lvm#117090) Only call getThisType() on an instance method.
… arguments (llvm#117394) Fixed a bug that UncountedLambdaCapturesChecker would emit a warning on a lambda capture when the lambda is invoked with arguments. LocalVisitor::VisitCallExpr was not tolerating a lambda invocation with more than 1 arguments.
Add `ObjCInterfaceDecl` to the list of types supported by the `hasType` and `hasDeclaration` matchers, `ObjCObjectPointerType` to the list of types supported by `pointee`. These AST matcher improvements will help the new WebKit checker for unsafe casts ([https://github.com/llvm/llvm-project/pull/114606](https://github.com/llvm/llvm-project/pull/114606)) match on unsafe Objective-C pointer casts.
…lvm#114606) This PR introduces a new checker `[alpha.webkit.MemoryUnsafeCastChecker]` that warns all downcasts from a base type to a derived type. rdar://137766829
…ize signletons. (llvm#119339) It's safe to have a raw pointer or a raw reference to a singleton object. Explicitly allow this in UncountedLocalVarsChecker and UncheckedLocalVarsChecker.
…in (llvm#119336) Prior to this PR, only CXXTemporaryObjectExpr, not CXXConstructExpr was recognized in tryToFindPtrOrigin.
In WebKit, we often write Foo::ensureBar function which lazily initializes m_bar and returns a raw pointer or a raw reference to m_bar. Such a return value is safe to use for the duration of a member function call in Foo so long as m_bar is const so that it never gets unset or updated with a new value once it's initialized. This PR adds support for recognizing these types of functions and treating its return value as a safe origin of a function argument (including "this") or a local variable.
adoptRef in WebKit constructs Ref/RefPtr so treat it as such in isCtorOfRefCounted. Also removed the support for makeRef and makeRefPtr as they don't exist any more.
…llvm#120528) In WebKit, we often capture this as Ref or RefPtr in addition to this itself so that the object lives as long as a capturing lambda stays alive. Detect this pattern and treat it as safe. This PR also makes the check for a lambda being passed as a function argument more robust by handling CXXBindTemporaryExpr, CXXConstructExpr, and DeclRefExpr referring to the lambda.
…m#120702) Added a nullptr check.
…lvm#120810) This fixes a bug where report links generated from files such as StylePrimitiveNumericTypes+Conversions.h in WebKit result in an error. --------- Co-authored-by: Brianna Fan <[email protected]>
…dn't take the object pointer into account. (llvm#125662) When a callee is a method call (e.g. calling a lambda), we need to skip the object pointer to match the parameter list with the call arguments. This manifests as a bug that the checker erroneously generate a warning for a lambda capture (L1) which is passed to a no-escape argument of another lambda (L2).
…ape]] on a member function no longer works. (llvm#126016)
…lvm#126203) Implicit value initialization is trivial for our purposes.
…#126353) Like const C++ member variables, treat const Ref, RefPtr, CheckedRef, CheckedPtr Objective-C ivars as a safe pointer origin in WebKit checkers.
…m#126470) Allow operator T&() in a member function which returns a const member variable. In particular, this will allow UniqueRef::operator T&() and Ref::operator T&() to be treated as a safe pointer origin when they're called on a const member.
…n a constructor (llvm#126869) Added the support for annotating a constructor's argument with [[clang::noescape]]. We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning.
…paqueValueExpr in trivial expressions (llvm#127182) Allow VisitArrayInitLoopExpr, VisitArrayInitIndexExpr, and VisitOpaqueValueExpr in trivial functions and statements.
…s pattern (llvm#126443) In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions.
For the purpose of determining triviality, ignore all attributes.
…is (llvm#127309) Add a missing nullptr check to declProtectsThis.
…ariable checkers. (llvm#127570) Like a C++ member variable, every Objective-C++ instance variable must be a RefPtr, Ref CheckedPtr, or CheckedRef to an object, not a raw pointer or reference.
|
@swift-ci please test |
devincoughlin
approved these changes
Feb 25, 2025
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.