[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

6681

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

6712

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

6681

Good point, fixed.

6712

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

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);
+  // Keep track of whether we have actual metadata to emit.
+  bool 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_$_"
-                                   + 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));
+    llvm::raw_svector_ostream(ExtName)
+        << Interface->getObjCRuntimeNameAsString() << "_$_" << OCD->getName();
+    auto protocolList =
+        EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_" +
+                             Interface->getObjCRuntimeNameAsString() + "_$_" +
+                             Category->getName(),
+                         Category->protocol_begin(), 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);
+  if (isEmptyCategory) {
+    // Empty category, don't emit any metadata.
+    values.abandon();
+    MethodDefinitions.clear();
+    return;
+  }
+
+  unsigned Size =
+      CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
   values.addInt(ObjCTypes.IntTy, Size);
 
   llvm::GlobalVariable *GCATV =
       finishAndCreateGlobal(values, ExtCatName.str(), CGM);
+
   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))

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

+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O0 -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"OBJC_LABEL_CATEGORY_$" = private global [1 x i8*] [i8* bitcast (%struct._category_t* @"_OBJC_$_CATEGORY_A_$_foo"
+
+@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