[lld] [COFF] Align import address chunks to the pointer size

[mstorsjo]

Summary

This fixes cases on ARM64 when importing from more than one DLL, in case the imports from the first DLL ended up unaligned.

When fixing up a IMAGE_REL_ARM64_PAGEOFFSET_12L, which shifts the offset by the load/store size, check that the shift doesn't discard any bits. (This would also detect if the import address chunks were unaligned.)

[ruiu]

COFF/Chunks.cpp

186–188

It is better to use uint32_t so that you don't need to think about sign-extended shift.

187

Use llvm::isPowerOf2_32.

188

Error messages should start with lowercase letter.

[mstorsjo]

COFF/Chunks.cpp

186–188

Sure - do you want that change separately in a different change, or squashed into this one?

187

Doesn't that check a different thing? This tests if the lowest Size bits of Imm are nonzero, while llvm::isPowerOf2_32 checks if the value is a power of two?

[ruiu]

COFF/Chunks.cpp

186–188

It's probably better to do in a separate patch, even though I don't care if you do that as passed-by change with this patch.

187

Ah, that's right.

188

Please fix.

[mstorsjo]

Made the error message lower case (will change the variable type in a separate change).

[ruiu]

LGTM

[mstorsjo]

Updated with the right diff with the uint32_t change removed from here.

[ruiu]

COFF/Chunks.cpp

189–190

It is probably better to write this as applyArm64Imm(Off, Imm >> Size)

[mstorsjo]

COFF/Chunks.cpp

189–190

Addressing this in https://reviews.llvm.org/D35646 as well.

[yinma]

This comment was removed by yinma.

