[ObjC] Error out when using forward-declared protocol in a @protocol expression

[arphaman]

Summary

Clang emits invalid protocol metadata when a @protocol expression is used with a forward-declared protocol. The protocol metadata is missing protocol conformance list of the protocol since we don't have access to the definition of it in the compiled translation unit. The linker then might end up picking the invalid metadata when linking which will lead to incorrect runtime protocol conformance checks.

This patch makes sure that Clang fails to compile code that uses a @protocol expression with a forward-declared protocol. This ensures that Clang does not emit invalid protocol metadata. I added an extra assert in CodeGen to ensure that this kind of issue won't happen in other places.

[rjmccall]

Hmm. I think this is a reasonable change to make to the language. Have you investigated to see if this causes source-compatibility problems?

include/clang/Basic/DiagnosticSemaKinds.td

849 ↗

(On Diff #155993)

I think that's the only warning in -Wat-protocol; we can at least anonymize it and leave a comment saying it's now empty.

lib/Sema/SemaExprObjC.cpp

1235 ↗

(On Diff #155993)

Please brace the second clause here. I think even the people who don't generally use braces still advise being consistent within a single if-else chain.

test/CodeGenObjC/forward-declare-protocol-gnu.m

6 ↗

(On Diff #155993)

Does this really not require a definition of P? Ugh. I wonder if that's reasonable to fix, too.

test/Parser/objc-cxx-keyword-identifiers.mm

22 ↗

(On Diff #155993)

Why did this test need to change?

[arphaman]

address review comments.

[arphaman]

Sorry for the delay.

In D49462#1166032, @rjmccall wrote:

Hmm. I think this is a reasonable change to make to the language. Have you investigated to see if this causes source-compatibility problems?

Yes, I tested this change on internal code base. There's an impact, but the impact is fairly minimal. The biggest issue is actually in some Swift overlay: https://github.com/apple/swift/blob/c275826a41aca8268719061636efc12717b8dae1/stdlib/public/SDK/Dispatch/Dispatch.mm#L36

test/CodeGenObjC/forward-declare-protocol-gnu.m

6 ↗

(On Diff #155993)

Nope, we don't emit the protocol metadata for it. It might make sense to require the definition with the implementation?

test/Parser/objc-cxx-keyword-identifiers.mm

22 ↗

(On Diff #155993)

We need to declare delete because it's used in a @protocol expression on line 63.

[rjmccall]

test/CodeGenObjC/forward-declare-protocol-gnu.m

6 ↗

(On Diff #155993)

Yeah, I think so. I would argue that there no places where we should be emitting metadata for an incomplete protocol.

test/Parser/objc-cxx-keyword-identifiers.mm

22 ↗

(On Diff #155993)

Ah, gotcha.

[arphaman]

test/CodeGenObjC/forward-declare-protocol-gnu.m

6 ↗

(On Diff #155993)

Ok, makes sense.
I will fix it in a follow-up patch then. I don't want to block this change as this patch fixes some nasty runtime issues. I will run a separate source compatibility assessment for the follow-up.

[rjmccall]

lib/CodeGen/CGObjCMac.cpp

6788 ↗

(On Diff #161142)

What happens in the @implementation case (the one that we're not diagnosing yet) when the protocol is a forward declaration?

[arphaman]

lib/CodeGen/CGObjCMac.cpp

6788 ↗

(On Diff #161142)

We emit an external global reference to the protocol metadata using GetOrEmitProtocolRef, so this assertion won't be triggered until we force the emission of protocol metadata from implementation as planned in a follow-up patch.

[rjmccall]

LGTM.

lib/CodeGen/CGObjCMac.cpp

6788 ↗

(On Diff #161142)

Okay. I mean, that's also unfortunate behavior, since protocol descriptors are basically linkonce and should be emitted in every translation unit that uses them, but I agree it's less damaging than the behavior for @protocol, and it means this assertion is safe.

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

def ARCRepeatedUseOfWeak : DiagGroup<"arc-repeated-use-of-weak",
                                      [ARCRepeatedUseOfWeakMaybe]>;
 def BlockCaptureAutoReleasing : DiagGroup<"block-capture-autoreleasing">;
 def ObjCBridge : DiagGroup<"bridge-cast">;
 
 def DeallocInCategory:DiagGroup<"dealloc-in-category">;
 def SelectorTypeMismatch : DiagGroup<"selector-type-mismatch">;
 def Selector : DiagGroup<"selector", [SelectorTypeMismatch]>;
 def Protocol : DiagGroup<"protocol">;
-def AtProtocol : DiagGroup<"at-protocol">;
+// No longer in use, preserve for backwards compatibility.
+def : DiagGroup<"at-protocol">;
 def PropertyAccessDotSyntax: DiagGroup<"property-access-dot-syntax">;
 def PropertyAttr : DiagGroup<"property-attribute-mismatch">;
 def SuperSubClassMismatch : DiagGroup<"super-class-method-mismatch">;
 def OverridingMethodMismatch : DiagGroup<"overriding-method-mismatch">;
 def VariadicMacros : DiagGroup<"variadic-macros">;
 def VectorConversion : DiagGroup<"vector-conversion">;      // clang specific
 def VexingParse : DiagGroup<"vexing-parse">;
 def VLA : DiagGroup<"vla">;

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_undef_interface : Warning<"cannot find interface declaration for %0">;
 def warn_duplicate_protocol_def : Warning<
   "duplicate protocol definition of %0 is ignored">,
   InGroup<DiagGroup<"duplicate-protocol">>;
 def err_protocol_has_circular_dependency : Error<
   "protocol has circular dependency">;
 def err_undeclared_protocol : Error<"cannot find protocol declaration for %0">;
 def warn_undef_protocolref : Warning<"cannot find protocol definition for %0">;
-def warn_atprotocol_protocol : Warning<
-  "@protocol is using a forward protocol declaration of %0">, InGroup<AtProtocol>;
+def err_atprotocol_protocol : Error<
+  "@protocol is using a forward protocol declaration of %0">;
 def warn_readonly_property : Warning<
   "attribute 'readonly' of property %0 restricts attribute "
   "'readwrite' of property inherited from %1">,
   InGroup<PropertyAttr>;
 
 def warn_property_attribute : Warning<
   "'%1' attribute on property %0 does not match the property inherited from %2">,
   InGroup<PropertyAttr>;

cfe/trunk/lib/CodeGen/CGObjCMac.cpp

llvm::Constant *CGObjCNonFragileABIMac::GetOrEmitProtocol(
   const ObjCProtocolDecl *PD) {
   llvm::GlobalVariable *Entry = Protocols[PD->getIdentifier()];
 
   // Early exit if a defining object has already been generated.
   if (Entry && Entry->hasInitializer())
     return Entry;
 
   // Use the protocol definition, if there is one.
-  if (const ObjCProtocolDecl *Def = PD->getDefinition())
-    PD = Def;
+  assert(PD->hasDefinition() &&
+         "emitting protocol metadata without definition");
+  PD = PD->getDefinition();
 
   auto methodLists = ProtocolMethodLists::get(PD);
 
   ConstantInitBuilder builder(CGM);
   auto values = builder.beginStruct(ObjCTypes.ProtocolnfABITy);
 
   // isa is NULL
   values.addNullPointer(ObjCTypes.ObjectPtrTy);

cfe/trunk/lib/Sema/SemaExpr.cpp

Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult &CallerRHS,
   //
   // Suppress this for references: C++ 8.5.3p5.
   if (!LHSType->isReferenceType()) {
     // FIXME: We potentially allocate here even if ConvertRHS is false.
     RHS = DefaultFunctionArrayLvalueConversion(RHS.get(), Diagnose);
     if (RHS.isInvalid())
       return Incompatible;
   }
-
-  Expr *PRE = RHS.get()->IgnoreParenCasts();
-  if (Diagnose && isa<ObjCProtocolExpr>(PRE)) {
-    ObjCProtocolDecl *PDecl = cast<ObjCProtocolExpr>(PRE)->getProtocol();
-    if (PDecl && !PDecl->hasDefinition()) {
-      Diag(PRE->getExprLoc(), diag::warn_atprotocol_protocol) << PDecl;
-      Diag(PDecl->getLocation(), diag::note_entity_declared_at) << PDecl;
-    }
-  }
-
   CastKind Kind;
   Sema::AssignConvertType result =
     CheckAssignmentConstraints(LHSType, RHS, Kind, ConvertRHS);
 
   // C99 6.5.16.1p2: The value of the right operand is converted to the
   // type of the assignment expression.
   // CheckAssignmentConstraints allows the left-hand side to be a reference,
   // so that we can use references in built-in functions even in C.

cfe/trunk/lib/Sema/SemaExprObjC.cpp

ExprResult Sema::ParseObjCProtocolExpression(IdentifierInfo *ProtocolId,
                                              SourceLocation LParenLoc,
                                              SourceLocation ProtoIdLoc,
                                              SourceLocation RParenLoc) {
   ObjCProtocolDecl* PDecl = LookupProtocol(ProtocolId, ProtoIdLoc);
   if (!PDecl) {
     Diag(ProtoLoc, diag::err_undeclared_protocol) << ProtocolId;
     return true;
   }
-  if (PDecl->hasDefinition())
+  if (!PDecl->hasDefinition()) {
+    Diag(ProtoLoc, diag::err_atprotocol_protocol) << PDecl;
+    Diag(PDecl->getLocation(), diag::note_entity_declared_at) << PDecl;
+  } else {
     PDecl = PDecl->getDefinition();
+  }
 
   QualType Ty = Context.getObjCProtoType();
   if (Ty.isNull())
     return true;
   Ty = Context.getObjCObjectPointerType(Ty);
   return new (Context) ObjCProtocolExpr(Ty, PDecl, AtLoc, ProtoIdLoc, RParenLoc);
 }
 

cfe/trunk/test/CodeGenObjC/forward-declare-protocol-gnu.m

 // RUN: %clang -S -emit-llvm %s -o - -x objective-c -fobjc-runtime=gnustep-1.5 | FileCheck  %s
 
 // Regression test: check that we don't crash when referencing a forward-declared protocol.
 @protocol P;
 
-Protocol *getProtocol(void)
-{
-          return @protocol(P);
-}
+@interface I <P>
+@end
+
+@implementation I
+
+@end
 
 // CHECK: @.objc_protocol

cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m

 // RUN: %clang_cc1 -emit-llvm -x objective-c %s -o - | FileCheck %s
 // rdar://16203115
 
 @interface NSObject @end
 
-@protocol P0;
+@protocol P0 @end
 
 @interface A : NSObject <P0>
 +(Class) getClass;
 @end
 
 @implementation A
 +(Class) getClass { return self; }
 @end
 
 int main() {
   Protocol *P0 = @protocol(P0);
   return 0;
 }
 
 // CHECK: @"\01l_OBJC_PROTOCOL_$_P0" = weak hidden global
-// CHECK: @"\01l_OBJC_CLASS_PROTOCOLS_$_A" = private global
 // CHECK: @"\01l_OBJC_LABEL_PROTOCOL_$_P0" = weak hidden global
+// CHECK: @"\01l_OBJC_CLASS_PROTOCOLS_$_A" = private global
 // CHECK: @"\01l_OBJC_PROTOCOL_REFERENCE_$_P0" = weak hidden global
 
 // CHECK: llvm.used = appending global [3 x i8*]
 // CHECK-SAME: "\01l_OBJC_PROTOCOL_$_P0"
 // CHECK-SAME: "\01l_OBJC_LABEL_PROTOCOL_$_P0"
 // CHECK-SAME: "\01l_OBJC_PROTOCOL_REFERENCE_$_P0"
 
 // CHECK: llvm.compiler.used = appending global [7 x i8*]
 // CHECK-SAME: OBJC_CLASS_NAME_
 // CHECK-SAME: OBJC_METH_VAR_NAME_
 // CHECK-SAME: OBJC_METH_VAR_TYPE_
 // CHECK-SAME: "\01l_OBJC_$_CLASS_METHODS_A"
-// CHECK-SAME: "\01l_OBJC_CLASS_PROTOCOLS_$_A"
 // CHECK-SAME: OBJC_CLASS_NAME_.1
+// CHECK-SAME: "\01l_OBJC_CLASS_PROTOCOLS_$_A"
 // CHECK-SAME: "OBJC_LABEL_CLASS_$"
 // CHECK-SAME: section "llvm.metadata"

cfe/trunk/test/CodeGenObjC/hidden-visibility.m

 @property int P;
 @end
 
 @implementation I
 @synthesize P;
 @end
 
 
-@protocol Prot0;
+@protocol Prot0 @end
 
 id f0() {
   return @protocol(Prot0);
 }
 
 

cfe/trunk/test/CodeGenObjC/link-errors.m

 // RUN: %clang_cc1 -triple i386-apple-darwin9 -fobjc-runtime=macosx-fragile-10.5 -emit-llvm -o %t %s
 // RUN: grep '.lazy_reference .objc_class_name_A' %t | count 1
 // RUN: grep '.lazy_reference .objc_class_name_Unknown' %t | count 1
 // RUN: grep '.lazy_reference .objc_class_name_Protocol' %t | count 1
 // RUN: %clang_cc1 -triple i386-apple-darwin9 -fobjc-runtime=macosx-fragile-10.5 -DWITH_IMPL -emit-llvm -o %t %s
 // RUN: grep '.lazy_reference .objc_class_name_Root' %t | count 1
 
 @interface Root
 -(id) alloc;
 -(id) init;
 @end
 
-@protocol P;
+@protocol P @end
 
 @interface A : Root
 @end
 
 @interface A (Category)
 +(void) foo;
 @end
 

cfe/trunk/test/CodeGenObjC/protocol-comdat.m

 // RUN: %clang -cc1 -triple thumbv7--windows-itanium -fobjc-runtime=ios -emit-llvm -o - %s -Wno-objc-root-class | FileCheck %s
 
 @protocol P
 - (void) method;
 @end
 
-@protocol Q;
-@protocol R;
+@protocol Q @end
+@protocol R @end
 
 @interface I<P>
 @end
 
 @implementation I
 - (void) method { }
 @end
 

cfe/trunk/test/CodeGenObjC/protocols-lazy.m

 // RUN: grep OBJC_PROTOCOL_INSTANCE_METHODS_P2 %t | count 3
 @protocol P2 -im1; @end
 void f0() { id x = @protocol(P2); }
 
 // Forward definition triggered by protocol reference.
 // RUN: grep OBJC_PROTOCOL_P3 %t | count 3
 // RUN: not grep OBJC_PROTOCOL_INSTANCE_METHODS_P3 %t
 @protocol P3;
-void f1() { id x = @protocol(P3); }
+@interface UserP3<P3>
+@end
+@implementation UserP3
+@end
 
 // Definition triggered by class reference.
 // RUN: grep OBJC_PROTOCOL_P4 %t | count 3
 // RUN: grep OBJC_PROTOCOL_INSTANCE_METHODS_P4 %t | count 3
 @protocol P4 -im1; @end
 @interface I0<P4> @end
 @implementation I0 -im1 { return 0; }; @end
 
 // Definition following forward reference.
 // RUN: grep OBJC_PROTOCOL_P5 %t | count 3
 // RUN: grep OBJC_PROTOCOL_INSTANCE_METHODS_P5 %t | count 3
 @protocol P5;
-void f2() { id x = @protocol(P5); } // This generates a forward
-                                    // reference, which has to be
-                                    // updated on the next line.
-@protocol P5 -im1; @end               
+@interface UserP5<P5> // This generates a forward
+@end
+@protocol P5 -im1; @end
+@implementation UserP5
+
+- im1 { }
+
+@end
 
 // Protocol reference following definition.
 // RUN: grep OBJC_PROTOCOL_P6 %t | count 4
 // RUN: grep OBJC_PROTOCOL_INSTANCE_METHODS_P6 %t | count 3
 @protocol P6 -im1; @end
 @interface I1<P6> @end
 @implementation I1 -im1 { return 0; }; @end
 void f3() { id x = @protocol(P6); }
 

cfe/trunk/test/CodeGenObjC/protocols.m

 // RUN: %clang_cc1 -emit-llvm-only %s
 
 void p(const char*, ...);
 
 @interface Root
 +(int) maxValue;
 -(int) conformsTo: (id) x;
 @end
 
-@protocol P0;
+@protocol P0
+@end
 
 @protocol P1
 +(void) classMethodReq0;
 -(void) methodReq0;
 @optional
 +(void) classMethodOpt1;
 -(void) methodOpt1;
 @required

cfe/trunk/test/PCH/objc_exprs.h

 
 @protocol foo;
+@protocol foo2
+@end
 @class itf;
 
 // Expressions
 typedef typeof(@"foo" "bar") objc_string;
 typedef typeof(@encode(int)) objc_encode;
-typedef typeof(@protocol(foo)) objc_protocol;
+typedef typeof(@protocol(foo2)) objc_protocol;
 typedef typeof(@selector(noArgs)) objc_selector_noArgs;
 typedef typeof(@selector(oneArg:)) objc_selector_oneArg;
 typedef typeof(@selector(foo:bar:)) objc_selector_twoArg;
 
 
 // Types.
 typedef typeof(id<foo>) objc_id_protocol_ty;
 

cfe/trunk/test/Parser/objc-cxx-keyword-identifiers.mm

 @end
 
 @protocol P // ok
 @end
 
 @protocol new // expected-error {{expected identifier; 'new' is a keyword in Objective-C++}}
 @end
 
-@protocol P2, delete; // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+@protocol P2;
+@protocol delete // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+@end
 
 @class Foo, try; // expected-error {{expected identifier; 'try' is a keyword in Objective-C++}}
 
 @interface Foo
 
 @property (readwrite, nonatomic) int a, b, throw; // expected-error {{expected member name or ';' after declaration specifiers; 'throw' is a keyword in Objective-C++}}
 
 -foo:(int)class; // expected-error {{expected identifier; 'class' is a keyword in Objective-C++}}

cfe/trunk/test/SemaObjC/protocol-expr-neg-1.m

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 @class Protocol;
 
 @protocol fproto; // expected-note {{'fproto' declared here}}
 
 @protocol p1 
 @end
 
 @class cl;
 
 int main()
 {
   Protocol *proto = @protocol(p1);
-        Protocol *fproto = @protocol(fproto); // expected-warning {{@protocol is using a forward protocol declaration of 'fproto'}}
+        Protocol *fproto = @protocol(fproto); // expected-error {{@protocol is using a forward protocol declaration of 'fproto'}}
   Protocol *pp = @protocol(i); // expected-error {{cannot find protocol declaration for 'i'}}
   Protocol *p1p = @protocol(cl); // expected-error {{cannot find protocol declaration for 'cl'}}
 }
 
 // rdar://17768630
 @protocol SuperProtocol; // expected-note {{'SuperProtocol' declared here}}
 @protocol TestProtocol; // expected-note {{'TestProtocol' declared here}}
 
 @interface I
 - (int) conformsToProtocol : (Protocol *)protocl;
 @end
 
 int doesConform(id foo) {
-  return [foo conformsToProtocol:@protocol(TestProtocol)]; // expected-warning {{@protocol is using a forward protocol declaration of 'TestProtocol'}}
+  return [foo conformsToProtocol:@protocol(TestProtocol)]; // expected-error {{@protocol is using a forward protocol declaration of 'TestProtocol'}}
 }
 
 int doesConformSuper(id foo) {
-  return [foo conformsToProtocol:@protocol(SuperProtocol)]; // expected-warning {{@protocol is using a forward protocol declaration of 'SuperProtocol'}}
+  return [foo conformsToProtocol:@protocol(SuperProtocol)]; // expected-error {{@protocol is using a forward protocol declaration of 'SuperProtocol'}}
 }