Skip to content

[llvm-dlltool] Use EXPORTAS name type for renamed imports on ARM64EC. #99346

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

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Jul 17, 2024

As discussed in #98229, renamed entries are more tricky on ARM64EC due to additional symbols (we need __imp_aux_* in addition to __imp_* and both mangled and unmangled symbol thunks). While we could extend weak aliases to add them, it seems cleaner to just always use EXPORTAS name type on ARM64EC targets. Unlike other targets, linkers supporting ARM64EC need to support EXPORTAS, so there is no compatibility problem with that.

Compared to weak aliases, there is a small difference for linked binaries. If both original and aliased names are referenced, this solution will create separate import entries with the same name. I don't think it's a problem in practice. Both MSVC and LLD (from my WIP branch) are fine with it and such binaries run fine on Windows.

@llvmbot
Copy link
Member

llvmbot commented Jul 17, 2024

@llvm/pr-subscribers-llvm-binary-utilities

Author: Jacek Caban (cjacek)

Changes

As discussed in #98229, renamed entries are more tricky on ARM64EC due to additional symbols (we need __imp_aux_* in addition to __imp_* and both mangled and unmangled symbol thunks). While we could extend weak aliases to add them, it seems cleaner to just always use EXPORTAS name type on ARM64EC targets. Unlike other targets, linkers supporting ARM64EC need to support EXPORTAS, so there is no compatibility problem with that.

Compared to weak aliases, there is a small difference for linked binaries. If both original and aliased names are referenced, this solution will create separate import entries with the same name. I don't think it's a problem in practice. Both MSVC and LLD (from my WIP branch) are fine with it and such binaries run fine on Windows.


Full diff: https://github.com/llvm/llvm-project/pull/99346.diff

2 Files Affected:

  • (modified) llvm/lib/Object/COFFImportFile.cpp (+4-1)
  • (modified) llvm/test/tools/llvm-dlltool/arm64ec.test (+18)
diff --git a/llvm/lib/Object/COFFImportFile.cpp b/llvm/lib/Object/COFFImportFile.cpp
index 2458a53cb6d54..e67b02405a3a1 100644
--- a/llvm/lib/Object/COFFImportFile.cpp
+++ b/llvm/lib/Object/COFFImportFile.cpp
@@ -729,7 +729,10 @@ Error writeImportLibrary(StringRef ImportName, StringRef Path,
         else if (Machine == IMAGE_FILE_MACHINE_I386 &&
                  applyNameType(IMPORT_NAME_NOPREFIX, Name) == E.ImportName)
           NameType = IMPORT_NAME_NOPREFIX;
-        else if (Name == E.ImportName)
+        else if (isArm64EC(M)) {
+          NameType = IMPORT_NAME_EXPORTAS;
+          ExportName = E.ImportName;
+        } else if (Name == E.ImportName)
           NameType = IMPORT_NAME;
         else {
           Deferred D;
diff --git a/llvm/test/tools/llvm-dlltool/arm64ec.test b/llvm/test/tools/llvm-dlltool/arm64ec.test
index b03b4eaf7b2d0..7b0590d3625b7 100644
--- a/llvm/test/tools/llvm-dlltool/arm64ec.test
+++ b/llvm/test/tools/llvm-dlltool/arm64ec.test
@@ -44,6 +44,19 @@ RUN: llvm-nm --print-armap test3.lib | FileCheck --check-prefix=ARMAP2 %s
 RUN: not llvm-dlltool -m arm64 -d test.def -N test2.def -l test4.lib 2>&1 | FileCheck --check-prefix=ERR %s
 ERR: native .def file is supported only on arm64ec target
 
+RUN: llvm-dlltool -m arm64ec -d test.def -l test3.lib
+RUN: llvm-readobj test3.lib | FileCheck --check-prefix=ALIAS %s
+
+ALIAS:      File: test.dll
+ALIAS-NEXT: Format: COFF-import-file-ARM64EC
+ALIAS-NEXT: Type: code
+ALIAS-NEXT: Name type: export as
+ALIAS-NEXT: Export name: func
+ALIAS-NEXT: Symbol: __imp_func
+ALIAS-NEXT: Symbol: func
+ALIAS-NEXT: Symbol: __imp_aux_func
+ALIAS-NEXT: Symbol: #func
+
 #--- test.def
 LIBRARY test.dll
 EXPORTS
@@ -53,3 +66,8 @@ EXPORTS
 LIBRARY test.dll
 EXPORTS
     otherfunc
+
+#--- test3.def
+LIBRARY test.dll
+EXPORTS
+    func == efunc

@cjacek cjacek force-pushed the dlltool-arm64ec branch from 7799bc5 to fbcf42b Compare July 17, 2024 19:24
@cjacek
Copy link
Contributor Author

cjacek commented Jul 17, 2024

Pushed new version with the test fixed.

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

The functional change LGTM, but one comment on the test and one unrelated discussion point.

#--- test3.def
LIBRARY test.dll
EXPORTS
func == efunc
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, isn't this the case that end up using EXPORTAS already, since #98229? Would it be clearer to test a case here, where you'd have a separate efunc entry, so that it shows that even if we have the target exported, we'd still use the EXPORTAS type instead of weak aliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, that's a more interesting case. I pushed more tests.

FWIW, we were not using weak alias path in that test, but for a wrong reason: we were comparing mangled name to demangled one (and generally skipping mangling fixup). That's no longer a problem with this PR as there are no deferred on EC target. Looking at it again we should probably take ExportName into account when filling RegularImports map anyway for corner cases with a mixture of mingw-style aliases and explicit EXPORTAS .

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@cjacek cjacek force-pushed the dlltool-arm64ec branch from 7fad409 to ba73d70 Compare July 22, 2024 12:05
@cjacek cjacek merged commit cffe115 into llvm:main Jul 22, 2024
3 of 5 checks passed
@cjacek cjacek deleted the dlltool-arm64ec branch July 22, 2024 12:13
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…#99346)

Summary:
Renamed entries are more tricky on ARM64EC than on other targets due
to additional symbols (we need `__imp_aux_*` in addition to `__imp_*`
and both mangled and unmangled symbol thunks). While we could extend
weak aliases to add them, it seems cleaner to just always use EXPORTAS
name type on ARM64EC targets. Unlike other targets, linkers supporting
ARM64EC need to support EXPORTAS, so there is no compatibility problem
with that.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants