[lld-macho][ObjC] Implement category merging into base class #92448
Conversation
|
@llvm/pr-subscribers-lld-macho @llvm/pr-subscribers-lld Author: None (alx32) ChangesCurrently category merging only supports merging multiple categories into one. With this commit we add the ability to fully merge categories into the base class, if the base class is included in the current module. This is the optimal approach for defined classes. Full diff: https://github.com/llvm/llvm-project/pull/92448.diff 2 Files Affected:
diff --git a/lld/MachO/ObjC.cpp b/lld/MachO/ObjC.cpp
index 9d1612beae872..c0b9ab669ac37 100644
--- a/lld/MachO/ObjC.cpp
+++ b/lld/MachO/ObjC.cpp
@@ -379,8 +379,8 @@ class ObjcCategoryMerger {
InfoWriteSection catPtrListInfo;
};
- // Information about a pointer list in the original categories (method lists,
- // protocol lists, etc)
+ // Information about a pointer list in the original categories or class(method
+ // lists, protocol lists, etc)
struct PointerListInfo {
PointerListInfo(const char *_categoryPrefix, uint32_t _pointersPerStruct)
: categoryPrefix(_categoryPrefix),
@@ -395,9 +395,9 @@ class ObjcCategoryMerger {
std::vector<Symbol *> allPtrs;
};
- // Full information about all the categories that extend a class. This will
- // include all the additional methods, protocols, and properties that are
- // contained in all the categories that extend a particular class.
+ // Full information describing an ObjC class . This will include all the
+ // additional methods, protocols, and properties that are contained in the
+ // class and all the categories that extend a particular class.
struct ClassExtensionInfo {
ClassExtensionInfo(CategoryLayout &_catLayout) : catLayout(_catLayout){};
@@ -456,9 +456,9 @@ class ObjcCategoryMerger {
const ClassExtensionInfo &extInfo,
const PointerListInfo &ptrList);
- void emitAndLinkProtocolList(Defined *parentSym, uint32_t linkAtOffset,
- const ClassExtensionInfo &extInfo,
- const PointerListInfo &ptrList);
+ Defined *emitAndLinkProtocolList(Defined *parentSym, uint32_t linkAtOffset,
+ const ClassExtensionInfo &extInfo,
+ const PointerListInfo &ptrList);
Defined *emitCategory(const ClassExtensionInfo &extInfo);
Defined *emitCatListEntrySec(const std::string &forCategoryName,
@@ -474,6 +474,10 @@ class ObjcCategoryMerger {
uint32_t offset);
Defined *tryGetDefinedAtIsecOffset(const ConcatInputSection *isec,
uint32_t offset);
+ Defined *getClassRo(const Defined *classSym, bool getMetaRo);
+ void mergeCategoriesIntoBaseClass(const Defined *baseClass,
+ std::vector<InfoInputCategory> &categories);
+ void eraseSymbolAtIsecOffset(ConcatInputSection *isec, uint32_t offset);
void tryEraseDefinedAtIsecOffset(const ConcatInputSection *isec,
uint32_t offset);
@@ -552,6 +556,32 @@ ObjcCategoryMerger::tryGetDefinedAtIsecOffset(const ConcatInputSection *isec,
return dyn_cast_or_null<Defined>(sym);
}
+// Get the class's ro_data symbol. If getMetaRo is true, then we will return
+// the meta-class's ro_data symbol. Otherwise, we will return the class
+// (instance) ro_data symbol.
+Defined *ObjcCategoryMerger::getClassRo(const Defined *classSym,
+ bool getMetaRo) {
+ ConcatInputSection *isec = dyn_cast<ConcatInputSection>(classSym->isec());
+ if (!isec)
+ return nullptr;
+
+ Defined *classRo = nullptr;
+ if (getMetaRo) {
+ Defined *metaClass = tryGetDefinedAtIsecOffset(
+ isec, classLayout.metaClassOffset + classSym->value);
+
+ classRo = metaClass ? tryGetDefinedAtIsecOffset(
+ dyn_cast<ConcatInputSection>(metaClass->isec()),
+ classLayout.roDataOffset)
+ : nullptr;
+ } else {
+ classRo = tryGetDefinedAtIsecOffset(isec, classLayout.roDataOffset +
+ classSym->value);
+ }
+
+ return classRo;
+}
+
// Given an ConcatInputSection or CStringInputSection and an offset, if there is
// a symbol(Defined) at that offset, then erase the symbol (mark it not live)
void ObjcCategoryMerger::tryEraseDefinedAtIsecOffset(
@@ -769,11 +799,11 @@ void ObjcCategoryMerger::parseCatInfoToExtInfo(const InfoInputCategory &catInfo,
// Generate a protocol list (including header) and link it into the parent at
// the specified offset.
-void ObjcCategoryMerger::emitAndLinkProtocolList(
+Defined *ObjcCategoryMerger::emitAndLinkProtocolList(
Defined *parentSym, uint32_t linkAtOffset,
const ClassExtensionInfo &extInfo, const PointerListInfo &ptrList) {
if (ptrList.allPtrs.empty())
- return;
+ return nullptr;
assert(ptrList.allPtrs.size() == ptrList.structCount);
@@ -820,6 +850,8 @@ void ObjcCategoryMerger::emitAndLinkProtocolList(
infoCategoryWriter.catPtrListInfo.relocTemplate);
offset += target->wordSize;
}
+
+ return ptrListSym;
}
// Generate a pointer list (including header) and link it into the parent at the
@@ -1265,10 +1297,16 @@ void ObjcCategoryMerger::removeRefsToErasedIsecs() {
void ObjcCategoryMerger::doMerge() {
collectAndValidateCategoriesData();
- for (auto &entry : categoryMap)
- if (entry.second.size() > 1)
+ for (auto &entry : categoryMap) {
+ if (isa<Defined>(entry.first)) {
+ // Merge all categories into the base class
+ auto *baseClass = cast<Defined>(entry.first);
+ mergeCategoriesIntoBaseClass(baseClass, entry.second);
+ } else if (entry.second.size() > 1) {
// Merge all categories into a new, single category
mergeCategoriesIntoSingleCategory(entry.second);
+ }
+ }
// Erase all categories that were merged
eraseMergedCategories();
@@ -1302,3 +1340,100 @@ void objc::mergeCategories() {
}
void objc::doCleanup() { ObjcCategoryMerger::doCleanup(); }
+
+void ObjcCategoryMerger::mergeCategoriesIntoBaseClass(
+ const Defined *baseClass, std::vector<InfoInputCategory> &categories) {
+ assert(categories.size() >= 1 && "Expected at least one category to merge");
+
+ // Collect all the info from the categories
+ ClassExtensionInfo extInfo(catLayout);
+ for (auto &catInfo : categories) {
+ parseCatInfoToExtInfo(catInfo, extInfo);
+ }
+
+ // Get metadata for the base class
+ Defined *metaRo = getClassRo(baseClass, /*getMetaRo=*/true);
+ ConcatInputSection *metaIsec = dyn_cast<ConcatInputSection>(metaRo->isec());
+ Defined *classRo = getClassRo(baseClass, /*getMetaRo=*/false);
+ ConcatInputSection *classIsec = dyn_cast<ConcatInputSection>(classRo->isec());
+
+ // Now collect the info from the base class from the various lists in the
+ // class metadata
+ parseProtocolListInfo(classIsec, roClassLayout.baseProtocolsOffset,
+ extInfo.protocols);
+
+ parsePointerListInfo(metaIsec, roClassLayout.baseMethodsOffset,
+ extInfo.classMethods);
+
+ parsePointerListInfo(metaIsec, roClassLayout.basePropertiesOffset,
+ extInfo.classProps);
+
+ parsePointerListInfo(classIsec, roClassLayout.baseMethodsOffset,
+ extInfo.instanceMethods);
+
+ parsePointerListInfo(classIsec, roClassLayout.basePropertiesOffset,
+ extInfo.instanceProps);
+
+ // Erase the old lists - these will be generated and replaced
+ eraseSymbolAtIsecOffset(metaIsec, roClassLayout.baseMethodsOffset);
+ eraseSymbolAtIsecOffset(metaIsec, roClassLayout.baseProtocolsOffset);
+ eraseSymbolAtIsecOffset(metaIsec, roClassLayout.basePropertiesOffset);
+ eraseSymbolAtIsecOffset(classIsec, roClassLayout.baseMethodsOffset);
+ eraseSymbolAtIsecOffset(classIsec, roClassLayout.baseProtocolsOffset);
+ eraseSymbolAtIsecOffset(classIsec, roClassLayout.basePropertiesOffset);
+
+ // Emit the newly merged lists - first into the meta RO then into the class RO
+ emitAndLinkPointerList(metaRo, roClassLayout.baseMethodsOffset, extInfo,
+ extInfo.classMethods);
+
+ // Protocols are a special case - the single list is referenced by both the
+ // class RO and meta RO. Here we emit it and link it into the meta RO
+ Defined *protoListSym = emitAndLinkProtocolList(
+ metaRo, roClassLayout.baseProtocolsOffset, extInfo, extInfo.protocols);
+
+ emitAndLinkPointerList(metaRo, roClassLayout.basePropertiesOffset, extInfo,
+ extInfo.classProps);
+
+ emitAndLinkPointerList(classRo, roClassLayout.baseMethodsOffset, extInfo,
+ extInfo.instanceMethods);
+
+ // If we emitted a new protocol list, link it to the class RO also
+ if (protoListSym) {
+ createSymbolReference(classRo, protoListSym,
+ roClassLayout.baseProtocolsOffset,
+ infoCategoryWriter.catBodyInfo.relocTemplate);
+ }
+
+ emitAndLinkPointerList(classRo, roClassLayout.basePropertiesOffset, extInfo,
+ extInfo.instanceProps);
+
+ // Mark all the categories as merged - this will be used to erase them later
+ for (auto &catInfo : categories)
+ catInfo.wasMerged = true;
+}
+
+// Erase the symbol at a given offset in an InputSection
+void ObjcCategoryMerger::eraseSymbolAtIsecOffset(ConcatInputSection *isec,
+ uint32_t offset) {
+ Defined *sym = tryGetDefinedAtIsecOffset(isec, offset);
+ if (!sym)
+ return;
+
+ // Remove the symbol from isec->symbols
+ assert(isa<Defined>(sym) && "Can only erase a Defined");
+ isec->symbols.erase(
+ std::remove(isec->symbols.begin(), isec->symbols.end(), sym),
+ isec->symbols.end());
+
+ // Remove the relocs that refer to this symbol
+ auto removeAtOff = [offset](Reloc const &r) { return r.offset == offset; };
+ isec->relocs.erase(
+ std::remove_if(isec->relocs.begin(), isec->relocs.end(), removeAtOff),
+ isec->relocs.end());
+
+ // Now, if the symbol fully occupies a ConcatInputSection, we can also erase
+ // the whole ConcatInputSection
+ if (ConcatInputSection *cisec = dyn_cast<ConcatInputSection>(sym->isec()))
+ if (cisec->data.size() == sym->size)
+ eraseISec(cisec);
+}
diff --git a/lld/test/MachO/objc-category-merging-extern-class-minimal.s b/lld/test/MachO/objc-category-merging-minimal.s
similarity index 59%
rename from lld/test/MachO/objc-category-merging-extern-class-minimal.s
rename to lld/test/MachO/objc-category-merging-minimal.s
index 5dd8924df5ad6..fcd90f178b150 100644
--- a/lld/test/MachO/objc-category-merging-extern-class-minimal.s
+++ b/lld/test/MachO/objc-category-merging-minimal.s
@@ -1,7 +1,8 @@
# REQUIRES: aarch64
# RUN: rm -rf %t; split-file %s %t && cd %t
-## Create a dylib with a fake base class to link against
+############ Test merging multiple categories into a single category ############
+## Create a dylib with a fake base class to link against in when merging between categories
# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o a64_fakedylib.o a64_fakedylib.s
# RUN: %lld -arch arm64 a64_fakedylib.o -o a64_fakedylib.dylib -dylib
@@ -14,6 +15,15 @@
# RUN: llvm-objdump --objc-meta-data --macho merge_cat_minimal_no_merge.dylib | FileCheck %s --check-prefixes=NO_MERGE_CATS
# RUN: llvm-objdump --objc-meta-data --macho merge_cat_minimal_merge.dylib | FileCheck %s --check-prefixes=MERGE_CATS
+############ Test merging multiple categories into the base class ############
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o merge_base_class_minimal.o merge_base_class_minimal.s
+# RUN: %lld -arch arm64 -dylib -o merge_base_class_minimal_yes_merge.dylib -objc_category_merging merge_base_class_minimal.o merge_cat_minimal.o
+# RUN: %lld -arch arm64 -dylib -o merge_base_class_minimal_no_merge.dylib merge_base_class_minimal.o merge_cat_minimal.o
+
+# RUN: llvm-objdump --objc-meta-data --macho merge_base_class_minimal_no_merge.dylib | FileCheck %s --check-prefixes=NO_MERGE_INTO_BASE
+# RUN: llvm-objdump --objc-meta-data --macho merge_base_class_minimal_yes_merge.dylib | FileCheck %s --check-prefixes=YES_MERGE_INTO_BASE
+
+
#### Check merge categories enabled ###
# Check that the original categories are not there
MERGE_CATS-NOT: __OBJC_$_CATEGORY_MyBaseClass_$_Category01
@@ -44,6 +54,28 @@ NO_MERGE_CATS: __OBJC_$_CATEGORY_MyBaseClass_$_Category01
NO_MERGE_CATS: __OBJC_$_CATEGORY_MyBaseClass_$_Category02
+#### Check merge cateogires into base class is disabled ####
+NO_MERGE_INTO_BASE: __OBJC_$_CATEGORY_MyBaseClass_$_Category01
+NO_MERGE_INTO_BASE: __OBJC_$_CATEGORY_MyBaseClass_$_Category02
+
+#### Check merge cateogires into base class is enabled and categories are merged into base class ####
+YES_MERGE_INTO_BASE-NOT: __OBJC_$_CATEGORY_MyBaseClass_$_Category01
+YES_MERGE_INTO_BASE-NOT: __OBJC_$_CATEGORY_MyBaseClass_$_Category02
+
+YES_MERGE_INTO_BASE: _OBJC_CLASS_$_MyBaseClass
+YES_MERGE_INTO_BASE-NEXT: _OBJC_METACLASS_$_MyBaseClass
+YES_MERGE_INTO_BASE: baseMethods
+YES_MERGE_INTO_BASE-NEXT: entsize 24
+YES_MERGE_INTO_BASE-NEXT: count 3
+YES_MERGE_INTO_BASE-NEXT: name {{.*}} cat01_InstanceMethod
+YES_MERGE_INTO_BASE-NEXT: types {{.*}} v16@0:8
+YES_MERGE_INTO_BASE-NEXT: imp -[MyBaseClass(Category01) cat01_InstanceMethod]
+YES_MERGE_INTO_BASE-NEXT: name {{.*}} cat02_InstanceMethod
+YES_MERGE_INTO_BASE-NEXT: types {{.*}} v16@0:8
+YES_MERGE_INTO_BASE-NEXT: imp -[MyBaseClass(Category02) cat02_InstanceMethod]
+YES_MERGE_INTO_BASE-NEXT: name {{.*}} baseInstanceMethod
+YES_MERGE_INTO_BASE-NEXT: types {{.*}} v16@0:8
+YES_MERGE_INTO_BASE-NEXT: imp -[MyBaseClass baseInstanceMethod]
#--- a64_fakedylib.s
@@ -156,3 +188,94 @@ L_OBJC_IMAGE_INFO:
.addrsig
.addrsig_sym __OBJC_$_CATEGORY_MyBaseClass_$_Category01
+
+#--- merge_base_class_minimal.s
+; clang -c merge_base_class_minimal.mm -O3 -target arm64-apple-macos -arch arm64 -S -o merge_base_class_minimal.s
+; ================== Generated from ObjC: ==================
+; __attribute__((objc_root_class))
+; @interface MyBaseClass
+; - (void)baseInstanceMethod;
+; @end
+;
+; @implementation MyBaseClass
+; - (void)baseInstanceMethod {}
+; @end
+; ================== Generated from ObjC ==================
+ .section __TEXT,__text,regular,pure_instructions
+ .build_version macos, 11, 0
+ .p2align 2
+"-[MyBaseClass baseInstanceMethod]":
+ .cfi_startproc
+; %bb.0:
+ ret
+ .cfi_endproc
+ .section __DATA,__objc_data
+ .globl _OBJC_CLASS_$_MyBaseClass
+ .p2align 3, 0x0
+_OBJC_CLASS_$_MyBaseClass:
+ .quad _OBJC_METACLASS_$_MyBaseClass
+ .quad 0
+ .quad 0
+ .quad 0
+ .quad __OBJC_CLASS_RO_$_MyBaseClass
+ .globl _OBJC_METACLASS_$_MyBaseClass
+ .p2align 3, 0x0
+_OBJC_METACLASS_$_MyBaseClass:
+ .quad _OBJC_METACLASS_$_MyBaseClass
+ .quad _OBJC_CLASS_$_MyBaseClass
+ .quad 0
+ .quad 0
+ .quad __OBJC_METACLASS_RO_$_MyBaseClass
+ .section __TEXT,__objc_classname,cstring_literals
+l_OBJC_CLASS_NAME_:
+ .asciz "MyBaseClass"
+ .section __DATA,__objc_const
+ .p2align 3, 0x0
+__OBJC_METACLASS_RO_$_MyBaseClass:
+ .long 3
+ .long 40
+ .long 40
+ .space 4
+ .quad 0
+ .quad l_OBJC_CLASS_NAME_
+ .quad 0
+ .quad 0
+ .quad 0
+ .quad 0
+ .quad 0
+ .section __TEXT,__objc_methname,cstring_literals
+l_OBJC_METH_VAR_NAME_:
+ .asciz "baseInstanceMethod"
+ .section __TEXT,__objc_methtype,cstring_literals
+l_OBJC_METH_VAR_TYPE_:
+ .asciz "v16@0:8"
+ .section __DATA,__objc_const
+ .p2align 3, 0x0
+__OBJC_$_INSTANCE_METHODS_MyBaseClass:
+ .long 24
+ .long 1
+ .quad l_OBJC_METH_VAR_NAME_
+ .quad l_OBJC_METH_VAR_TYPE_
+ .quad "-[MyBaseClass baseInstanceMethod]"
+ .p2align 3, 0x0
+__OBJC_CLASS_RO_$_MyBaseClass:
+ .long 2
+ .long 0
+ .long 0
+ .space 4
+ .quad 0
+ .quad l_OBJC_CLASS_NAME_
+ .quad __OBJC_$_INSTANCE_METHODS_MyBaseClass
+ .quad 0
+ .quad 0
+ .quad 0
+ .quad 0
+ .section __DATA,__objc_classlist,regular,no_dead_strip
+ .p2align 3, 0x0
+l_OBJC_LABEL_CLASS_$:
+ .quad _OBJC_CLASS_$_MyBaseClass
+ .section __DATA,__objc_imageinfo,regular,no_dead_strip
+L_OBJC_IMAGE_INFO:
+ .long 0
+ .long 64
+.subsections_via_symbols
|
| return nullptr; | ||
|
|
||
| return tryGetDefinedAtIsecOffset( | ||
| dyn_cast<ConcatInputSection>(metaClass->isec()), |
There was a problem hiding this comment.
Can it be null? Then tryGetDefinedAtIsecOffset didn't seem to check it.
Otherwise, can we make it a static case, or adding an assertion?
There was a problem hiding this comment.
tryGetDefinedAtIsecOffset does check it for null. It does tryGetSymbolAtIsecOffset(which checks for null) + dyn_cast_or_null (also checks for null).
alx32
force-pushed
the
12_merge_into_classes
branch
from
9344b65 to
c5e125c
Compare
| } | ||
|
|
||
| emitAndLinkPointerList(metaRo, roClassLayout.baseMethodsOffset, extInfo, | ||
| extInfo.classMethods); |
There was a problem hiding this comment.
This code pattern seems unfortunate from the previous/existing work.
All these repetitive operations are related to extInfo and its fields classMethods, ...
I think the right abstraction is to do these operations in ClassExtensionInfo so that we can call extInfo.emitAndLinkPointerList(metaRO, roClassLayout) here.
I guess we might do the similar thing for parsePointerListInfo as it's about collecting info into ClassExtensInfo. Basically, it'd be much cleaner by implementing reader/writer parts within it and enumerate each fields.
There was a problem hiding this comment.
and enumerate each fields.
How should the enumeration look like ?
There was a problem hiding this comment.
Maybe it's too disruptive at this moment in this change. I'm just saying with this mergeCategoriesIntoBaseClass introduction, I'm seeing a lot of repetition from the previous mergeCategoriesIntoSingleCategory case. At a high-level, I'd like to see more structured data structure -- this main merger class has two components that are (1) a reader/parser which produces extInfo and (2) a writer which writes extInfo. Currently it appears all data structures are flattened, and accessed across different fields.
| PointerListInfo(const char *_categoryPrefix, uint32_t _pointersPerStruct) | ||
| : categoryPrefix(_categoryPrefix), | ||
| pointersPerStruct(_pointersPerStruct) {} | ||
|
|
||
| inline bool operator==(const PointerListInfo &cmp) { |
There was a problem hiding this comment.
I see this is only used by an assertion. Checking size only but not other pointer contents is okay? I don't have a better suggestion, but this seems unnatural to define an equality while only considering the part of contents.
There was a problem hiding this comment.
Checking size only but not other pointer contents is okay
allPtrs == cmp.allPtrs on line 394 compares the pointer list contents - i.e. that the symbols in the array are identical (as pointer values) and are in the same order.
seems unnatural to define an equality while only considering the part of contents.
I am not sure what you mean here, do you mean comparing the symbols by pointer values is not OK ? Because this should be OK as the symbols are unique pointers.
There was a problem hiding this comment.
Oh. I missed that part. btw, what about categoryPrefix? doesn't it matter?
Anyhow, overall I don't feel we need this operator overloading just for an assertion, and wonder if we can inline them somehow for an assertion.
There was a problem hiding this comment.
categoryPrefix shouldn't matter either way. Yes, operator overloading seems necessary but not sure how else to easily achieve the result. I guess we could do something like parseProtocolListInfo(a).structSize == parseProtocolListInfo(b).structSize && parseProtocolListInfo(a). ..., which would mean calling parseProtocolListInfo for each field which looks worse than the current approach.
| PointerListInfo(const char *_categoryPrefix, uint32_t _pointersPerStruct) | ||
| : categoryPrefix(_categoryPrefix), | ||
| pointersPerStruct(_pointersPerStruct) {} | ||
|
|
||
| inline bool operator==(const PointerListInfo &cmp) { |
There was a problem hiding this comment.
Oh. I missed that part. btw, what about categoryPrefix? doesn't it matter?
Anyhow, overall I don't feel we need this operator overloading just for an assertion, and wonder if we can inline them somehow for an assertion.
| } | ||
|
|
||
| emitAndLinkPointerList(metaRo, roClassLayout.baseMethodsOffset, extInfo, | ||
| extInfo.classMethods); |
There was a problem hiding this comment.
Maybe it's too disruptive at this moment in this change. I'm just saying with this mergeCategoriesIntoBaseClass introduction, I'm seeing a lot of repetition from the previous mergeCategoriesIntoSingleCategory case. At a high-level, I'd like to see more structured data structure -- this main merger class has two components that are (1) a reader/parser which produces extInfo and (2) a writer which writes extInfo. Currently it appears all data structures are flattened, and accessed across different fields.
Currently category merging only supports merging multiple categories into one. With this commit we add the ability to fully merge categories into the base class, if the base class is included in the current module. This is the optimal approach for defined classes.