[LLD, MachO] Default objc_relative_method_lists on MacOS11+/iOS14+ #101360
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch makes `objc_relative_method_lists` default on MacOS 11+ / iOS 14+. Manual override still work if command line argument is provided. To test this change, many explict arguments are removed from the test files. Some explict `no_objc_relative...` are also added for tests that don't support this yet. Signed-off-by: Peter Rong <PeterRong@meta.com>
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-macho Author: Peter Rong (DataCorrupted) ChangesThis patch makes To test this change, many explict arguments are removed from the test files. Some explict Full diff: https://github.com/llvm/llvm-project/pull/101360.diff 6 Files Affected:
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index f3d2a93914f71..bbe934d9c3887 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -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;
+ };
+ llvm_unreachable("RelativeMethodList should default to false, control flow "
+ "should not reach here");
}
void SymbolPatterns::clear() {
diff --git a/lld/test/MachO/objc-category-conflicts.s b/lld/test/MachO/objc-category-conflicts.s
index eb4c0adfd9e93..b7e358afe63b6 100644
--- a/lld/test/MachO/objc-category-conflicts.s
+++ b/lld/test/MachO/objc-category-conflicts.s
@@ -1,3 +1,6 @@
+# TODO: `-objc_relative_method_lists` is turned on by default.
+# Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing.
+
# 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
@@ -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:
diff --git a/lld/test/MachO/objc-category-merging-complete-test.s b/lld/test/MachO/objc-category-merging-complete-test.s
index cb112073eb871..403c78f04e62c 100644
--- a/lld/test/MachO/objc-category-merging-complete-test.s
+++ b/lld/test/MachO/objc-category-merging-complete-test.s
@@ -1,16 +1,19 @@
+# TODO: `-objc_relative_method_lists` is turned on by default.
+# Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing.
+
# 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
@@ -18,7 +21,7 @@
# 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
diff --git a/lld/test/MachO/objc-category-merging-erase-objc-name-test.s b/lld/test/MachO/objc-category-merging-erase-objc-name-test.s
index aeb2395b3a858..1acd3f2fce93b 100644
--- a/lld/test/MachO/objc-category-merging-erase-objc-name-test.s
+++ b/lld/test/MachO/objc-category-merging-erase-objc-name-test.s
@@ -1,3 +1,6 @@
+# TODO: `-objc_relative_method_lists` is turned on by default.
+# Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing.
+
; REQUIRES: aarch64
; Here we test that if we defined a protocol MyTestProtocol and also a category MyTestProtocol
@@ -5,7 +8,7 @@
; 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 ===
diff --git a/lld/test/MachO/objc-category-merging-minimal.s b/lld/test/MachO/objc-category-merging-minimal.s
index 527493303c583..13a0ac9c431e4 100644
--- a/lld/test/MachO/objc-category-merging-minimal.s
+++ b/lld/test/MachO/objc-category-merging-minimal.s
@@ -1,15 +1,18 @@
+# TODO: `-objc_relative_method_lists` is turned on by default.
+# Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing.
+
# 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
@@ -17,15 +20,15 @@
############ 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 ###
diff --git a/lld/test/MachO/objc-relative-method-lists-simple.s b/lld/test/MachO/objc-relative-method-lists-simple.s
index 9f54b5ad828a0..171325eff77fa 100644
--- a/lld/test/MachO/objc-relative-method-lists-simple.s
+++ b/lld/test/MachO/objc-relative-method-lists-simple.s
@@ -2,15 +2,15 @@
# UNSUPPORTED: target=arm{{.*}}-unknown-linux-gnueabihf
# RUN: rm -rf %t; split-file %s %t && cd %t
-## Compile a64_rel_dylib.o
-# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos -o a64_rel_dylib.o a64_simple_class.s
+## Compile a64_rel_dylib.o with MacOS 11
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos11 -o a64_rel_dylib.o a64_simple_class.s
## Test arm64 + relative method lists
-# RUN: %no-lsystem-lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -objc_relative_method_lists
+# RUN: %no-lsystem-lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64
# RUN: llvm-objdump --macho --objc-meta-data a64_rel_dylib.dylib | FileCheck %s --check-prefix=CHK_REL
## Test arm64 + relative method lists + dead-strip
-# RUN: %no-lsystem-lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -objc_relative_method_lists -dead_strip
+# RUN: %no-lsystem-lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -dead_strip
# RUN: llvm-objdump --macho --objc-meta-data a64_rel_dylib.dylib | FileCheck %s --check-prefix=CHK_REL
## Test arm64 + traditional method lists (no relative offsets)
|
alx32
reviewed
Jul 31, 2024
Comment on lines
1
to
2
| # TODO: `-objc_relative_method_lists` is turned on by default. | ||
| # Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing. |
There was a problem hiding this comment.
I think we can make the situation clearer with something like the below.
Could you also file a github issue and link it in the comment ?
Suggested change
| # TODO: `-objc_relative_method_lists` is turned on by default. | |
| # Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing. | |
| # Adding -no_objc_relative_method_lists was needed to be explicitly added to this test to avoid it crashing after `-objc_relative_method_lists` was made default. | |
| # TODO: Make the test compatible with default -objc_relative_method_lists and remove the -no_objc_relative_method_lists flag . Issue #FILE_AN_ISSUE |
There was a problem hiding this comment.
Is my understanding correct that these crashes only occur when category merging (which is still off by default) is enabled alongside relative method lists?
If not, I'd like to see the crashes fixed first.
There was a problem hiding this comment.
Yes for the cases modified here.
There was a problem hiding this comment.
I updated the tests to reflect that both flags are needed to crash.
Comment on lines
1
to
2
| # TODO: `-objc_relative_method_lists` is turned on by default. | ||
| # Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing. |
| @@ -1,11 +1,14 @@ | |||
| # TODO: `-objc_relative_method_lists` is turned on by default. | |||
| # Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing. | |||
Comment on lines
1
to
3
| # TODO: `-objc_relative_method_lists` is turned on by default. | ||
| # Adding `-no_objc_relative_method_lists` explicitly here to avoid crashing. | ||
|
|
| @@ -2,15 +2,15 @@ | |||
| # UNSUPPORTED: target=arm{{.*}}-unknown-linux-gnueabihf | |||
There was a problem hiding this comment.
We need to add cases to test the new logic. Specifically:
MacOS 10 => defaults to not relative
MacOS 11 => defaults to relative
MacOS 10 + -objc_relative_method_lists => relative
MacOS 12 + -no_objc_relative_method_lists=> not relative
There was a problem hiding this comment.
- MacOS 10 => defaults to not relative
- MacOS 11 => defaults to relative
- MacOS 10 + -objc_relative_method_lists => relative
- MacOS 12 + -no_objc_relative_method_lists=> not relative
(2) Should be covered by removing -objc_relative_method_lists in the tests
(4) Should be covered by unchanged -no_objc_relative_method_lists on Line 17 (on MacOS 11 tho)
I need to modify lit config to test 1 and 3 (our lit config defaults to MacOS 11 now). I tried yesterday but with no luck. Let me try it again.
There was a problem hiding this comment.
our lit config defaults to MacOS 11 now
Why change lit configs - why not manually overwrite on the command line ? Like here.
…lt off on MacOS 10 Signed-off-by: Peter Rong <PeterRong@meta.com>
alx32
reviewed
Aug 1, 2024
| @@ -0,0 +1,244 @@ | |||
| # REQUIRES: aarch64 | |||
There was a problem hiding this comment.
This test seems copied from objc-relative-method-lists-simple.s. Is there a reason to copy the whole thing - rather than doing it directly in objc-relative-method-lists-simple.s ?
There was a problem hiding this comment.
They are slightly different and needs to be highlighted (minos 10 vs 11, output difference). Do you prefer to have them merged?
There was a problem hiding this comment.
Yes, better to have them merged.
If you're referring to the existing .build_version macos, 11, 0 In objc-relative-method-lists-simple.s - I think it just works removing that and having the command line flags be the determining one.
Signed-off-by: Peter Rong <PeterRong@meta.com>
DataCorrupted
commented
Aug 1, 2024
| @@ -125,7 +136,7 @@ CHK_NO_REL-NEXT: imp +[MyClass class_method_02] | |||
| .include "objc-macros.s" | |||
|
|
|||
| .section __TEXT,__text,regular,pure_instructions | |||
| .build_version macos, 11, 0 | |||
| .build_version macos, 10, 0 | |||
There was a problem hiding this comment.
Build version still required or llvm-mc defaults to 11 somehow.
There was a problem hiding this comment.
It produces 11 output even when specifying -triple=arm64-apple-macos10 ?
Or where does it default - in this test we specificly specify either 10 or 11 triple in llvm-mc invocation.
There was a problem hiding this comment.
How about changing the "old" target to 10.15 instead? -data_const is already the default for that version, so the change to the segment name above wouldn't be needed.
Even if we do say that our generated binaries should run on macOS 10.0 (from 2001!), they won't. Dyld opcodes -- which we use for pre-chained-fixups targets -- were introduced around the 10.9 times.
There was a problem hiding this comment.
10.15 still generates -data, any tips? (bumping to 10.16 would enable relative method list by default and invalidate the purpose of testing)
There was a problem hiding this comment.
This should work:
--- a/lld/test/MachO/objc-relative-method-lists-simple.s
+++ b/lld/test/MachO/objc-relative-method-lists-simple.s
@@ -2,8 +2,8 @@
# UNSUPPORTED: target=arm{{.*}}-unknown-linux-gnueabihf
# RUN: rm -rf %t; split-file %s %t && cd %t
-## Compile a64_rel_dylib.o with MacOS 11
-# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos11 -o a64_rel_dylib.o a64_simple_class.s
+## Compile a64_rel_dylib.o
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos10.15 -o a64_rel_dylib.o a64_simple_class.s
## Test arm64 + relative method lists
# RUN: %no-lsystem-lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64
@@ -17,19 +17,16 @@
# RUN: %no-lsystem-lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -no_objc_relative_method_lists
# RUN: llvm-objdump --macho --objc-meta-data a64_rel_dylib.dylib | FileCheck %s --check-prefix=CHK_NO_REL
-## Compile a64_rel_dylib.o with MacOS 10
-# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos10 -o a64_rel_dylib.o a64_simple_class.s
-
## Test arm64 + relative method lists by explicitly adding `-objc_relative_method_lists`.
-# RUN: %lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -platform_version macOS 10.0 10.0 -objc_relative_method_lists
+# RUN: %lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -platform_version macOS 10.15 10.15 -objc_relative_method_lists
# RUN: llvm-objdump --macho --objc-meta-data a64_rel_dylib.dylib | FileCheck %s --check-prefix=CHK_REL
## Test arm64 + no relative method lists by default.
-# RUN: %lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -platform_version macOS 10.0 10.0
+# RUN: %lld a64_rel_dylib.o -o a64_rel_dylib.dylib -map a64_rel_dylib.map -dylib -arch arm64 -platform_version macOS 10.15 10.15
# RUN: llvm-objdump --macho --objc-meta-data a64_rel_dylib.dylib | FileCheck %s --check-prefix=CHK_NO_REL
-CHK_REL: Contents of (__DATA{{(_CONST)?}},__objc_classlist) section
+CHK_REL: Contents of (__DATA_CONST,__objc_classlist) section
CHK_REL-NEXT: _OBJC_CLASS_$_MyClass
CHK_REL: baseMethods
CHK_REL-NEXT: entsize 12 (relative)
@@ -62,7 +59,7 @@ CHK_REL-NEXT: imp 0x{{[0-9a-f]*}} (0x{{[0-9a-f]*}}) +[MyClass class_method_0
CHK_NO_REL-NOT: (relative)
-CHK_NO_REL: Contents of (__DATA{{(_CONST)?}},__objc_classlist) section
+CHK_NO_REL: Contents of (__DATA_CONST,__objc_classlist) section
CHK_NO_REL-NEXT: _OBJC_CLASS_$_MyClass
CHK_NO_REL: baseMethods 0x{{[0-9a-f]*}} (struct method_list_t *)
@@ -136,7 +133,7 @@ CHK_NO_REL-NEXT: imp +[MyClass class_method_02]
.include "objc-macros.s"
.section __TEXT,__text,regular,pure_instructions
-.build_version macos, 10, 0
+.build_version macos, 10, 15
.objc_selector_def "-[MyClass instance_method_00]"
.objc_selector_def "-[MyClass instance_method_01]"- no need to run the assembler twice; we can link objects targeting
10.15into macOS 11 binaries - had to change the version in the
-platform_versionflag
There was a problem hiding this comment.
Oh Cool! That works!
DataCorrupted
commented
Aug 1, 2024
| @@ -51,7 +62,7 @@ CHK_REL-NEXT: imp 0x{{[0-9a-f]*}} (0x{{[0-9a-f]*}}) +[MyClass class_method_0 | |||
|
|
|||
| CHK_NO_REL-NOT: (relative) | |||
|
|
|||
| CHK_NO_REL: Contents of (__DATA_CONST,__objc_classlist) section | |||
| CHK_NO_REL: Contents of (__DATA{{(_CONST)?}},__objc_classlist) section | |||
There was a problem hiding this comment.
CONST doesn't show up on MacOS 10
BertalanD
reviewed
Aug 1, 2024
lld/MachO/Driver.cpp
Outdated
Comment on lines
1146
to
1154
| 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; | ||
| }; |
There was a problem hiding this comment.
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
Looks like ld64 special-cases x86, but other than that, the version2020Fall condition covers watchOS, tvOS and friends as well.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Category merging: Change tests to reflect that both flag is needed to crash Signed-off-by: Peter Rong <PeterRong@meta.com>
alx32
approved these changes
Aug 7, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/1902 Here is the relevant piece of the build log for the reference: |
|
Hi, we are seeing a LLDB test failure after this change was landed and the stack trace from the failed test looks related to this patch: Build task link: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b8740230069184308337/overview We haven't yet finish a local revert and verification but looking at the blamelist, the change you landed is most likely. Could you take a look please? |
This seems to be a bug in the relative method list implementation - exposed by this change making it the default. EDIT: This seems to be just a regular lldb test from the llvm repo - thought it's from an external project. I'll try running it locally. |
The failure coming from LLDB test |
|
We had a test run with this patch as well as b3b6f7c reverted and the affected lldb test passed. Since this patch (while not directly) broke LLVM's own lldb test, can we revert it and reland it after the underlying issue with |
DataCorrupted
added a commit
to DataCorrupted/llvm-project
that referenced
this pull request
Aug 8, 2024
…OS14+ (llvm#101360)" This reverts commit 6a3604e.
|
Let's revert this for now, we will figure out how to fix this later and try to land again. See #102420 |
DataCorrupted
added a commit
that referenced
this pull request
Aug 8, 2024
@zeroomega - Do you happen to know if this failure happens with Mac-x64 only ? I tried repro-ing on Mac-arm64 but the test passed for me. |
We currently don't build and test lldb on mac-arm64 bot due to some dependency issues. But I can try #104081 with this default objc_relative_method_lists change and see if it breaks LLDB on mac-x64 |
zeroomega
added a commit
to zeroomega/llvm-project
that referenced
this pull request
Aug 15, 2024
This reverts commit dd251b0.
@alx32 my test run showed |
|
Sounds good, let me revert #102420 |
DataCorrupted
added a commit
to DataCorrupted/llvm-project
that referenced
this pull request
Aug 15, 2024
This patch makes `-objc_relative_method_lists` default on MacOS 10.16+/iOS 14+. Manual override still work if command line argument is provided. To test this change, many explict arguments are removed from the test files. Some explict `-objc_no_objc_relative_method_lists` are also added for tests that don't support this yet. This commit tries to revive llvm#101360, which exposes a bug that breaks CI. llvm#104081 has fixed that bug.
DataCorrupted
added a commit
that referenced
this pull request
Aug 16, 2024
…#104519) This patch makes `-objc_relative_method_lists` default on MacOS 10.16+/iOS 14+. Manual override still work if command line argument is provided. To test this change, many explict arguments are removed from the test files. Some explict `-objc_no_objc_relative_method_lists` are also added for tests that don't support this yet. This commit tries to revive #101360, which exposes a bug that breaks CI. #104081 has fixed that bug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch makes
objc_relative_method_listsdefault on MacOS 11+ / iOS 14+. Manual override still work if command line argument is provided.To test this change, many explict arguments are removed from the test files. Some explict
no_objc_relative...are also added for tests that don't support this yet.