Skip to content
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

[llvm-lib][Object][COFF] Use ARM64 machine type for import library descriptor objects. #78537

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Jan 18, 2024

This is what MSVC lib.exe does. I changed machine type stored in ObjectFactory to native type, while the actual type is passed by argument to functions that need it. Doing it that way requires a few more changes, but prepares us for later -defArm64Native support, which will require import objects to be a mix of ARM64 and ARM64EC types (my prototype looks like this: cjacek@8e7dffe).

MSVC puts descriptor symbols in both regular archive map and EC map, this is not implemented by this PR. That's handled by archive writer, would require some special cases and doesn't seem useful in practice.

cc @bylaws

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 18, 2024

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

Author: Jacek Caban (cjacek)

Changes

This is what MSVC lib.exe does. I changed machine type stored in ObjectFactory to native type, while the actual type is passed by argument to functions that need it. Doing it that way requires a few more changes, but prepares us for later -defArm64Native support, which will require import objects to be a mix of ARM64 and ARM64EC types (my prototype looks like this: cjacek@8e7dffe).

MSVC puts descriptor symbols in both regular archive map and EC map, this is not implemented by this PR. That's handled by archive writer, would require some special cases and doesn't seem useful in practice.

cc @bylaws


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

2 Files Affected:

  • (modified) llvm/lib/Object/COFFImportFile.cpp (+35-26)
  • (modified) llvm/test/tools/llvm-lib/arm64ec-implib.test (+8-6)
