[clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty

[guitard0g]

Summary

Currently, if we create a category in ObjC that is empty, we still emit runtime metadata for that category. This is a scenario that could commonly be run into when using __attribute__((objc_direct_members)), which elides the need for much of the category metadata. This is slightly wasteful and can be easily skipped by checking the category metadata contents during CodeGen.

rdar://66177182

[guitard0g]

Fixed code formatting.

[zoecarver]

clang/lib/CodeGen/CGObjCMac.cpp

6682

Does this really need to be an &=? Is there any case where isEmptyCategory isn't true here?

clang/test/CodeGenObjC/category-class-empty.m

2

Is -O3 needed here?

clang/test/CodeGenObjC/non-lazy-classes.m

45–48

This is to prevent another test from failing? Makes sense.

[jkorous]

LGTM if we have test coverage for all the cases.

clang/test/CodeGenObjC/category-class-empty.m

2

I would even think -O0 is better and consider also -disable-llvm-passes. Ultimately the goal is to make sure frontend codegen doesn't emit the metadata so the less processing of the IR in the backend the more sensitive the test will be.

11

I assume all the cases when we want to emit the metadata (at least one instance method, at least one class method, ...) are covered by other tests, right?
If there's a case that's missing we should add a test for it.

[ahatanak]

clang/lib/CodeGen/CGObjCMac.cpp

6709

Is it not possible to check whether the category is empty here? If it's empty, you can avoid creating the global variable and abandon the builder.

[guitard0g]

Address comments and fix fragile test to use -O0 and -disable-llvm-passes.

[guitard0g]

Abandon builder earlier before creating global variable.

[guitard0g]

clang/lib/CodeGen/CGObjCMac.cpp

6682

Good point, fixed.

6709

Good point, done.

clang/test/CodeGenObjC/category-class-empty.m

2

Good catch, changed to -O0 with -disable-llvm-passes.

11

Yeah a fair number of tests would break if we started removing categories with anything inside of them.

clang/test/CodeGenObjC/non-lazy-classes.m

45–48

Yeah this is the one case where the IRGen check was looking for the category even though it's empty. Should be a harmless change.

[guitard0g]

Nit: deleted empty line.

[ahatanak]

LGTM

[guitard0g]

@ahatanak I don't have commit access, any chance you could commit this for me?

clang/lib/CodeGen/CGObjCMac.cpp

void CGObjCNonFragileABIMac::GenerateCategory(const ObjCCategoryImplDecl *OCD) {
 
   ConstantInitBuilder builder(CGM);
   auto values = builder.beginStruct(ObjCTypes.CategorynfABITy);
   values.add(GetClassName(OCD->getIdentifier()->getName()));
   // meta-class entry symbol
   values.add(GetClassGlobal(Interface, /*metaclass*/ false, NotForDefinition));
   std::string listName =
       (Interface->getObjCRuntimeNameAsString() + "_$_" + OCD->getName()).str();
+  bool isEmptyCategory = true; // Keep track of whether we have actual metadata to emit.
 
   SmallVector<const ObjCMethodDecl *, 16> instanceMethods;
   SmallVector<const ObjCMethodDecl *, 8> classMethods;
   for (const auto *MD : OCD->methods()) {
     if (MD->isDirectMethod())
       continue;
     if (MD->isInstanceMethod()) {
       instanceMethods.push_back(MD);
     } else {
       classMethods.push_back(MD);
     }
   }
 
-  values.add(emitMethodList(listName, MethodListType::CategoryInstanceMethods,
-                            instanceMethods));
-  values.add(emitMethodList(listName, MethodListType::CategoryClassMethods,
-                            classMethods));
+  auto instanceMethodList = emitMethodList(listName, MethodListType::CategoryInstanceMethods,
+                                           instanceMethods);
+  auto classMethodList = emitMethodList(listName, MethodListType::CategoryClassMethods,
+                                        classMethods);
+  values.add(instanceMethodList);
+  values.add(classMethodList);
+  isEmptyCategory &= instanceMethodList->isNullValue() && classMethodList->isNullValue();
 
   const ObjCCategoryDecl *Category =
     Interface->FindCategoryDeclaration(OCD->getIdentifier());
   if (Category) {
     SmallString<256> ExtName;
     llvm::raw_svector_ostream(ExtName) << Interface->getObjCRuntimeNameAsString() << "_$_"
                                        << OCD->getName();
-    values.add(EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
+    auto protocolList = EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
                                          + Interface->getObjCRuntimeNameAsString() + "_$_"
                                          + Category->getName(),
                                          Category->protocol_begin(),
-                                Category->protocol_end()));
-    values.add(EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
-                                OCD, Category, ObjCTypes, false));
-    values.add(EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
-                                OCD, Category, ObjCTypes, true));
+                                         Category->protocol_end());
+    auto propertyList = EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
+                                         OCD, Category, ObjCTypes, false);
+    auto classPropertyList = EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
+                                              OCD, Category, ObjCTypes, true);
+    values.add(protocolList);
+    values.add(propertyList);
+    values.add(classPropertyList);
+    isEmptyCategory &= protocolList->isNullValue() && propertyList->isNullValue() &&
+                       classPropertyList->isNullValue();
   } else {
     values.addNullPointer(ObjCTypes.ProtocolListnfABIPtrTy);
     values.addNullPointer(ObjCTypes.PropertyListPtrTy);
     values.addNullPointer(ObjCTypes.PropertyListPtrTy);
   }
 
   unsigned Size = CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
   values.addInt(ObjCTypes.IntTy, Size);
 
   llvm::GlobalVariable *GCATV =
       finishAndCreateGlobal(values, ExtCatName.str(), CGM);
+
+  if (!isEmptyCategory) {
     CGM.addCompilerUsedGlobal(GCATV);
     if (Interface->hasAttr<ObjCClassStubAttr>())
       DefinedStubCategories.push_back(GCATV);
     else
       DefinedCategories.push_back(GCATV);
 
     // Determine if this category is also "non-lazy".
     if (ImplementationIsNonLazy(OCD))
       DefinedNonLazyCategories.push_back(GCATV);
+  }
   // method definition entries must be clear for next implementation.
   MethodDefinitions.clear();
 }
 
 /// emitMethodConstant - Return a struct objc_method constant.  If
 /// forProtocol is true, the implementation will be null; otherwise,
 /// the method must have a definition registered with the runtime.
 ///

clang/test/CodeGenObjC/category-class-empty.m

+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O3 -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"_OBJC_$_CATEGORY_A_$_foo" = internal global %struct._category_t
+
+@interface A
+@end
+__attribute__((objc_direct_members))
+@interface A(foo)
+- (void)foo_myStuff;
+@end
+@implementation A(foo)
+- (void)foo_myStuff {
+}
+@end
+

clang/test/CodeGenObjC/non-lazy-classes.m

 @implementation D @end
 
 @interface E @end
 
 __attribute__((objc_nonlazy_class))
 @implementation E @end
 
 __attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
+}
+@end