COFF/Chunks.cpp

 static void applyArm64Imm(uint8_t *Off, uint64_t Imm) {
   uint32_t Orig = read32le(Off);
   Imm += (Orig >> 10) & 0xFFF;
   Orig &= ~(0xFFF << 10);
   write32le(Off, Orig | ((Imm & 0xFFF) << 10));
 }
 
 static void applyArm64Ldr(uint8_t *Off, uint64_t Imm) {
-  int Size = read32le(Off) >> 30;
+  uint32_t Size = read32le(Off) >> 30;
+  if ((Imm & ((1 << Size) - 1)) != 0)
+    fatal("misaligned ldr/str offset");
   Imm >>= Size;
   applyArm64Imm(Off, Imm);
 }
 
 void SectionChunk::applyRelARM64(uint8_t *Off, uint16_t Type, OutputSection *OS,
                                  uint64_t S, uint64_t P) const {
   switch (Type) {
   case IMAGE_REL_ARM64_PAGEBASE_REL21: applyArm64Addr(Off, (S >> 12) - (P >> 12)); break;
   case IMAGE_REL_ARM64_PAGEOFFSET_12A: applyArm64Imm(Off, S & 0xfff); break;
   case IMAGE_REL_ARM64_PAGEOFFSET_12L: applyArm64Ldr(Off, S & 0xfff); break;

COFF/DLL.cpp

 private:
   StringRef Name;
   uint16_t Hint;
 };
 
 // A chunk for the import descriptor table.
 class LookupChunk : public Chunk {
 public:
-  explicit LookupChunk(Chunk *C) : HintName(C) {}
+  explicit LookupChunk(Chunk *C) : HintName(C) { Align = ptrSize(); }
   size_t getSize() const override { return ptrSize(); }
 
   void writeTo(uint8_t *Buf) const override {
     write32le(Buf + OutputSectionOff, HintName->getRVA());
   }
 
   Chunk *HintName;
 };
 
 // A chunk for the import descriptor table.
 // This chunk represent import-by-ordinal symbols.
 // See Microsoft PE/COFF spec 7.1. Import Header for details.
 class OrdinalOnlyChunk : public Chunk {
 public:
-  explicit OrdinalOnlyChunk(uint16_t V) : Ordinal(V) {}
+  explicit OrdinalOnlyChunk(uint16_t V) : Ordinal(V) { Align = ptrSize(); }
   size_t getSize() const override { return ptrSize(); }
 
   void writeTo(uint8_t *Buf) const override {
     // An import-by-ordinal slot has MSB 1 to indicate that
     // this is import-by-ordinal (and not import-by-name).
     if (Config->is64()) {
       write64le(Buf + OutputSectionOff, (1ULL << 63) | Ordinal);
     } else {
public:
   Defined *Imp = nullptr;
   Chunk *Desc = nullptr;
   Defined *Helper = nullptr;
 };
 
 // A chunk for the import descriptor table.
 class DelayAddressChunk : public Chunk {
 public:
-  explicit DelayAddressChunk(Chunk *C) : Thunk(C) {}
+  explicit DelayAddressChunk(Chunk *C) : Thunk(C) { Align = ptrSize(); }
   size_t getSize() const override { return ptrSize(); }
 
   void writeTo(uint8_t *Buf) const override {
     if (Config->is64()) {
       write64le(Buf + OutputSectionOff, Thunk->getRVA() + Config->ImageBase);
     } else {
       write32le(Buf + OutputSectionOff, Thunk->getRVA() + Config->ImageBase);
     }

test/COFF/Inputs/library2.def

+LIBRARY library2
+EXPORTS
+  function2

test/COFF/arm64-import2.test

+# REQUIRES: aarch64
+
+# RUN: yaml2obj < %s > %t.obj
+# RUN: llvm-objdump -d %t.obj | FileCheck %s -check-prefix BEFORE
+# RUN: lld-link /entry:main /subsystem:console /out:%t.exe %t.obj %p/Inputs/library-arm64.lib %p/Inputs/library2-arm64.lib
+# RUN: llvm-objdump -d %t.exe | FileCheck %s -check-prefix AFTER
+# RUN: llvm-readobj -coff-imports %t.exe | FileCheck %s -check-prefix IMPORTS
+
+# BEFORE: Disassembly of section .text:
+# BEFORE:        0:       00 00 00 94     bl      #0
+# BEFORE:        4:       00 00 00 94     bl      #0
+# BEFORE:        8:       c0 03 5f d6     ret
+
+# AFTER: Disassembly of section .text:
+# AFTER:  140001000:      03 00 00 94     bl      #12
+# AFTER:  140001004:      05 00 00 94     bl      #20
+# AFTER:  140001008:      c0 03 5f d6     ret
+# AFTER:  14000100c:      10 00 00 b0     adrp    x16, #4096
+# AFTER:  140001010:      10 32 40 f9     ldr     x16, [x16, #96]
+# AFTER:  140001014:      00 02 1f d6     br      x16
+# AFTER:  140001018:      10 00 00 b0     adrp    x16, #4096
+# AFTER:  14000101c:      10 3a 40 f9     ldr     x16, [x16, #112]
+# AFTER:  140001020:      00 02 1f d6     br      x16
+
+# IMPORTS: Import {
+# IMPORTS:   Name: library.dll
+# IMPORTS:   ImportLookupTableRVA: 0x2040
+# IMPORTS:   ImportAddressTableRVA: 0x2060
+# IMPORTS:   Symbol: function (2)
+# IMPORTS: }
+# IMPORTS: Import {
+# IMPORTS:   Name: library2.dll
+# IMPORTS:   ImportLookupTableRVA: 0x2050
+# IMPORTS:   ImportAddressTableRVA: 0x2070
+# IMPORTS:   Symbol: function2 (0)
+# IMPORTS: }
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_ARM64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       4
+    SectionData:     0000009400000094C0035FD6
+    Relocations:
+      - VirtualAddress:  0
+        SymbolName:      function
+        Type:            3
+      - VirtualAddress:  4
+        SymbolName:      function2
+        Type:            3
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          12
+      NumberOfRelocations: 2
+      NumberOfLinenumbers: 0
+      CheckSum:        1438860354
+      Number:          1
+  - Name:            main
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            function
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            function2
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...