[Arm64EC] Copy import descriptors to the EC Map #84834

dpaoliello · 2024-03-11T21:11:41Z

As noted in #78537, MSVC places import descriptors in both the EC and regular map - that PR moved the descriptors to ONLY the regular map, however this causes linking errors when linking as Arm64EC:

bcryptprimitives.lib(bcryptprimitives.dll) : error LNK2001: unresolved external symbol __IMPORT_DESCRIPTOR_bcryptprimitives (EC Symbol)

This change copies import descriptors from the regular map to the EC map, which fixes this linking error.

llvmbot · 2024-03-11T21:12:13Z

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

Author: Daniel Paoliello (dpaoliello)

Changes

As noted in <#78537>, MSVC places import descriptors in both the EC and regular map - that PR moved the descriptors to ONLY the regular map, however this causes linking errors when linking as Arm64EC:

bcryptprimitives.lib(bcryptprimitives.dll) : error LNK2001: unresolved external symbol __IMPORT_DESCRIPTOR_bcryptprimitives (EC Symbol)

This change copies import descriptors from the regular map to the EC map, which fixes this linking error.

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

5 Files Affected:

(modified) llvm/include/llvm/Object/COFFImportFile.h (+5)
(modified) llvm/lib/Object/ArchiveWriter.cpp (+19-2)
(modified) llvm/lib/Object/COFFImportFile.cpp (+3-6)
(modified) llvm/test/tools/llvm-dlltool/arm64ec.test (+6)
(modified) llvm/test/tools/llvm-lib/arm64ec-implib.test (+12)

diff --git a/llvm/include/llvm/Object/COFFImportFile.h b/llvm/include/llvm/Object/COFFImportFile.h
index 402ded0d64fef2..369fd5b8705aed 100644
--- a/llvm/include/llvm/Object/COFFImportFile.h
+++ b/llvm/include/llvm/Object/COFFImportFile.h
@@ -26,6 +26,11 @@
 namespace llvm {
 namespace object {
 
+constexpr std::string_view ImportDescriptorPrefix = "__IMPORT_DESCRIPTOR_";
+constexpr std::string_view NullImportDescriptorSymbolName = "__NULL_IMPORT_DESCRIPTOR";
+constexpr std::string_view NullThunkDataPrefix = "\x7f";
+constexpr std::string_view NullThunkDataSuffix = "_NULL_THUNK_DATA";
+
 class COFFImportFile : public SymbolicFile {
 private:
   enum SymbolIndex { ImpSymbol, ThunkSymbol, ECAuxSymbol, ECThunkSymbol };
diff --git a/llvm/lib/Object/ArchiveWriter.cpp b/llvm/lib/Object/ArchiveWriter.cpp
index be51093933a87c..fd50779ca9f748 100644
--- a/llvm/lib/Object/ArchiveWriter.cpp
+++ b/llvm/lib/Object/ArchiveWriter.cpp
@@ -677,6 +677,15 @@ static bool isECObject(object::SymbolicFile &Obj) {
   return false;
 }
 
+bool isImportDescriptor(const std::string_view Name) {
+  return Name.substr(0, ImportDescriptorPrefix.size()) == ImportDescriptorPrefix ||
+         Name == NullImportDescriptorSymbolName ||
+         (Name.substr(0, NullThunkDataPrefix.size()) == NullThunkDataPrefix &&
+          Name.size() > NullThunkDataSuffix.size() &&
+          Name.substr(Name.size() - NullThunkDataSuffix.size()) ==
+              NullThunkDataSuffix);
+}
+
 static Expected<std::vector<unsigned>> getSymbols(SymbolicFile *Obj,
                                                   uint16_t Index,
                                                   raw_ostream &SymNames,
@@ -687,8 +696,14 @@ static Expected<std::vector<unsigned>> getSymbols(SymbolicFile *Obj,
     return Ret;
 
   std::map<std::string, uint16_t> *Map = nullptr;
-  if (SymMap)
-    Map = SymMap->UseECMap && isECObject(*Obj) ? &SymMap->ECMap : &SymMap->Map;
+  bool CopyImportDescriptorToECMap = false;
+  if (SymMap) {
+    bool IsECObject = isECObject(*Obj);
+    Map = SymMap->UseECMap && IsECObject ? &SymMap->ECMap : &SymMap->Map;
+    // If EC is enabled, then the import descriptors are NOT put into EC objects
+    // so we need to copy them to the EC map manually.
+    CopyImportDescriptorToECMap = SymMap->UseECMap && !IsECObject;
+  }
 
   for (const object::BasicSymbolRef &S : Obj->symbols()) {
     if (!isArchiveSymbol(S))
@@ -704,6 +719,8 @@ static Expected<std::vector<unsigned>> getSymbols(SymbolicFile *Obj,
       if (Map == &SymMap->Map) {
         Ret.push_back(SymNames.tell());
         SymNames << Name << '\0';
+        if (CopyImportDescriptorToECMap && isImportDescriptor(Name))
+          SymMap->ECMap[Name] = Index;
       }
     } else {
       Ret.push_back(SymNames.tell());
diff --git a/llvm/lib/Object/COFFImportFile.cpp b/llvm/lib/Object/COFFImportFile.cpp
index 376dd126baf61a..f2f2b3835e20cb 100644
--- a/llvm/lib/Object/COFFImportFile.cpp
+++ b/llvm/lib/Object/COFFImportFile.cpp
@@ -108,7 +108,7 @@ template <class T> static void append(std::vector<uint8_t> &B, const T &Data) {
 }
 
 static void writeStringTable(std::vector<uint8_t> &B,
-                             ArrayRef<const std::string> Strings) {
+                             ArrayRef<const std::string_view> Strings) {
   // The COFF string table consists of a 4-byte value which is the size of the
   // table, including the length field itself.  This value is followed by the
   // string content itself, which is an array of null-terminated C-style
@@ -171,9 +171,6 @@ static Expected<std::string> replace(StringRef S, StringRef From,
   return (Twine(S.substr(0, Pos)) + To + S.substr(Pos + From.size())).str();
 }
 
-static const std::string NullImportDescriptorSymbolName =
-    "__NULL_IMPORT_DESCRIPTOR";
-
 namespace {
 // This class constructs various small object files necessary to support linking
 // symbols imported from a DLL.  The contents are pretty strictly defined and
@@ -192,8 +189,8 @@ class ObjectFactory {
 public:
   ObjectFactory(StringRef S, MachineTypes M)
       : NativeMachine(M), ImportName(S), Library(llvm::sys::path::stem(S)),
-        ImportDescriptorSymbolName(("__IMPORT_DESCRIPTOR_" + Library).str()),
-        NullThunkSymbolName(("\x7f" + Library + "_NULL_THUNK_DATA").str()) {}
+        ImportDescriptorSymbolName((ImportDescriptorPrefix + Library).str()),
+        NullThunkSymbolName((NullThunkDataPrefix + Library + NullThunkDataSuffix).str()) {}
 
   // Creates an Import Descriptor.  This is a small object file which contains a
   // reference to the terminators and contains the library name (entry) for the
diff --git a/llvm/test/tools/llvm-dlltool/arm64ec.test b/llvm/test/tools/llvm-dlltool/arm64ec.test
index e742a77ff78a52..b03b4eaf7b2d0e 100644
--- a/llvm/test/tools/llvm-dlltool/arm64ec.test
+++ b/llvm/test/tools/llvm-dlltool/arm64ec.test
@@ -12,9 +12,12 @@ ARMAP-NEXT: test_NULL_THUNK_DATA in test.dll
 ARMAP-EMPTY:
 ARMAP-NEXT: Archive EC map
 ARMAP-NEXT: #func in test.dll
+ARMAP-NEXT: __IMPORT_DESCRIPTOR_test in test.dll
+ARMAP-NEXT: __NULL_IMPORT_DESCRIPTOR in test.dll
 ARMAP-NEXT: __imp_aux_func in test.dll
 ARMAP-NEXT: __imp_func in test.dll
 ARMAP-NEXT: func in test.dll
+ARMAP-NEXT: test_NULL_THUNK_DATA in test.dll
 
 RUN: llvm-dlltool -m arm64ec -d test.def -N test2.def -l test2.lib
 RUN: llvm-nm --print-armap test2.lib | FileCheck --check-prefix=ARMAP2 %s
@@ -28,9 +31,12 @@ ARMAP2-NEXT: test_NULL_THUNK_DATA in test.dll
 ARMAP2-EMPTY:
 ARMAP2-NEXT: Archive EC map
 ARMAP2-NEXT: #func in test.dll
+ARMAP2-NEXT: __IMPORT_DESCRIPTOR_test in test.dll
+ARMAP2-NEXT: __NULL_IMPORT_DESCRIPTOR in test.dll
 ARMAP2-NEXT: __imp_aux_func in test.dll
 ARMAP2-NEXT: __imp_func in test.dll
 ARMAP2-NEXT: func in test.dll
+ARMAP2-NEXT: test_NULL_THUNK_DATA in test.dll
 
 RUN: llvm-dlltool -m arm64ec -d test.def --input-native-def test2.def -l test3.lib
 RUN: llvm-nm --print-armap test3.lib | FileCheck --check-prefix=ARMAP2 %s
diff --git a/llvm/test/tools/llvm-lib/arm64ec-implib.test b/llvm/test/tools/llvm-lib/arm64ec-implib.test
index 77bdc23589fd25..00eddd2a475268 100644
--- a/llvm/test/tools/llvm-lib/arm64ec-implib.test
+++ b/llvm/test/tools/llvm-lib/arm64ec-implib.test
@@ -16,6 +16,8 @@ ARMAP-NEXT: #funcexp in test.dll
 ARMAP-NEXT: #mangledfunc in test.dll
 ARMAP-NEXT: ?test_cpp_func@@$$hYAHPEAX@Z in test.dll
 ARMAP-NEXT: ?test_cpp_func@@YAHPEAX@Z in test.dll
+ARMAP-NEXT: __IMPORT_DESCRIPTOR_test in test.dll
+ARMAP-NEXT: __NULL_IMPORT_DESCRIPTOR in test.dll
 ARMAP-NEXT: __imp_?test_cpp_func@@YAHPEAX@Z in test.dll
 ARMAP-NEXT: __imp_aux_?test_cpp_func@@YAHPEAX@Z in test.dll
 ARMAP-NEXT: __imp_aux_expname in test.dll
@@ -28,6 +30,7 @@ ARMAP-NEXT: __imp_mangledfunc in test.dll
 ARMAP-NEXT: expname in test.dll
 ARMAP-NEXT: funcexp in test.dll
 ARMAP-NEXT: mangledfunc in test.dll
+ARMAP-NEXT: test_NULL_THUNK_DATA in test.dll
 
 RUN: llvm-readobj test.lib | FileCheck -check-prefix=READOBJ %s
 
@@ -122,6 +125,8 @@ ARMAPX-NEXT: #funcexp in test.dll
 ARMAPX-NEXT: #mangledfunc in test.dll
 ARMAPX-NEXT: ?test_cpp_func@@$$hYAHPEAX@Z in test.dll
 ARMAPX-NEXT: ?test_cpp_func@@YAHPEAX@Z in test.dll
+ARMAPX-NEXT: __IMPORT_DESCRIPTOR_test in test.dll
+ARMAPX-NEXT: __NULL_IMPORT_DESCRIPTOR in test.dll
 ARMAPX-NEXT: __imp_?test_cpp_func@@YAHPEAX@Z in test.dll
 ARMAPX-NEXT: __imp_aux_?test_cpp_func@@YAHPEAX@Z in test.dll
 ARMAPX-NEXT: __imp_aux_expname in test.dll
@@ -134,6 +139,7 @@ ARMAPX-NEXT: __imp_mangledfunc in test.dll
 ARMAPX-NEXT: expname in test.dll
 ARMAPX-NEXT: funcexp in test.dll
 ARMAPX-NEXT: mangledfunc in test.dll
+ARMAPX-NEXT: test_NULL_THUNK_DATA in test.dll
 
 RUN: llvm-readobj testx.lib | FileCheck -check-prefix=READOBJX %s
 
@@ -255,6 +261,8 @@ ARMAPX2-NEXT: #funcexp in test2.dll
 ARMAPX2-NEXT: #mangledfunc in test2.dll
 ARMAPX2-NEXT: ?test_cpp_func@@$$hYAHPEAX@Z in test2.dll
 ARMAPX2-NEXT: ?test_cpp_func@@YAHPEAX@Z in test2.dll
+ARMAPX2-NEXT: __IMPORT_DESCRIPTOR_test2 in test2.dll
+ARMAPX2-NEXT: __NULL_IMPORT_DESCRIPTOR in test2.dll
 ARMAPX2-NEXT: __imp_?test_cpp_func@@YAHPEAX@Z in test2.dll
 ARMAPX2-NEXT: __imp_aux_?test_cpp_func@@YAHPEAX@Z in test2.dll
 ARMAPX2-NEXT: __imp_aux_expname in test2.dll
@@ -267,6 +275,7 @@ ARMAPX2-NEXT: __imp_mangledfunc in test2.dll
 ARMAPX2-NEXT: expname in test2.dll
 ARMAPX2-NEXT: funcexp in test2.dll
 ARMAPX2-NEXT: mangledfunc in test2.dll
+ARMAPX2-NEXT: test2_NULL_THUNK_DATA in test2.dll
 
 ARMAPX2:      test2.dll:
 ARMAPX2:      00000000 T #funcexp
@@ -309,6 +318,8 @@ EXPAS-ARMAP-NEXT: #func1 in test.dll
 EXPAS-ARMAP-NEXT: #func2 in test.dll
 EXPAS-ARMAP-NEXT: #func3 in test.dll
 EXPAS-ARMAP-NEXT: #func4 in test.dll
+EXPAS-ARMAP-NEXT: __IMPORT_DESCRIPTOR_test in test.dll
+EXPAS-ARMAP-NEXT: __NULL_IMPORT_DESCRIPTOR in test.dll
 EXPAS-ARMAP-NEXT: __imp_aux_func1 in test.dll
 EXPAS-ARMAP-NEXT: __imp_aux_func2 in test.dll
 EXPAS-ARMAP-NEXT: __imp_aux_func3 in test.dll
@@ -323,6 +334,7 @@ EXPAS-ARMAP-NEXT: func1 in test.dll
 EXPAS-ARMAP-NEXT: func2 in test.dll
 EXPAS-ARMAP-NEXT: func3 in test.dll
 EXPAS-ARMAP-NEXT: func4 in test.dll
+EXPAS-ARMAP-NEXT: test_NULL_THUNK_DATA in test.dll
 
 EXPAS-READOBJ:      File: test.dll
 EXPAS-READOBJ-NEXT: Format: COFF-import-file-ARM64EC

github-actions · 2024-03-11T21:14:18Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/lib/Object/ArchiveWriter.cpp

efriedma-quic

Generally looks okay, but I'll defer to @cjacek for approval.

llvm/lib/Object/ArchiveWriter.cpp

cjacek

LGTM, thanks!

As noted in <llvm#78537>, MSVC places import descriptors in both the EC and regular map - that PR moved the descriptors to ONLY the regular map, however this causes linking errors when linking as Arm64EC: ``` bcryptprimitives.lib(bcryptprimitives.dll) : error LNK2001: unresolved external symbol __IMPORT_DESCRIPTOR_bcryptprimitives (EC Symbol) ``` This change copies import descriptors from the regular map to the EC map, which fixes this linking error.

dpaoliello requested review from cjacek and efriedma-quic March 11, 2024 21:11

llvmbot added the llvm:binary-utilities label Mar 11, 2024

dpaoliello mentioned this pull request Mar 11, 2024

Backport fixes for ARM64EC import libraries #84590

Merged

dpaoliello force-pushed the impdesc branch from 98863a8 to a633311 Compare March 11, 2024 21:31

cjacek reviewed Mar 11, 2024

View reviewed changes

llvm/lib/Object/ArchiveWriter.cpp Outdated Show resolved Hide resolved

dpaoliello force-pushed the impdesc branch from a633311 to 3b7db0a Compare March 11, 2024 21:50

efriedma-quic reviewed Mar 11, 2024

View reviewed changes

llvm/lib/Object/ArchiveWriter.cpp Outdated Show resolved Hide resolved

dpaoliello force-pushed the impdesc branch from 3b7db0a to 650aa74 Compare March 12, 2024 00:13

Copy import descriptors to the EC Map

6e8d3ce

dpaoliello force-pushed the impdesc branch from 650aa74 to 6e8d3ce Compare March 12, 2024 16:32

dpaoliello requested a review from cjacek March 12, 2024 17:46

cjacek approved these changes Mar 12, 2024

View reviewed changes

dpaoliello merged commit 1a6ec90 into llvm:main Mar 12, 2024

dpaoliello deleted the impdesc branch March 12, 2024 21:10

cjacek mentioned this pull request Mar 15, 2024

[llvm-ar][Object][COFF] Add support for EC symbols to llvm-ar. #85230

Merged

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Arm64EC] Copy import descriptors to the EC Map #84834

[Arm64EC] Copy import descriptors to the EC Map #84834

dpaoliello commented Mar 11, 2024

llvmbot commented Mar 11, 2024

github-actions bot commented Mar 11, 2024 •

edited

Loading

efriedma-quic left a comment

cjacek left a comment

[Arm64EC] Copy import descriptors to the EC Map #84834

[Arm64EC] Copy import descriptors to the EC Map #84834

Conversation

dpaoliello commented Mar 11, 2024

llvmbot commented Mar 11, 2024

github-actions bot commented Mar 11, 2024 • edited Loading

efriedma-quic left a comment

Choose a reason for hiding this comment

cjacek left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2024 •

edited

Loading