-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[LLD][COFF] Add support for -includeoptional on ARM64X #126300
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.
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
Conversation
Include symbols from both symbol tables.
Since this is an LLD extension, we can define its behavior on ARM64X. Unlike |
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Jacek Caban (cjacek) ChangesInclude symbols from both symbol tables. Full diff: https://github.com/llvm/llvm-project/pull/126300.diff 2 Files Affected:
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 281510b7ac6ea6..7ffd3f2219757c 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -2557,14 +2557,14 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
// it.
if (symtab.findUnderscore("_load_config_used"))
symtab.addGCRoot(symtab.mangle("_load_config_used"));
- });
- if (args.hasArg(OPT_include_optional)) {
- // Handle /includeoptional
- for (auto *arg : args.filtered(OPT_include_optional))
- if (isa_and_nonnull<LazyArchive>(ctx.symtab.find(arg->getValue())))
- ctx.symtab.addGCRoot(arg->getValue());
- }
+ if (args.hasArg(OPT_include_optional)) {
+ // Handle /includeoptional
+ for (auto *arg : args.filtered(OPT_include_optional))
+ if (isa_and_nonnull<LazyArchive>(symtab.find(arg->getValue())))
+ symtab.addGCRoot(arg->getValue());
+ }
+ });
} while (run());
}
diff --git a/lld/test/COFF/arm64x-includeoptional.s b/lld/test/COFF/arm64x-includeoptional.s
new file mode 100644
index 00000000000000..40e885bd1c4536
--- /dev/null
+++ b/lld/test/COFF/arm64x-includeoptional.s
@@ -0,0 +1,17 @@
+// REQUIRE: aarch64
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-windows %s -o %t.arm64.obj
+// RUN: llvm-mc -filetype=obj -triple=arm64ec-windows %s -o %t.arm64ec.obj
+// RUN: llvm-mc -filetype=obj -triple=arm64ec-windows %S/Inputs/loadconfig-arm64ec.s -o %t-loadconfig-arm64ec.obj
+// RUN: llvm-mc -filetype=obj -triple=aarch64-windows %S/Inputs/loadconfig-arm64.s -o %t-loadconfig-arm64.obj
+
+// RUN: llvm-lib -machine:arm64x -out:%t-test.lib %t.arm64.obj %t.arm64ec.obj %t-loadconfig-arm64ec.obj %t-loadconfig-arm64.obj
+// RUN: lld-link -machine:arm64x -dll -noentry -out:%t.dll %t-test.lib -includeoptional:sym
+
+// RUN: llvm-readobj --hex-dump=.test %t.dll | FileCheck %s
+// CHECK: 0x180004000 01000000 01000000
+
+ .globl sym
+ .section .test,"dr"
+sym:
+ .word 1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As you say this option isn't a standard link.exe option, and I don't think there's much direct use of it; the main use is probably via the mingw driver via the -u
option. So the main question is what behaviour makes most sense for that flag. In the end, yes, I agree that it probably makes most sense to try to pull in the symbol in both namespaces.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/17798 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/15533 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/16667 Here is the relevant piece of the build log for the reference
|
I used REQUIRE instead of REQUIRES in the test. I pushed a fixup. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/11876 Here is the relevant piece of the build log for the reference
|
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.
Include symbols from both symbol tables.