-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[LLD][COFF] Allow additional attributes in forwarding exports. #86535
Conversation
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Jacek Caban (cjacek) ChangesTesting with MSVC link.exe showed that it respects such options, while LLD currently discards them. I noticed it while working on adding support for EXPORTAS parameter and needed to decide what to do about this early return. Full diff: https://github.com/llvm/llvm-project/pull/86535.diff 2 Files Affected:
diff --git a/lld/COFF/DriverUtils.cpp b/lld/COFF/DriverUtils.cpp
index fc8eb327be49bd..0fa4769bab19db 100644
--- a/lld/COFF/DriverUtils.cpp
+++ b/lld/COFF/DriverUtils.cpp
@@ -577,16 +577,15 @@ Export LinkerDriver::parseExport(StringRef arg) {
if (y.contains(".")) {
e.name = x;
e.forwardTo = y;
- return e;
+ } else {
+ e.extName = x;
+ e.name = y;
+ if (e.name.empty())
+ goto err;
}
-
- e.extName = x;
- e.name = y;
- if (e.name.empty())
- goto err;
}
- // If "<name>=<internalname>[,@ordinal[,NONAME]][,DATA][,PRIVATE]"
+ // Optional parameters "[,@ordinal[,NONAME]][,DATA][,PRIVATE]"
while (!rest.empty()) {
StringRef tok;
std::tie(tok, rest) = rest.split(",");
diff --git a/lld/test/COFF/export.test b/lld/test/COFF/export.test
index d340e0174b563e..7b804852e6d34a 100644
--- a/lld/test/COFF/export.test
+++ b/lld/test/COFF/export.test
@@ -76,18 +76,45 @@ SYMTAB: exportfn3 in export.test.tmp.DLL
# RUN: lld-link /out:%t.dll /dll %t.obj /export:foo=kernel32.foobar
# RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=FORWARDER %s
+# RUN: llvm-nm -M %t.lib | FileCheck --check-prefix=SYMTAB-FWD %s
# RUN: echo "EXPORTS foo=kernel32.foobar" > %t.def
-# RUN: lld-link /out:%t.dll /dll %t.obj /def:%t.def
-# RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=FORWARDER %s
+# RUN: lld-link /out:%t-def.dll /dll %t.obj /def:%t.def
+# RUN: llvm-objdump -p %t-def.dll | FileCheck --check-prefix=FORWARDER %s
+# RUN: llvm-nm -M %t-def.lib | FileCheck --check-prefix=SYMTAB-FWD %s
FORWARDER: Export Table:
-FORWARDER: DLL name: export.test.tmp.dll
+FORWARDER: DLL name: export.test.tmp
FORWARDER: Ordinal base: 1
FORWARDER: Ordinal RVA Name
FORWARDER: 1 0x1010 exportfn
FORWARDER: 2 foo (forwarded to kernel32.foobar)
+SYMTAB-FWD: __imp_exportfn3 in export.test.tmp
+SYMTAB-FWD: __imp_foo in export.test.tmp
+SYMTAB-FWD: exportfn3 in export.test.tmp
+SYMTAB-FWD: foo in export.test.tmp
+
+# RUN: lld-link /out:%t-fwd-priv.dll /dll %t.obj /export:foo=kernel32.foobar,DATA,PRIVATE
+# RUN: llvm-objdump -p %t-fwd-priv.dll | FileCheck --check-prefix=FORWARDER %s
+# RUN: llvm-nm -M %t-fwd-priv.lib | FileCheck --check-prefix=SYMTAB-FWD-PRIV %s
+
+SYMTAB-FWD-PRIV: __imp_exportfn3 in export.test.tmp-fwd-priv
+SYMTAB-FWD-PRIV-NOT: __imp_foo
+SYMTAB-FWD-PRIV: exportfn3 in export.test.tmp-fwd-priv
+SYMTAB-FWD-PRIV-NOT: foo
+
+# RUN: lld-link /out:%t-fwd-ord.dll /dll %t.obj /export:foo=kernel32.foobar,@3,NONAME
+# RUN: llvm-objdump -p %t-fwd-ord.dll | FileCheck --check-prefix=FORWARDER-ORD %s
+# RUN: llvm-nm -M %t-fwd-ord.lib | FileCheck --check-prefix=SYMTAB-FWD %s
+
+FORWARDER-ORD: Export Table:
+FORWARDER-ORD: DLL name: export.test.tmp-fwd-ord.dll
+FORWARDER-ORD: Ordinal base: 3
+FORWARDER-ORD: Ordinal RVA Name
+FORWARDER-ORD: 3 (forwarded to kernel32.foobar)
+FORWARDER-ORD: 4 0x1010 exportfn3
+
# RUN: lld-link /out:%t.dll /dll %t.obj /merge:.rdata=.text /export:exportfn1 /export:exportfn2
# RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=MERGE --match-full-lines %s
|
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
This LGTM, thanks!
Does the same work in llvm-dlltool and/or llvm-lib /def:
as well, or do we need to do some corresponding change there?
It should already work with |
lld/test/COFF/export.test
Outdated
FORWARDER: Ordinal base: 1 | ||
FORWARDER: Ordinal RVA Name | ||
FORWARDER: 1 0x1010 exportfn | ||
FORWARDER: 2 foo (forwarded to kernel32.foobar) | ||
|
||
SYMTAB-FWD: __imp_exportfn3 in export.test.tmp | ||
SYMTAB-FWD: __imp_foo in export.test.tmp |
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.
Useful to add -NEXT in case they are adjacent. When things go wrong, -NEXT provides a better diagnostic
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.
I added -NEXT and pushed. Thanks for reviews.
9d242f5
to
fd913d7
Compare
Testing with MSVC link.exe showed that it respects such options, while LLD currently discards them.
I noticed it while working on adding support for EXPORTAS parameter and needed to decide what to do about this early return.