diff --git a/llvm/lib/Object/COFFImportFile.cpp b/llvm/lib/Object/COFFImportFile.cpp
index b60e32f498eb5a..60556c149bf735 100644
--- a/llvm/lib/Object/COFFImportFile.cpp
+++ b/llvm/lib/Object/COFFImportFile.cpp
@@ -150,7 +150,7 @@ namespace {
 class ObjectFactory {
   using u16 = support::ulittle16_t;
   using u32 = support::ulittle32_t;
-  MachineTypes Machine;
+  MachineTypes NativeMachine;
   BumpPtrAllocator Alloc;
   StringRef ImportName;
   StringRef Library;
@@ -159,7 +159,7 @@ class ObjectFactory {
 
 public:
   ObjectFactory(StringRef S, MachineTypes M)
-      : Machine(M), ImportName(S), Library(llvm::sys::path::stem(S)),
+      : NativeMachine(M), ImportName(S), Library(llvm::sys::path::stem(S)),
         ImportDescriptorSymbolName(("__IMPORT_DESCRIPTOR_" + Library).str()),
         NullThunkSymbolName(("\x7f" + Library + "_NULL_THUNK_DATA").str()) {}
 
@@ -182,10 +182,14 @@ class ObjectFactory {
   // Create a short import file which is described in PE/COFF spec 7. Import
   // Library Format.
   NewArchiveMember createShortImport(StringRef Sym, uint16_t Ordinal,
-                                     ImportType Type, ImportNameType NameType);
+                                     ImportType Type, ImportNameType NameType,
+                                     MachineTypes Machine);
 
   // Create a weak external file which is described in PE/COFF Aux Format 3.
-  NewArchiveMember createWeakExternal(StringRef Sym, StringRef Weak, bool Imp);
+  NewArchiveMember createWeakExternal(StringRef Sym, StringRef Weak, bool Imp,
+                                      MachineTypes Machine);
+
+  bool is64Bit() const { return COFF::is64Bit(NativeMachine); }
 };
 } // namespace
 
@@ -197,7 +201,7 @@ ObjectFactory::createImportDescriptor(std::vector<uint8_t> &Buffer) {
 
   // COFF Header
   coff_file_header Header{
-      u16(Machine),
+      u16(NativeMachine),
       u16(NumberOfSections),
       u32(0),
       u32(sizeof(Header) + (NumberOfSections * sizeof(coff_section)) +
@@ -208,7 +212,7 @@ ObjectFactory::createImportDescriptor(std::vector<uint8_t> &Buffer) {
           (ImportName.size() + 1)),
       u32(NumberOfSymbols),
       u16(0),
-      u16(is64Bit(Machine) ? C_Invalid : IMAGE_FILE_32BIT_MACHINE),
+      u16(is64Bit() ? C_Invalid : IMAGE_FILE_32BIT_MACHINE),
   };
   append(Buffer, Header);
 
@@ -250,11 +254,11 @@ ObjectFactory::createImportDescriptor(std::vector<uint8_t> &Buffer) {
 
   const coff_relocation RelocationTable[NumberOfRelocations] = {
       {u32(offsetof(coff_import_directory_table_entry, NameRVA)), u32(2),
-       u16(getImgRelRelocation(Machine))},
+       u16(getImgRelRelocation(NativeMachine))},
       {u32(offsetof(coff_import_directory_table_entry, ImportLookupTableRVA)),
-       u32(3), u16(getImgRelRelocation(Machine))},
+       u32(3), u16(getImgRelRelocation(NativeMachine))},
       {u32(offsetof(coff_import_directory_table_entry, ImportAddressTableRVA)),
-       u32(4), u16(getImgRelRelocation(Machine))},
+       u32(4), u16(getImgRelRelocation(NativeMachine))},
   };
   append(Buffer, RelocationTable);
 
@@ -336,7 +340,7 @@ ObjectFactory::createNullImportDescriptor(std::vector<uint8_t> &Buffer) {
 
   // COFF Header
   coff_file_header Header{
-      u16(Machine),
+      u16(NativeMachine),
       u16(NumberOfSections),
       u32(0),
       u32(sizeof(Header) + (NumberOfSections * sizeof(coff_section)) +
@@ -344,7 +348,7 @@ ObjectFactory::createNullImportDescriptor(std::vector<uint8_t> &Buffer) {
           sizeof(coff_import_directory_table_entry)),
       u32(NumberOfSymbols),
       u16(0),
-      u16(is64Bit(Machine) ? C_Invalid : IMAGE_FILE_32BIT_MACHINE),
+      u16(is64Bit() ? C_Invalid : IMAGE_FILE_32BIT_MACHINE),
   };
   append(Buffer, Header);
 
@@ -393,11 +397,11 @@ ObjectFactory::createNullImportDescriptor(std::vector<uint8_t> &Buffer) {
 NewArchiveMember ObjectFactory::createNullThunk(std::vector<uint8_t> &Buffer) {
   const uint32_t NumberOfSections = 2;
   const uint32_t NumberOfSymbols = 1;
-  uint32_t VASize = is64Bit(Machine) ? 8 : 4;
+  uint32_t VASize = is64Bit() ? 8 : 4;
 
   // COFF Header
   coff_file_header Header{
-      u16(Machine),
+      u16(NativeMachine),
       u16(NumberOfSections),
       u32(0),
       u32(sizeof(Header) + (NumberOfSections * sizeof(coff_section)) +
@@ -407,7 +411,7 @@ NewArchiveMember ObjectFactory::createNullThunk(std::vector<uint8_t> &Buffer) {
           VASize),
       u32(NumberOfSymbols),
       u16(0),
-      u16(is64Bit(Machine) ? C_Invalid : IMAGE_FILE_32BIT_MACHINE),
+      u16(is64Bit() ? C_Invalid : IMAGE_FILE_32BIT_MACHINE),
   };
   append(Buffer, Header);
 
@@ -422,8 +426,7 @@ NewArchiveMember ObjectFactory::createNullThunk(std::vector<uint8_t> &Buffer) {
        u32(0),
        u16(0),
        u16(0),
-       u32((is64Bit(Machine) ? IMAGE_SCN_ALIGN_8BYTES
-                             : IMAGE_SCN_ALIGN_4BYTES) |
+       u32((is64Bit() ? IMAGE_SCN_ALIGN_8BYTES : IMAGE_SCN_ALIGN_4BYTES) |
            IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
            IMAGE_SCN_MEM_WRITE)},
       {{'.', 'i', 'd', 'a', 't', 'a', '$', '4'},
@@ -436,8 +439,7 @@ NewArchiveMember ObjectFactory::createNullThunk(std::vector<uint8_t> &Buffer) {
        u32(0),
        u16(0),
        u16(0),
-       u32((is64Bit(Machine) ? IMAGE_SCN_ALIGN_8BYTES
-                             : IMAGE_SCN_ALIGN_4BYTES) |
+       u32((is64Bit() ? IMAGE_SCN_ALIGN_8BYTES : IMAGE_SCN_ALIGN_4BYTES) |
            IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
            IMAGE_SCN_MEM_WRITE)},
   };
@@ -445,12 +447,12 @@ NewArchiveMember ObjectFactory::createNullThunk(std::vector<uint8_t> &Buffer) {
 
   // .idata$5, ILT
   append(Buffer, u32(0));
-  if (is64Bit(Machine))
+  if (is64Bit())
     append(Buffer, u32(0));
 
   // .idata$4, IAT
   append(Buffer, u32(0));
-  if (is64Bit(Machine))
+  if (is64Bit())
     append(Buffer, u32(0));
 
   // Symbol Table
@@ -475,7 +477,8 @@ NewArchiveMember ObjectFactory::createNullThunk(std::vector<uint8_t> &Buffer) {
 NewArchiveMember ObjectFactory::createShortImport(StringRef Sym,
                                                   uint16_t Ordinal,
                                                   ImportType ImportType,
-                                                  ImportNameType NameType) {
+                                                  ImportNameType NameType,
+                                                  MachineTypes Machine) {
   size_t ImpSize = ImportName.size() + Sym.size() + 2; // +2 for NULs
   size_t Size = sizeof(coff_import_header) + ImpSize;
   char *Buf = Alloc.Allocate<char>(Size);
@@ -501,7 +504,8 @@ NewArchiveMember ObjectFactory::createShortImport(StringRef Sym,
 }
 
 NewArchiveMember ObjectFactory::createWeakExternal(StringRef Sym,
-                                                   StringRef Weak, bool Imp) {
+                                                   StringRef Weak, bool Imp,
+                                                   MachineTypes Machine) {
   std::vector<uint8_t> Buffer;
   const uint32_t NumberOfSections = 1;
   const uint32_t NumberOfSymbols = 5;
@@ -585,8 +589,11 @@ Error writeImportLibrary(StringRef ImportName, StringRef Path,
                          ArrayRef<COFFShortExport> Exports,
                          MachineTypes Machine, bool MinGW) {
 
+  MachineTypes NativeMachine =
+      isArm64EC(Machine) ? IMAGE_FILE_MACHINE_ARM64 : Machine;
+
   std::vector<NewArchiveMember> Members;
-  ObjectFactory OF(llvm::sys::path::filename(ImportName), Machine);
+  ObjectFactory OF(llvm::sys::path::filename(ImportName), NativeMachine);
 
   std::vector<uint8_t> ImportDescriptor;
   Members.push_back(OF.createImportDescriptor(ImportDescriptor));
@@ -620,13 +627,15 @@ Error writeImportLibrary(StringRef ImportName, StringRef Path,
       return Name.takeError();
 
     if (!E.AliasTarget.empty() && *Name != E.AliasTarget) {
-      Members.push_back(OF.createWeakExternal(E.AliasTarget, *Name, false));
-      Members.push_back(OF.createWeakExternal(E.AliasTarget, *Name, true));
+      Members.push_back(
+          OF.createWeakExternal(E.AliasTarget, *Name, false, Machine));
+      Members.push_back(
+          OF.createWeakExternal(E.AliasTarget, *Name, true, Machine));
       continue;
     }
 
     Members.push_back(
-        OF.createShortImport(*Name, E.Ordinal, ImportType, NameType));
+        OF.createShortImport(*Name, E.Ordinal, ImportType, NameType, Machine));
   }
 
   return writeArchive(Path, Members, SymtabWritingMode::NormalSymtab,
diff --git a/llvm/test/tools/llvm-lib/arm64ec-implib.test b/llvm/test/tools/llvm-lib/arm64ec-implib.test
index 3c74b4bf660765..1071f8ca877838 100644
--- a/llvm/test/tools/llvm-lib/arm64ec-implib.test
+++ b/llvm/test/tools/llvm-lib/arm64ec-implib.test
@@ -5,28 +5,30 @@ RUN: llvm-lib -machine:arm64ec -def:test.def -out:test.lib
 
 RUN: llvm-nm --print-armap test.lib | FileCheck -check-prefix=ARMAP %s
 
-ARMAP:      Archive EC map
+ARMAP:      Archive map
 ARMAP-NEXT: __IMPORT_DESCRIPTOR_test in test.dll
 ARMAP-NEXT: __NULL_IMPORT_DESCRIPTOR in test.dll
+ARMAP-NEXT: test_NULL_THUNK_DATA in test.dll
+ARMAP-EMPTY:
+ARMAP-NEXT: Archive EC map
 ARMAP-NEXT: __imp_dataexp in test.dll
 ARMAP-NEXT: __imp_funcexp in test.dll
 ARMAP-NEXT: funcexp in test.dll
-ARMAP-NEXT: test_NULL_THUNK_DATA in test.dll
 
 RUN: llvm-readobj test.lib | FileCheck -check-prefix=READOBJ %s
 
 READOBJ:      File: test.lib(test.dll)
-READOBJ-NEXT: Format: COFF-ARM64EC
+READOBJ-NEXT: Format: COFF-ARM64
 READOBJ-NEXT: Arch: aarch64
 READOBJ-NEXT: AddressSize: 64bit
 READOBJ-EMPTY:
 READOBJ-NEXT: File: test.lib(test.dll)
-READOBJ-NEXT: Format: COFF-ARM64EC
+READOBJ-NEXT: Format: COFF-ARM64
 READOBJ-NEXT: Arch: aarch64
 READOBJ-NEXT: AddressSize: 64bit
 READOBJ-EMPTY:
 READOBJ-NEXT: File: test.lib(test.dll)
-READOBJ-NEXT: Format: COFF-ARM64EC
+READOBJ-NEXT: Format: COFF-ARM64
 READOBJ-NEXT: Arch: aarch64
 READOBJ-NEXT: AddressSize: 64bit
 READOBJ-EMPTY:
@@ -38,7 +40,7 @@ READOBJ-NEXT: Symbol: __imp_funcexp
 READOBJ-NEXT: Symbol: funcexp
 READOBJ-EMPTY:
 READOBJ-NEXT: File: test.dll
-READOBJ-NEXT: Format: COFF-import-file-ARM64EC
+READOBJ-NEXT: Format: COFF-import-file-ARM64
 READOBJ-NEXT: Type: data
 READOBJ-NEXT: Name type: name
 READOBJ-NEXT: Symbol: __imp_dataexp


RUN: llvm-readobj test.lib | FileCheck -check-prefix=READOBJ %s

READOBJ: File: test.lib(test.dll)
READOBJ-NEXT: Format: COFF-ARM64EC
READOBJ-NEXT: Format: COFF-ARM64
Copy link
Member

Choose a reason for hiding this comment

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

How about READOBJ-NEXT: Format: COFF-ARM64{{$}} to ensure the pattern will not match COFF-ARM64EC ?

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, thanks. I pushed a fix.

@cjacek cjacek merged commit 80fcf48 into llvm:main Jan 18, 2024
3 of 4 checks passed
@cjacek cjacek deleted the implib-native-arch branch January 18, 2024 23:46
dpaoliello added a commit that referenced this pull request Mar 12, 2024
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.
dpaoliello added a commit to dpaoliello/llvm-project that referenced this pull request Mar 12, 2024
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 added a commit to dpaoliello/llvm-project that referenced this pull request Mar 13, 2024
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 added a commit to dpaoliello/llvm-project that referenced this pull request Mar 13, 2024
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 added a commit to dpaoliello/llvm-project that referenced this pull request Mar 15, 2024
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.
tstellar pushed a commit to dpaoliello/llvm-project that referenced this pull request Mar 16, 2024
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.
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.

None yet

4 participants