Skip to content

[LLD, MachO] Default objc_relative_method_lists on MacOS11+/iOS14+ #101360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 7, 2024
Merged

[LLD, MachO] Default objc_relative_method_lists on MacOS11+/iOS14+ #101360

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f50084
[LLD, MachO] Default objc_relative_method_lists on MacOS11+/iOS14+
DataCorrupted Jul 30, 2024
f40e80c
Update comments in the tests; Create a test to check if flag is defau…
DataCorrupted Jul 31, 2024
dee85bc
Merge two tests into one.
DataCorrupted Aug 1, 2024
d82b683
Default relative method lest: all platforms are included
DataCorrupted Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions lld/MachO/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1140,12 +1140,20 @@ static bool shouldEmitRelativeMethodLists(const InputArgList &args) {
if (arg && arg->getOption().getID() == OPT_no_objc_relative_method_lists)
return false;

// TODO: If no flag is specified, don't default to false, but instead:
// - default false on < ios14
// - default true on >= ios14
// For now, until this feature is confirmed stable, default to false if no
// flag is explicitly specified
return false;
// If no flag is specified:
// - default true on >= ios14/macos11
// - default false on everything else
switch (config->platformInfo.target.Platform) {
case PLATFORM_IOS:
case PLATFORM_IOSSIMULATOR:
return config->platformInfo.target.MinDeployment >= VersionTuple(14, 0);
case PLATFORM_MACOS:
return config->platformInfo.target.MinDeployment >= VersionTuple(11, 0);
default:
return false;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we can't enable it for the other platforms?

See for example what we do for chained fixups (which based on my understanding was basically introduced alongside this feature):

llvm-project/lld/MachO/Driver.cpp

Lines 1109 to 1132 in 0512ba0

static const std::array<std::pair<PlatformType, VersionTuple>, 9> minVersion =
{{
{PLATFORM_IOS, VersionTuple(13, 4)},
{PLATFORM_IOSSIMULATOR, VersionTuple(16, 0)},
{PLATFORM_MACOS, VersionTuple(13, 0)},
{PLATFORM_TVOS, VersionTuple(14, 0)},
{PLATFORM_TVOSSIMULATOR, VersionTuple(15, 0)},
{PLATFORM_WATCHOS, VersionTuple(7, 0)},
{PLATFORM_WATCHOSSIMULATOR, VersionTuple(8, 0)},
{PLATFORM_XROS, VersionTuple(1, 0)},
{PLATFORM_XROS_SIMULATOR, VersionTuple(1, 0)},
}};
PlatformType platform = config->platformInfo.target.Platform;
auto it = llvm::find_if(minVersion,
[&](const auto &p) { return p.first == platform; });
// We don't know the versions for other platforms, so default to disabled.
if (it == minVersion.end())
return false;
if (it->second > config->platformInfo.target.MinDeployment)
return false;
return true;

Looks like ld64 special-cases x86, but other than that, the version2020Fall condition covers watchOS, tvOS and friends as well.

https://github.com/apple-oss-distributions/ld64/blob/47f477cb721755419018f7530038b272e9d0cdea/src/ld/Options.cpp#L6085-L6101

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can definitely add those platforms as well.

But can you shed more light how to special case x86 like the code you mentioned?

I don't find equivalence of Options::kDynamicExecutable in LLVM yet, only MH_EXECUTE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think that part is safe to ignore, no need to special case it.

Apple might need to back-deploy executables during macOS's development process, but that is not relevant for user code.

llvm_unreachable("RelativeMethodList should default to false, control flow "
"should not reach here");
}

void SymbolPatterns::clear() {
Expand Down
19 changes: 11 additions & 8 deletions lld/test/MachO/objc-category-conflicts.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# `-no_objc_relative_method_lists` needs to be explicitly added to this test to avoid crashing after `-objc_relative_method_lists` was made default.
# TODO: Make this test compatible with default `-objc_relative_method_lists` and remove the `-no_objc_relative_method_lists` flag. Issue #101419

# REQUIRES: x86
# RUN: rm -rf %t; split-file %s %t
# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos11.0 -I %t %t/cat1.s -o %t/cat1.o
Expand All @@ -10,31 +13,31 @@
# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos11.0 -I %t %t/cat2.s --defsym MAKE_LOAD_METHOD=1 -o %t/cat2-with-load.o
# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos11.0 -I %t %t/klass.s --defsym MAKE_LOAD_METHOD=1 -o %t/klass-with-load.o
# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos11.0 -I %t %t/klass-with-no-rodata.s -o %t/klass-with-no-rodata.o
# RUN: %lld -dylib -lobjc %t/klass.o -o %t/libklass.dylib
# RUN: %lld -no_objc_relative_method_lists -dylib -lobjc %t/klass.o -o %t/libklass.dylib

# RUN: %no-fatal-warnings-lld --check-category-conflicts -dylib -lobjc %t/klass.o %t/cat1.o %t/cat2.o -o \
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists --check-category-conflicts -dylib -lobjc %t/klass.o %t/cat1.o %t/cat2.o -o \
# RUN: /dev/null 2>&1 | FileCheck %s --check-prefixes=CATCLS,CATCAT
# RUN: %no-fatal-warnings-lld --check-category-conflicts -dylib -lobjc %t/libklass.dylib %t/cat1.o \
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists --check-category-conflicts -dylib -lobjc %t/libklass.dylib %t/cat1.o \
# RUN: %t/cat2.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=CATCAT

# RUN: %no-fatal-warnings-lld --check-category-conflicts -dylib -lobjc %t/klass_w_sym.o %t/cat1_w_sym.o %t/cat2_w_sym.o -o \
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists --check-category-conflicts -dylib -lobjc %t/klass_w_sym.o %t/cat1_w_sym.o %t/cat2_w_sym.o -o \
# RUN: /dev/null 2>&1 | FileCheck %s --check-prefixes=CATCLS_W_SYM,CATCAT_W_SYM
# RUN: %no-fatal-warnings-lld --check-category-conflicts -dylib -lobjc %t/libklass.dylib %t/cat1_w_sym.o \
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists --check-category-conflicts -dylib -lobjc %t/libklass.dylib %t/cat1_w_sym.o \
# RUN: %t/cat2_w_sym.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=CATCAT_W_SYM

## Check that we don't emit spurious warnings around the +load method while
## still emitting the other warnings. Note that we have made separate
## `*-with-load.s` files for ease of comparison with ld64; ld64 will not warn
## at all if multiple +load methods are present.
# RUN: %no-fatal-warnings-lld --check-category-conflicts -dylib -lobjc %t/klass-with-load.o \
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists --check-category-conflicts -dylib -lobjc %t/klass-with-load.o \
# RUN: %t/cat1-with-load.o %t/cat2-with-load.o -o /dev/null 2>&1 | \
# RUN: FileCheck %s --check-prefixes=CATCLS,CATCAT --implicit-check-not '+load'

## Regression test: Check that we don't crash.
# RUN: %no-fatal-warnings-lld --check-category-conflicts -dylib -lobjc %t/klass-with-no-rodata.o -o /dev/null
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists --check-category-conflicts -dylib -lobjc %t/klass-with-no-rodata.o -o /dev/null

## Check that we don't emit any warnings without --check-category-conflicts.
# RUN: %no-fatal-warnings-lld -dylib -lobjc %t/klass.o %t/cat1.o %t/cat2.o -o \
# RUN: %no-fatal-warnings-lld -no_objc_relative_method_lists -dylib -lobjc %t/klass.o %t/cat1.o %t/cat2.o -o \
# RUN: /dev/null 2>&1 | FileCheck %s --implicit-check-not 'warning' --allow-empty

# CATCLS: warning: method '+s1' has conflicting definitions:
Expand Down
15 changes: 9 additions & 6 deletions lld/test/MachO/objc-category-merging-complete-test.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,27 @@
# `-no_objc_relative_method_lists` needs to be explicitly added to this test to avoid crashing after `-objc_relative_method_lists` was made default.
# TODO: Make this test compatible with default `-objc_relative_method_lists` and remove the `-no_objc_relative_method_lists` flag. Issue #101419

# REQUIRES: aarch64
# RUN: rm -rf %t; split-file %s %t && cd %t

############ Test merging multiple categories into a single category ############
## Create a dylib to link against(a64_file1.dylib) and merge categories in the main binary (file2_merge_a64.exe)
# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o a64_file1.o a64_file1.s
# RUN: %lld -arch arm64 a64_file1.o -o a64_file1.dylib -dylib
# RUN: %lld -no_objc_relative_method_lists -arch arm64 a64_file1.o -o a64_file1.dylib -dylib

# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o a64_file2.o a64_file2.s
# RUN: %lld -arch arm64 -o a64_file2_no_merge.exe a64_file1.dylib a64_file2.o
# RUN: %lld -arch arm64 -o a64_file2_no_merge_v2.exe a64_file1.dylib a64_file2.o -no_objc_category_merging
# RUN: %lld -arch arm64 -o a64_file2_no_merge_v3.exe a64_file1.dylib a64_file2.o -objc_category_merging -no_objc_category_merging
# RUN: %lld -arch arm64 -o a64_file2_merge.exe -objc_category_merging a64_file1.dylib a64_file2.o
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -o a64_file2_no_merge.exe a64_file1.dylib a64_file2.o
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -o a64_file2_no_merge_v2.exe a64_file1.dylib a64_file2.o -no_objc_category_merging
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -o a64_file2_no_merge_v3.exe a64_file1.dylib a64_file2.o -objc_category_merging -no_objc_category_merging
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -o a64_file2_merge.exe -objc_category_merging a64_file1.dylib a64_file2.o

# RUN: llvm-objdump --objc-meta-data --macho a64_file2_no_merge.exe | FileCheck %s --check-prefixes=NO_MERGE_CATS
# RUN: llvm-objdump --objc-meta-data --macho a64_file2_no_merge_v2.exe | FileCheck %s --check-prefixes=NO_MERGE_CATS
# RUN: llvm-objdump --objc-meta-data --macho a64_file2_no_merge_v3.exe | FileCheck %s --check-prefixes=NO_MERGE_CATS
# RUN: llvm-objdump --objc-meta-data --macho a64_file2_merge.exe | FileCheck %s --check-prefixes=MERGE_CATS

############ Test merging multiple categories into the base class ############
# RUN: %lld -arch arm64 -o a64_file2_merge_into_class.exe -objc_category_merging a64_file1.o a64_file2.o
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -o a64_file2_merge_into_class.exe -objc_category_merging a64_file1.o a64_file2.o
# RUN: llvm-objdump --objc-meta-data --macho a64_file2_merge_into_class.exe | FileCheck %s --check-prefixes=MERGE_CATS_CLS


Expand Down
5 changes: 4 additions & 1 deletion lld/test/MachO/objc-category-merging-erase-objc-name-test.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
# `-no_objc_relative_method_lists` needs to be explicitly added to this test to avoid crashing after `-objc_relative_method_lists` was made default.
# TODO: Make this test compatible with default `-objc_relative_method_lists` and remove the `-no_objc_relative_method_lists` flag. Issue #101419

; REQUIRES: aarch64

; Here we test that if we defined a protocol MyTestProtocol and also a category MyTestProtocol
; then when merging the category into the base class (and deleting the category), we don't
; delete the 'MyTestProtocol' name

; RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o %T/erase-objc-name.o %s
; RUN: %lld -arch arm64 -dylib -o %T/erase-objc-name.dylib %T/erase-objc-name.o -objc_category_merging
; RUN: %lld -no_objc_relative_method_lists -arch arm64 -dylib -o %T/erase-objc-name.dylib %T/erase-objc-name.o -objc_category_merging
; RUN: llvm-objdump --objc-meta-data --macho %T/erase-objc-name.dylib | FileCheck %s --check-prefixes=MERGE_CATS

; === Check merge categories enabled ===
Expand Down
15 changes: 9 additions & 6 deletions lld/test/MachO/objc-category-merging-minimal.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,34 @@
# `-no_objc_relative_method_lists` needs to be explicitly added to this test to avoid crashing after `-objc_relative_method_lists` was made default.
# TODO: Make this test compatible with default `-objc_relative_method_lists` and remove the `-no_objc_relative_method_lists` flag. Issue #101419

# REQUIRES: aarch64
# RUN: rm -rf %t; split-file %s %t && cd %t

############ 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
# RUN: %lld -no_objc_relative_method_lists -arch arm64 a64_fakedylib.o -o a64_fakedylib.dylib -dylib

## Create our main testing dylib - linking against the fake dylib above
# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o merge_cat_minimal.o merge_cat_minimal.s
# RUN: %lld -arch arm64 -dylib -o merge_cat_minimal_no_merge.dylib a64_fakedylib.dylib merge_cat_minimal.o
# RUN: %lld -arch arm64 -dylib -o merge_cat_minimal_merge.dylib -objc_category_merging a64_fakedylib.dylib merge_cat_minimal.o
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -dylib -o merge_cat_minimal_no_merge.dylib a64_fakedylib.dylib merge_cat_minimal.o
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -dylib -o merge_cat_minimal_merge.dylib -objc_category_merging a64_fakedylib.dylib merge_cat_minimal.o

## Now verify that the flag caused category merging to happen appropriatelly
# 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: %lld -no_objc_relative_method_lists -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 -no_objc_relative_method_lists -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

############ Test merging swift category into the base class ############
# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o MyBaseClassSwiftExtension.o MyBaseClassSwiftExtension.s
# RUN: %lld -arch arm64 -dylib -o merge_base_class_swift_minimal_yes_merge.dylib -objc_category_merging MyBaseClassSwiftExtension.o merge_base_class_minimal.o
# RUN: %lld -no_objc_relative_method_lists -arch arm64 -dylib -o merge_base_class_swift_minimal_yes_merge.dylib -objc_category_merging MyBaseClassSwiftExtension.o merge_base_class_minimal.o
# RUN: llvm-objdump --objc-meta-data --macho merge_base_class_swift_minimal_yes_merge.dylib | FileCheck %s --check-prefixes=YES_MERGE_INTO_BASE_SWIFT

#### Check merge categories enabled ###
Expand Down
Loading