[clang] 42f9d0c - [objc_direct] Tigthen checks for direct methods
Pierre Habouzit via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 20 10:57:44 PST 2019
Author: Pierre Habouzit
Date: 2019-12-20T10:57:36-08:00
New Revision: 42f9d0c0bee32a1a48a45c039988d27115f30da9
URL: https://github.com/llvm/llvm-project/commit/42f9d0c0bee32a1a48a45c039988d27115f30da9
DIFF: https://github.com/llvm/llvm-project/commit/42f9d0c0bee32a1a48a45c039988d27115f30da9.diff
LOG: [objc_direct] Tigthen checks for direct methods
Because the name of a direct method must be agreed upon by the caller
and the implementation, certain bad practices that one can get away with
when using dynamism are fatal with direct methods.
To avoid really weird and unscruttable linker error, tighten the
front-end error reporting.
Rule 1:
Direct methods can only have at most one declaration in an @interface
container. Any redeclaration is strictly forbidden.
Today some amount of redeclaration is tolerated between the main
interface and categories for dynamic methods, but we can't have that.
Rule 2:
Direct method implementations can only be declared in a matching
@interface container: when implemented in the primary @implementation
then the declaration must be in the primary @interface or an
extension, and when implemented in a category, the declaration must be
in the @interface for the same category.
Also fix another issue with ObjCMethod::getCanonicalDecl(): when an
implementation lives in the primary @interface, then its canonical
declaration can be in any extension, even when it's not an accessor.
Add Sema tests to cover the new errors, and CG tests to beef up testing
around function names for categories and extensions.
Radar-Id: <rdar://problem/58054563>
Differential Revision: https://reviews.llvm.org/D71694
Added:
clang/test/SemaObjC/method-direct-one-definition.m
Modified:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/AST/DeclObjC.cpp
clang/lib/CodeGen/CGObjCMac.cpp
clang/lib/Sema/SemaDeclObjC.cpp
clang/test/CodeGenObjC/direct-method.m
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 315e836cd397..b714cb81f0ca 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -995,6 +995,12 @@ def warn_objc_boxing_invalid_utf8_string : Warning<
def err_objc_direct_on_protocol : Error<
"'objc_direct' attribute cannot be applied to %select{methods|properties}0 "
"declared in an Objective-C protocol">;
+def err_objc_direct_duplicate_decl : Error<
+ "%select{|direct }0method declaration conflicts "
+ "with previous %select{|direct }1declaration of method %2">;
+def err_objc_direct_impl_decl_mismatch : Error<
+ "direct method was declared in %select{the primary interface|an extension|a category}0 "
+ "but is implemented in %select{the primary interface|a category|a
diff erent category}1">;
def err_objc_direct_missing_on_decl : Error<
"direct method implementation was previously declared not direct">;
def err_objc_direct_on_override : Error<
diff --git a/clang/lib/AST/DeclObjC.cpp b/clang/lib/AST/DeclObjC.cpp
index ca70afd9db25..9a84e3c4a510 100644
--- a/clang/lib/AST/DeclObjC.cpp
+++ b/clang/lib/AST/DeclObjC.cpp
@@ -956,32 +956,32 @@ ObjCMethodDecl *ObjCMethodDecl::getNextRedeclarationImpl() {
ObjCMethodDecl *ObjCMethodDecl::getCanonicalDecl() {
auto *CtxD = cast<Decl>(getDeclContext());
+ const auto &Sel = getSelector();
if (auto *ImplD = dyn_cast<ObjCImplementationDecl>(CtxD)) {
if (ObjCInterfaceDecl *IFD = ImplD->getClassInterface()) {
- if (ObjCMethodDecl *MD = IFD->getMethod(getSelector(),
- isInstanceMethod()))
+ // When the container is the ObjCImplementationDecl (the primary
+ // @implementation), then the canonical Decl is either in
+ // the class Interface, or in any of its extension.
+ //
+ // So when we don't find it in the ObjCInterfaceDecl,
+ // sift through extensions too.
+ if (ObjCMethodDecl *MD = IFD->getMethod(Sel, isInstanceMethod()))
return MD;
- // readwrite properties may have been re-declared in an extension.
- // look harder.
- if (isPropertyAccessor())
- for (auto *Ext : IFD->known_extensions())
- if (ObjCMethodDecl *MD =
- Ext->getMethod(getSelector(), isInstanceMethod()))
- return MD;
+ for (auto *Ext : IFD->known_extensions())
+ if (ObjCMethodDecl *MD = Ext->getMethod(Sel, isInstanceMethod()))
+ return MD;
}
} else if (auto *CImplD = dyn_cast<ObjCCategoryImplDecl>(CtxD)) {
if (ObjCCategoryDecl *CatD = CImplD->getCategoryDecl())
- if (ObjCMethodDecl *MD = CatD->getMethod(getSelector(),
- isInstanceMethod()))
+ if (ObjCMethodDecl *MD = CatD->getMethod(Sel, isInstanceMethod()))
return MD;
}
if (isRedeclaration()) {
// It is possible that we have not done deserializing the ObjCMethod yet.
ObjCMethodDecl *MD =
- cast<ObjCContainerDecl>(CtxD)->getMethod(getSelector(),
- isInstanceMethod());
+ cast<ObjCContainerDecl>(CtxD)->getMethod(Sel, isInstanceMethod());
return MD ? MD : this;
}
diff --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp
index a38e5a49f0f4..e18105fe0193 100644
--- a/clang/lib/CodeGen/CGObjCMac.cpp
+++ b/clang/lib/CodeGen/CGObjCMac.cpp
@@ -928,7 +928,8 @@ class CGObjCCommonMac : public CodeGen::CGObjCRuntime {
/// \param[out] NameOut - The return value.
void GetNameForMethod(const ObjCMethodDecl *OMD,
const ObjCContainerDecl *CD,
- SmallVectorImpl<char> &NameOut);
+ SmallVectorImpl<char> &NameOut,
+ bool ignoreCategoryNamespace = false);
/// GetMethodVarName - Return a unique constant for the given
/// selector's name. The return value has type char *.
@@ -4033,7 +4034,7 @@ CGObjCCommonMac::GenerateDirectMethod(const ObjCMethodDecl *OMD,
return I->second;
SmallString<256> Name;
- GetNameForMethod(OMD, CD, Name);
+ GetNameForMethod(OMD, CD, Name, /*ignoreCategoryNamespace*/true);
CodeGenTypes &Types = CGM.getTypes();
llvm::FunctionType *MethodTy =
@@ -4088,9 +4089,9 @@ void CGObjCCommonMac::GenerateDirectMethodPrologue(
nullptr, true);
Builder.CreateStore(result.getScalarVal(), selfAddr);
- // Nullable `Class` expressions cannot be messaged with a direct method
- // so the only reason why the receive can be null would be because
- // of weak linking.
+ // Nullable `Class` expressions cannot be messaged with a direct method
+ // so the only reason why the receive can be null would be because
+ // of weak linking.
ReceiverCanBeNull = isWeakLinkedClass(OID);
}
@@ -5687,14 +5688,16 @@ CGObjCCommonMac::GetPropertyTypeString(const ObjCPropertyDecl *PD,
void CGObjCCommonMac::GetNameForMethod(const ObjCMethodDecl *D,
const ObjCContainerDecl *CD,
- SmallVectorImpl<char> &Name) {
+ SmallVectorImpl<char> &Name,
+ bool ignoreCategoryNamespace) {
llvm::raw_svector_ostream OS(Name);
assert (CD && "Missing container decl in GetNameForMethod");
OS << '\01' << (D->isInstanceMethod() ? '-' : '+')
<< '[' << CD->getName();
- if (const ObjCCategoryImplDecl *CID =
- dyn_cast<ObjCCategoryImplDecl>(D->getDeclContext()))
- OS << '(' << *CID << ')';
+ if (!ignoreCategoryNamespace)
+ if (const ObjCCategoryImplDecl *CID =
+ dyn_cast<ObjCCategoryImplDecl>(D->getDeclContext()))
+ OS << '(' << *CID << ')';
OS << ' ' << D->getSelector().getAsString() << ']';
}
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
index 70c04571ac67..4fdddfbb7a7e 100644
--- a/clang/lib/Sema/SemaDeclObjC.cpp
+++ b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -4508,12 +4508,6 @@ static void mergeInterfaceMethodToImpl(Sema &S,
method->getLocation()));
}
- if (!method->isDirectMethod())
- if (const auto *attr = prevMethod->getAttr<ObjCDirectAttr>()) {
- method->addAttr(
- ObjCDirectAttr::CreateImplicit(S.Context, attr->getLocation()));
- }
-
// Merge nullability of the result type.
QualType newReturnType
= mergeTypeNullabilityForRedecl(
@@ -4738,16 +4732,73 @@ Decl *Sema::ActOnMethodDeclaration(
}
}
+ // A method is either tagged direct explicitly, or inherits it from its
+ // canonical declaration.
+ //
+ // We have to do the merge upfront and not in mergeInterfaceMethodToImpl()
+ // because IDecl->lookupMethod() returns more possible matches than just
+ // the canonical declaration.
+ if (!ObjCMethod->isDirectMethod()) {
+ const ObjCMethodDecl *CanonicalMD = ObjCMethod->getCanonicalDecl();
+ if (const auto *attr = CanonicalMD->getAttr<ObjCDirectAttr>()) {
+ ObjCMethod->addAttr(
+ ObjCDirectAttr::CreateImplicit(Context, attr->getLocation()));
+ }
+ }
+
// Merge information from the @interface declaration into the
// @implementation.
if (ObjCInterfaceDecl *IDecl = ImpDecl->getClassInterface()) {
if (auto *IMD = IDecl->lookupMethod(ObjCMethod->getSelector(),
ObjCMethod->isInstanceMethod())) {
mergeInterfaceMethodToImpl(*this, ObjCMethod, IMD);
- if (const auto *attr = ObjCMethod->getAttr<ObjCDirectAttr>()) {
- if (!IMD->isDirectMethod()) {
- Diag(attr->getLocation(), diag::err_objc_direct_missing_on_decl);
+
+ // The Idecl->lookupMethod() above will find declarations for ObjCMethod
+ // in one of these places:
+ //
+ // (1) the canonical declaration in an @interface container paired
+ // with the ImplDecl,
+ // (2) non canonical declarations in @interface not paired with the
+ // ImplDecl for the same Class,
+ // (3) any superclass container.
+ //
+ // Direct methods only allow for canonical declarations in the matching
+ // container (case 1).
+ //
+ // Direct methods overriding a superclass declaration (case 3) is
+ // handled during overrides checks in CheckObjCMethodOverrides().
+ //
+ // We deal with same-class container mismatches (Case 2) here.
+ if (IDecl == IMD->getClassInterface()) {
+ auto diagContainerMismatch = [&] {
+ int decl = 0, impl = 0;
+
+ if (auto *Cat = dyn_cast<ObjCCategoryDecl>(IMD->getDeclContext()))
+ decl = Cat->IsClassExtension() ? 1 : 2;
+
+ if (auto *Cat = dyn_cast<ObjCCategoryImplDecl>(ImpDecl))
+ impl = 1 + (decl != 0);
+
+ Diag(ObjCMethod->getLocation(),
+ diag::err_objc_direct_impl_decl_mismatch)
+ << decl << impl;
Diag(IMD->getLocation(), diag::note_previous_declaration);
+ };
+
+ if (const auto *attr = ObjCMethod->getAttr<ObjCDirectAttr>()) {
+ if (ObjCMethod->getCanonicalDecl() != IMD) {
+ diagContainerMismatch();
+ } else if (!IMD->isDirectMethod()) {
+ Diag(attr->getLocation(), diag::err_objc_direct_missing_on_decl);
+ Diag(IMD->getLocation(), diag::note_previous_declaration);
+ }
+ } else if (const auto *attr = IMD->getAttr<ObjCDirectAttr>()) {
+ if (ObjCMethod->getCanonicalDecl() != IMD) {
+ diagContainerMismatch();
+ } else {
+ ObjCMethod->addAttr(
+ ObjCDirectAttr::CreateImplicit(Context, attr->getLocation()));
+ }
}
}
@@ -4779,11 +4830,42 @@ Decl *Sema::ActOnMethodDeclaration(
}
}
} else {
- if (!ObjCMethod->isDirectMethod() &&
- ClassDecl->hasAttr<ObjCDirectMembersAttr>()) {
- ObjCMethod->addAttr(
- ObjCDirectAttr::CreateImplicit(Context, ObjCMethod->getLocation()));
+ if (!isa<ObjCProtocolDecl>(ClassDecl)) {
+ if (!ObjCMethod->isDirectMethod() &&
+ ClassDecl->hasAttr<ObjCDirectMembersAttr>()) {
+ ObjCMethod->addAttr(
+ ObjCDirectAttr::CreateImplicit(Context, ObjCMethod->getLocation()));
+ }
+
+ // There can be a single declaration in any @interface container
+ // for a given direct method, look for clashes as we add them.
+ //
+ // For valid code, we should always know the primary interface
+ // declaration by now, however for invalid code we'll keep parsing
+ // but we won't find the primary interface and IDecl will be nil.
+ ObjCInterfaceDecl *IDecl = dyn_cast<ObjCInterfaceDecl>(ClassDecl);
+ if (!IDecl)
+ IDecl = cast<ObjCCategoryDecl>(ClassDecl)->getClassInterface();
+
+ if (IDecl)
+ if (auto *IMD = IDecl->lookupMethod(ObjCMethod->getSelector(),
+ ObjCMethod->isInstanceMethod(),
+ /*shallowCategoryLookup=*/false,
+ /*followSuper=*/false)) {
+ if (isa<ObjCProtocolDecl>(IMD->getDeclContext())) {
+ // Do not emit a diagnostic for the Protocol case:
+ // diag::err_objc_direct_on_protocol has already been emitted
+ // during parsing for these with a nicer diagnostic.
+ } else if (ObjCMethod->isDirectMethod() || IMD->isDirectMethod()) {
+ Diag(ObjCMethod->getLocation(),
+ diag::err_objc_direct_duplicate_decl)
+ << ObjCMethod->isDirectMethod() << IMD->isDirectMethod()
+ << ObjCMethod->getDeclName();
+ Diag(IMD->getLocation(), diag::note_previous_declaration);
+ }
+ }
}
+
cast<DeclContext>(ClassDecl)->addDecl(ObjCMethod);
}
diff --git a/clang/test/CodeGenObjC/direct-method.m b/clang/test/CodeGenObjC/direct-method.m
index c42910c1676a..e53c99bc0f5e 100644
--- a/clang/test/CodeGenObjC/direct-method.m
+++ b/clang/test/CodeGenObjC/direct-method.m
@@ -172,10 +172,19 @@ @interface Foo : Root {
@interface Foo ()
@property(nonatomic, readwrite) int getDirect_setDynamic;
@property(nonatomic, readwrite, direct) int getDynamic_setDirect;
+- (int)directMethodInExtension __attribute__((objc_direct));
+ at end
+
+ at interface Foo (Cat)
+- (int)directMethodInCategory __attribute__((objc_direct));
@end
__attribute__((objc_direct_members))
@implementation Foo
+// CHECK-LABEL: define hidden i32 @"\01-[Foo directMethodInExtension]"(
+- (int)directMethodInExtension {
+ return 42;
+}
// CHECK-LABEL: define hidden i32 @"\01-[Foo getDirect_setDynamic]"(
// CHECK-LABEL: define internal void @"\01-[Foo setGetDirect_setDynamic:]"(
// CHECK-LABEL: define internal i32 @"\01-[Foo getDynamic_setDirect]"(
@@ -183,6 +192,17 @@ @implementation Foo
// CHECK-LABEL: define internal void @"\01-[Foo .cxx_destruct]"(
@end
+ at implementation Foo (Cat)
+// CHECK-LABEL: define hidden i32 @"\01-[Foo directMethodInCategory]"(
+- (int)directMethodInCategory {
+ return 42;
+}
+// CHECK-LABEL: define hidden i32 @"\01-[Foo directMethodInCategoryNoDecl]"(
+- (int)directMethodInCategoryNoDecl __attribute__((objc_direct)) {
+ return 42;
+}
+ at end
+
int useRoot(Root *r) {
// CHECK-LABEL: define i32 @useRoot
// CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root getInt]"
@@ -195,8 +215,14 @@ int useFoo(Foo *f) {
// CHECK-LABEL: define i32 @useFoo
// CHECK: call void bitcast {{.*}} @"\01-[Foo setGetDynamic_setDirect:]"
// CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo getDirect_setDynamic]"
+ // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInExtension]"
+ // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategory]"
+ // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategoryNoDecl]"
[f setGetDynamic_setDirect:1];
- return [f getDirect_setDynamic];
+ return [f getDirect_setDynamic] +
+ [f directMethodInExtension] +
+ [f directMethodInCategory] +
+ [f directMethodInCategoryNoDecl];
}
__attribute__((objc_root_class))
diff --git a/clang/test/SemaObjC/method-direct-one-definition.m b/clang/test/SemaObjC/method-direct-one-definition.m
new file mode 100644
index 000000000000..e6355d2cb7ba
--- /dev/null
+++ b/clang/test/SemaObjC/method-direct-one-definition.m
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-protocol-method-implementation %s
+
+__attribute__((objc_root_class))
+ at interface A
+ at end
+
+ at interface A (Cat)
+- (void)A_Cat __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+ at end
+
+ at implementation A
+- (void)A_Cat { // expected-error {{direct method was declared in a category but is implemented in the primary interface}}
+}
+ at end
+
+__attribute__((objc_root_class))
+ at interface B
+- (void)B_primary __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+ at end
+
+ at interface B ()
+- (void)B_extension __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+ at end
+
+ at interface B (Cat)
+- (void)B_Cat __attribute__((objc_direct));
+ at end
+
+ at interface B (OtherCat)
+- (void)B_OtherCat __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+ at end
+
+ at implementation B (Cat)
+- (void)B_primary { // expected-error {{direct method was declared in the primary interface but is implemented in a category}}
+}
+- (void)B_extension { // expected-error {{direct method was declared in an extension but is implemented in a
diff erent category}}
+}
+- (void)B_Cat {
+}
+- (void)B_OtherCat { // expected-error {{direct method was declared in a category but is implemented in a
diff erent category}}
+}
+ at end
+
+__attribute__((objc_root_class))
+ at interface C
+- (void)C1 __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+- (void)C2; // expected-note {{previous declaration is here}}
+ at end
+
+ at interface C (Cat)
+- (void)C1; // expected-error {{method declaration conflicts with previous direct declaration of method 'C1'}}
+- (void)C2 __attribute__((objc_direct)); // expected-error {{direct method declaration conflicts with previous declaration of method 'C2'}}
+ at end
More information about the cfe-commits
mailing list