ObjCGNU: Fix empty v3 protocols being emitted two fields short

[DHowett-MSFT]

Summary

Protocols that were being referenced but could not be fully realized were being emitted without properties/optional_properties. Since all v3 protocols must be 9 processor words wide, the lack of these fields is catastrophic for the runtime.

As an example, the runtime cannot know here that properties and optional_properties are invalid.

[theraven]

Looks correct to me. Protocols are only referenced by pointer, so it doesn't matter for old versions of the runtime if these fields are present.

[rjmccall]

The change LGTM, but please add a test.

[DHowett-MSFT]

Added a test per @rjmccall's suggestion. I chose to test this by emitting assembly because the llvm ir is significantly harder to pick apart for the true _size_ of the protocol.

[rjmccall]

Hmm. Alright, I guess.

[theraven]

Isn't it better to test for the correct structure existing in the IR?

[DHowett-MSFT]

It seems more fragile to check the contents of the protocol rather than the invariant we care most about (its size).
I'm willing to do that, and am updating the patch accordingly, but I don't want to commit to a more fragile test than is necessary.
Illustratively, if the GNUstep runtime one day chooses to emit empty v3 protocols as {3, null, null, null, null, null, null, null} (which it may want to do to avoid emitting empty method lists, and which seems to work at runtime), a test capturing the body of the protocol will fail.

[theraven]

I think that we emit empty method lists so that the GCC runtime doesn't choke on them, though there's no reason why we couldn't with the GNUstep ABI. It would therefore be nice if the test failed if we did change to emitting null so that we could update the test at that point and check that it's actually doing the right thing.

The ELF version of the v2 ABI is mostly finished now, modulo writing a load of clang tests, so I'll upload that for review soon.

It would also help to have a gnu- prefix on the test name so that it's easy to identify the relevant tests, though not essential.

[DHowett-MSFT]

I've fixed the test to check the LLVM IR; I chose to use CHECK-SAME and use indentation to clarify what was going on. We're now checking that an empty protocol is emitted in full, and that all of its members match our expectations.

[DHowett-MSFT]

Hi! Is there anything else holding up this patch? Thanks!

lib/CodeGen/CGObjCGNU.cpp

CGObjCGNU::GenerateEmptyProtocol(const std::string &ProtocolName) {
   auto Elements = Builder.beginStruct();
 
   // The isa pointer must be set to a magic number so the runtime knows it's
   // the correct layout.
   Elements.add(llvm::ConstantExpr::getIntToPtr(
           llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
+  Elements.add(ProtocolList); /* .protocol_list */
+  Elements.add(MethodList);   /* .instance_methods */
+  Elements.add(MethodList);   /* .class_methods */
+  Elements.add(MethodList);   /* .optional_instance_methods */
+  Elements.add(MethodList);   /* .optional_class_methods */
+  Elements.add(NULLPtr);      /* .properties */
+  Elements.add(NULLPtr);      /* .optional_properties */
   return Elements.finishAndCreateGlobal(".objc_protocol",
                                         CGM.getPointerAlign());
 }
 
 void CGObjCGNU::GenerateProtocol(const ObjCProtocolDecl *PD) {
   ASTContext &Context = CGM.getContext();
   std::string ProtocolName = PD->getNameAsString();
   

test/CodeGenObjC/gnu-empty-protocol-v3.m

+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fobjc-runtime=gnustep-1.9 -emit-llvm -o - %s | FileCheck %s
+
+@protocol X;
+
+__attribute__((objc_root_class))
+@interface Z <X>
+@end
+
+@implementation Z
+@end
+
+// CHECK:      @.objc_protocol_list = internal global { i8*, i32, [0 x i8*] } zeroinitializer, align 4
+// CHECK:      @.objc_method_list = internal global { i32, [0 x { i8*, i8* }] } zeroinitializer, align 4
+// CHECK:      @.objc_protocol_name = private unnamed_addr constant [2 x i8] c"X\00", align 1
+// CHECK:      @.objc_protocol = internal global { i8*, i8*, { i8*, i32, [0 x i8*] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, i8*, i8* } {
+// CHECK-SAME:     i8* inttoptr (i32 3 to i8*),
+// CHECK-SAME:     i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_protocol_name, i32 0, i32 0),
+// CHECK-SAME:     { i8*, i32, [0 x i8*] }* @.objc_protocol_list,
+// CHECK-SAME:     { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME:     { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME:     { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME:     { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME:     i8* null,
+// CHECK-SAME:     i8* null
+// CHECK-SAME: }, align 4