mwyman updated this revision to Diff 256209.
mwyman marked an inline comment as done.
mwyman added a comment.
Updating per Stephane's comments. To deal with struct fields needed to match
MemberRefExprs.
However, in doing so I discovered that the match conditions I thought were
catching ObjcIvarRefExpr or MemberRefExpr were sometimes incorrectly matching
on the self reference, which is itself a DeclRefExpr to an object with strong
lifetime semantics. This meant correct usages would sometimes be caught,
because the ObjcIvarRefExpr wouldn't match but the DeclRefExpr (to self) did.
That self (or other object) that is dereferenced (implicity or explicitly) has
a parent ImplicitCastExpr in the AST, so I've dealt with this situation by
excluding that case from matching the declRefExpr.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77571/new/
https://reviews.llvm.org/D77571
Files:
clang-tools-extra/clang-tidy/objc/CMakeLists.txt
clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp
clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.h
clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst
clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
Index: clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy %s objc-nsinvocation-argument-lifetime %t
+
+__attribute__((objc_root_class))
+@interface NSObject
+@end
+
+@interface NSInvocation : NSObject
+- (void)getArgument:(void *)Arg atIndex:(int)Index;
+- (void)getReturnValue:(void *)ReturnValue;
+@end
+
+@interface OtherClass : NSObject
+- (void)getArgument:(void *)Arg atIndex:(int)Index;
+@end
+
+void foo(NSInvocation *Invocation) {
+ __unsafe_unretained id Arg2;
+ id Arg3;
+ // CHECK-FIXES: __unsafe_unretained id Arg3;
+ NSObject __strong *Arg4;
+ // CHECK-FIXES: NSObject __unsafe_unretained *Arg4;
+ __weak id Arg5;
+ // CHECK-FIXES: __unsafe_unretained id Arg5;
+ id ReturnValue;
+ // CHECK-FIXES: __unsafe_unretained id ReturnValue;
+ void (^BlockArg1)();
+ // CHECK-FIXES: __unsafe_unretained void (^BlockArg1)();
+ __unsafe_unretained void (^BlockArg2)();
+
+ [Invocation getArgument:&Arg2 atIndex:2];
+
+ [Invocation getArgument:&Arg3 atIndex:3];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:&Arg4 atIndex:4];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:&Arg5 atIndex:5];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:&BlockArg1 atIndex:6];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:&BlockArg2 atIndex:6];
+
+ [Invocation getReturnValue:&ReturnValue];
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: NSInvocation '-getReturnValue:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:(void *)0 atIndex:6];
+}
+
+void bar(OtherClass *OC) {
+ id Arg;
+ [OC getArgument:&Arg atIndex:2];
+}
+
+struct Foo {
+ __unsafe_unretained id Field;
+};
+
+@interface TestClass : NSObject {
+@public
+ id Argument1;
+ __unsafe_unretained id Argument2;
+ struct Foo Bar;
+}
+@end
+
+@implementation TestClass
+
+- (void)processInvocation:(NSInvocation *)Invocation {
+ [Invocation getArgument:&Argument1 atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+ [Invocation getArgument:&self->Argument1 atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+ [Invocation getArgument:&Argument2 atIndex:2];
+ [Invocation getArgument:&self->Argument2 atIndex:2];
+
+ [Invocation getReturnValue:&(self->Bar.Field)];
+}
+
+@end
+
+void baz(NSInvocation *Invocation, TestClass *Obj) {
+ [Invocation getArgument:&Obj->Argument1 atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+ [Invocation getArgument:&Obj->Argument2 atIndex:2];
+}
Index: clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst
@@ -0,0 +1,39 @@
+.. title:: clang-tidy - objc-nsinvocation-argument-lifetime
+
+objc-nsinvocation-argument-lifetime
+===================================
+
+Finds calls to ``NSInvocation`` methods under ARC that don't have proper
+argument object lifetimes. When passing Objective-C objects as parameters
+to the ``NSInvocation`` methods ``getArgument:atIndex:`` and
+``getReturnValue:``, the values are copied by value into the argument pointer,
+which leads to to incorrect releasing behavior if the object pointers are
+not declared ``__unsafe_unretained``.
+
+For code:
+
+.. code-block:: objc
+
+ id arg;
+ [invocation getArgument:&arg atIndex:2];
+
+ __strong id returnValue;
+ [invocation getReturnValue:&returnValue];
+
+The fix will be:
+
+.. code-block:: objc
+
+ __unsafe_unretained id arg;
+ [invocation getArgument:&arg atIndex:2];
+
+ __unsafe_unretained id returnValue;
+ [invocation getReturnValue:&returnValue];
+
+The check will warn on being passed instance variable references that have
+lifetimes other than ``__unsafe_unretained``, but does not propose a fix:
+
+.. code-block:: objc
+
+ // "id _returnValue" is declaration of instance variable of class.
+ [invocation getReturnValue:&self->_returnValue];
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -240,6 +240,7 @@
`objc-dealloc-in-category <objc-dealloc-in-category.html>`_,
`objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_,
`objc-missing-hash <objc-missing-hash.html>`_,
+ `objc-nsinvocation-argument-lifetime <objc-nsinvocation-argument-lifetime.html>`_, "Yes"
`objc-property-declaration <objc-property-declaration.html>`_, "Yes"
`objc-super-self <objc-super-self.html>`_, "Yes"
`openmp-exception-escape <openmp-exception-escape.html>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -92,7 +92,7 @@
Finds ``cnd_wait``, ``cnd_timedwait``, ``wait``, ``wait_for``, or
``wait_until`` function calls when the function is not invoked from a loop
- that checks whether a condition predicate holds or the function has a
+ that checks whether a condition predicate holds or the function has a
condition parameter.
- New :doc:`bugprone-reserved-identifier
@@ -134,6 +134,12 @@
Finds recursive functions and diagnoses them.
+- New :doc:`objc-nsinvocation-argument-lifetime
+ <clang-tidy/checks/objc-nsinvocation-argument-lifetime>` check.
+
+ Finds calls to ``NSInvocation`` methods under ARC that don't have proper
+ argument object lifetimes.
+
New check aliases
^^^^^^^^^^^^^^^^^
@@ -161,7 +167,7 @@
^^^^^^^^^^^^^^^^^^^^^^^^^^
- Improved :doc:`readability-qualified-auto
- <clang-tidy/checks/readability-qualified-auto>` check now supports a
+ <clang-tidy/checks/readability-qualified-auto>` check now supports a
`AddConstToQualified` to enable adding ``const`` qualifiers to variables
typed with ``auto *`` and ``auto &``.
Index: clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
+++ clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
@@ -13,6 +13,7 @@
#include "DeallocInCategoryCheck.h"
#include "ForbiddenSubclassingCheck.h"
#include "MissingHashCheck.h"
+#include "NSInvocationArgumentLifetimeCheck.h"
#include "PropertyDeclarationCheck.h"
#include "SuperSelfCheck.h"
@@ -33,6 +34,8 @@
"objc-forbidden-subclassing");
CheckFactories.registerCheck<MissingHashCheck>(
"objc-missing-hash");
+ CheckFactories.registerCheck<NSInvocationArgumentLifetimeCheck>(
+ "objc-nsinvocation-argument-lifetime");
CheckFactories.registerCheck<PropertyDeclarationCheck>(
"objc-property-declaration");
CheckFactories.registerCheck<SuperSelfCheck>(
Index: clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.h
@@ -0,0 +1,39 @@
+//===--- NSInvocationArgumentLifetimeCheck.h - clang-tidy -------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Basic/LangStandard.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds calls to NSInvocation methods under ARC that don't have proper
+/// argument object lifetimes.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-nsinvocation-argument-lifetime.html
+class NSInvocationArgumentLifetimeCheck : public ClangTidyCheck {
+public:
+ NSInvocationArgumentLifetimeCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.ObjC && LangOpts.ObjCAutoRefCount;
+ }
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H
Index: clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp
@@ -0,0 +1,145 @@
+//===--- NSInvocationArgumentLifetimeCheck.cpp - clang-tidy ---------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "NSInvocationArgumentLifetimeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attrs.inc"
+#include "clang/AST/ComputeDependence.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprObjC.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+namespace {
+
+static constexpr StringRef WeakText = "__weak";
+static constexpr StringRef StrongText = "__strong";
+static constexpr StringRef UnsafeUnretainedText = "__unsafe_unretained";
+
+/// Matches ObjCIvarRefExpr, DeclRefExpr, or MemberExpr that reference
+/// Objective-C object (or block) variables or fields whose object lifetimes
+/// are not __unsafe_unretained.
+AST_POLYMORPHIC_MATCHER(isObjCManagedLifetime,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(ObjCIvarRefExpr,
+ DeclRefExpr,
+ MemberExpr)) {
+ QualType QT = Node.getType();
+ return (QT->getScalarTypeKind() == Type::STK_ObjCObjectPointer ||
+ QT->getScalarTypeKind() == Type::STK_BlockPointer) &&
+ QT.getQualifiers().getObjCLifetime() > Qualifiers::OCL_ExplicitNone;
+}
+
+static llvm::Optional<FixItHint>
+fixItHintReplacementForOwnershipString(StringRef Text, CharSourceRange Range,
+ StringRef Ownership) {
+ size_t Index = Text.find(Ownership);
+ if (Index == StringRef::npos)
+ return llvm::None;
+
+ SourceLocation Begin = Range.getBegin().getLocWithOffset(Index);
+ SourceLocation End = Begin.getLocWithOffset(Ownership.size());
+ return FixItHint::CreateReplacement(SourceRange(Begin, End),
+ UnsafeUnretainedText);
+}
+
+static llvm::Optional<FixItHint>
+fixItHintForVarDecl(const VarDecl *VD, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ assert(VD && "VarDecl parameter must not be null");
+ // Don't provide fix-its for any parameter variables at this time.
+ if (isa<ParmVarDecl>(VD))
+ return llvm::None;
+
+ // Currently there is no way to directly get the source range for the
+ // __weak/__strong ObjC lifetime qualifiers, so it's necessary to string
+ // search in the source code.
+ CharSourceRange Range = Lexer::makeFileCharRange(
+ CharSourceRange::getTokenRange(VD->getSourceRange()), SM, LangOpts);
+ if (Range.isInvalid()) {
+ // An invalid range likely means inside a macro, in which case don't supply
+ // a fix-it.
+ return llvm::None;
+ }
+
+ StringRef VarDeclText = Lexer::getSourceText(Range, SM, LangOpts);
+ if (llvm::Optional<FixItHint> Hint =
+ fixItHintReplacementForOwnershipString(VarDeclText, Range, WeakText))
+ return Hint;
+
+ if (llvm::Optional<FixItHint> Hint = fixItHintReplacementForOwnershipString(
+ VarDeclText, Range, StrongText))
+ return Hint;
+
+ return FixItHint::CreateInsertion(Range.getBegin(), "__unsafe_unretained ");
+}
+
+} // namespace
+
+void NSInvocationArgumentLifetimeCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ objcMessageExpr(
+ hasReceiverType(asString("NSInvocation *")),
+ anyOf(hasSelector("getArgument:atIndex:"),
+ hasSelector("getReturnValue:")),
+ hasArgument(
+ 0, anyOf(hasDescendant(memberExpr(isObjCManagedLifetime())),
+ hasDescendant(objcIvarRefExpr(isObjCManagedLifetime())),
+ hasDescendant(
+ // Reference to variables, but when dereferencing
+ // to ivars/fields a more-descendent variable
+ // reference (e.g. self) may match with strong
+ // object lifetime, leading to an incorrect match.
+ // Exclude these conditions.
+ declRefExpr(to(varDecl().bind("var")),
+ unless(hasParent(implicitCastExpr())),
+ isObjCManagedLifetime())))))
+ .bind("call"),
+ this);
+}
+
+void NSInvocationArgumentLifetimeCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *MatchedExpr = Result.Nodes.getNodeAs<ObjCMessageExpr>("call");
+
+ auto Diag = diag(MatchedExpr->getArg(0)->getBeginLoc(),
+ "NSInvocation %objcinstance0 should only pass pointers to "
+ "objects with ownership __unsafe_unretained")
+ << MatchedExpr->getSelector();
+
+ // Only provide fix-it hints for references to local variables; fixes for
+ // instance variable references don't have as clear an automated fix.
+ const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var");
+ if (!VD)
+ return;
+
+ if (auto Hint = fixItHintForVarDecl(VD, *Result.SourceManager,
+ Result.Context->getLangOpts()))
+ Diag << *Hint;
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/objc/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/objc/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/objc/CMakeLists.txt
@@ -8,6 +8,7 @@
DeallocInCategoryCheck.cpp
ForbiddenSubclassingCheck.cpp
MissingHashCheck.cpp
+ NSInvocationArgumentLifetimeCheck.cpp
ObjCTidyModule.cpp
PropertyDeclarationCheck.cpp
SuperSelfCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
