[LLD][COFF] PR49068: Include the IMAGE_REL_BASED_HIGHLOW relocation base type when the machine is 64 bits and the relocation type is ADDR32

[ayrivera]

Summary

The COFF driver produces an ABSOLUTE relocation base for an ADDR32
relocation type and the system is 64 bits (machine=AMD64). The
relocation information won't be added in the output and could
produce an incorrect address access during run-time. This change
set checks if the relocation type is IMAGE_REL_AMD64_ADDR32 and
if so, adds the relocated symbol as IMAGE_REL_BASED_HIGHLOW base.

[rnk]

This is the oldest unread message in my inbox. I added other LLD COFF people who may be able to establish the correct behavior.

[ayrivera]

In D96619#2745467, @rnk wrote:

This is the oldest unread message in my inbox. I added other LLD COFF people who may be able to establish the correct behavior.

Thanks for the follow up Reid Kleckner.

[aganea]

@ayrivera : Would you mind please giving a bit more context about your usage, and why do you need this to work?

I think this patch only partially solves the "exposed"/buggy part of the issue. A bit more would be needed if we want to be correct.

When linking your test .OBJ with MSVC link.exe, we get:

>link simple.obj
Microsoft (R) Incremental Linker Version 14.28.29914.0
Copyright (C) Microsoft Corporation.  All rights reserved.

simple.obj : error LNK2017: 'ADDR32' relocation to 'arr' invalid without /LARGEADDRESSAWARE:NO
LINK : fatal error LNK1165: link failed because of fixup errors

The documentation for that error code indicates that there are several constraints when using 32-bit addressing in 64-bit images: https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2017?view=msvc-160

• For a starter, LLD does not display an equivalent error when linking simple.obj without the proper options.
• If using /LARGEADDRESSAWARE:NO like suggested by Microsoft, it seems LLD doesn't disable /HIGHENTROPYVA (it is still on)
• The doc seems to suggest to use /FIXED in this scenario, but link.exe seems happy without. However if the image is rellocated, the 32-bit addressing might not work?
• The default base when using /LARGEADDRESSAWARE:NO should probably be 0x400000, but LLD still generates 0x140000000
• Finally, the doc suggests to "limit" the image to 3 GB, but the PE/spec mentions that the limit for a PE image size is already 2 GB: https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx (section 3.4) so I think there's nothing to do for this.

I feel the above issues should be fixed/mitigated if we wanted to land this patch.

Subsidiary question, do we need to fix llvm/lib/Object/RelocationResolver.cpp as well? That is, add support for IMAGE_REL_AMD64_ADDR32 in supportsCOFFX86_64() and resolveCOFFX86_64()?

[mstorsjo]

In D96619#2746012, @aganea wrote:

@ayrivera : Would you mind please giving a bit more context about your usage, and why do you need this to work?

I think this patch only partially solves the "exposed"/buggy part of the issue. A bit more would be needed if we want to be correct.
When linking your test .OBJ with MSVC link.exe, we get:

>link simple.obj
Microsoft (R) Incremental Linker Version 14.28.29914.0
Copyright (C) Microsoft Corporation.  All rights reserved.

simple.obj : error LNK2017: 'ADDR32' relocation to 'arr' invalid without /LARGEADDRESSAWARE:NO
LINK : fatal error LNK1165: link failed because of fixup errors

The documentation for that error code indicates that there are several constraints when using 32-bit addressing in 64-bit images: https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2017?view=msvc-160

• For a starter, LLD does not display an equivalent error when linking simple.obj without the proper options.
• If using /LARGEADDRESSAWARE:NO like suggested by Microsoft, it seems LLD doesn't disable /HIGHENTROPYVA (it is still on)
• The doc seems to suggest to use /FIXED in this scenario, but link.exe seems happy without. However if the image is rellocated, the 32-bit addressing might not work?
• The default base when using /LARGEADDRESSAWARE:NO should probably be 0x400000, but LLD still generates 0x140000000
• Finally, the doc suggests to "limit" the image to 3 GB, but the PE/spec mentions that the limit for a PE image size is already 2 GB: https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx (section 3.4) so I think there's nothing to do for this.

I feel the above issues should be fixed/mitigated if we wanted to land this patch.

I kind of disagree on that part - I maybe wouldn't block this patch in itself on that: If given an object file with such a relocation today, lld does the wrong thing, silently. With the patch it would do the right thing - so landing the patch would be one incremental step towards this target. But it would indeed be good to address those points too.

Secondly, ARM64 also has got the same kind of relocation, IMAGE_REL_ARM64_ADDR32. But link.exe says LINK : warning LNK4075: ignoring '/LARGEADDRESSAWARE:NO' due to '/ARM64' specification if one tries to specify that option - but if linking an object file that contains IMAGE_REL_ARM64_ADDR32 it doesn't actually give the warning you quoted above.

Subsidiary question, do we need to fix llvm/lib/Object/RelocationResolver.cpp as well? That is, add support for IMAGE_REL_AMD64_ADDR32 in supportsCOFFX86_64() and resolveCOFFX86_64()?

I don't think that's strictly necessary - those funtions only handle a small subset of available relocations anyway, in practice the ones that are needed for parsing dwarf debug info from an unrelocated object file (for mapping e.g. undefined references to a source file+line).

lld/test/COFF/reloc-x64-add32.s

2

system-windows isn't needed here - we can assemble and link things on any platform, and nothing of the test should require actually running the compilation on windows.

[ayrivera]

In D96619#2746012, @aganea wrote:

@ayrivera : Would you mind please giving a bit more context about your usage, and why do you need this to work?

I think this patch only partially solves the "exposed"/buggy part of the issue. A bit more would be needed if we want to be correct.

When linking your test .OBJ with MSVC link.exe, we get:

>link simple.obj
Microsoft (R) Incremental Linker Version 14.28.29914.0
Copyright (C) Microsoft Corporation.  All rights reserved.

simple.obj : error LNK2017: 'ADDR32' relocation to 'arr' invalid without /LARGEADDRESSAWARE:NO
LINK : fatal error LNK1165: link failed because of fixup errors

The documentation for that error code indicates that there are several constraints when using 32-bit addressing in 64-bit images: https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2017?view=msvc-160

• For a starter, LLD does not display an equivalent error when linking simple.obj without the proper options.
• If using /LARGEADDRESSAWARE:NO like suggested by Microsoft, it seems LLD doesn't disable /HIGHENTROPYVA (it is still on)
• The doc seems to suggest to use /FIXED in this scenario, but link.exe seems happy without. However if the image is rellocated, the 32-bit addressing might not work?
• The default base when using /LARGEADDRESSAWARE:NO should probably be 0x400000, but LLD still generates 0x140000000
• Finally, the doc suggests to "limit" the image to 3 GB, but the PE/spec mentions that the limit for a PE image size is already 2 GB: https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx (section 3.4) so I think there's nothing to do for this.

I feel the above issues should be fixed/mitigated if we wanted to land this patch.

Subsidiary question, do we need to fix llvm/lib/Object/RelocationResolver.cpp as well? That is, add support for IMAGE_REL_AMD64_ADDR32 in supportsCOFFX86_64() and resolveCOFFX86_64()?

In D96619#2746012, @aganea wrote:

@ayrivera : Would you mind please giving a bit more context about your usage, and why do you need this to work?

I think this patch only partially solves the "exposed"/buggy part of the issue. A bit more would be needed if we want to be correct.

When linking your test .OBJ with MSVC link.exe, we get:

>link simple.obj
Microsoft (R) Incremental Linker Version 14.28.29914.0
Copyright (C) Microsoft Corporation.  All rights reserved.

simple.obj : error LNK2017: 'ADDR32' relocation to 'arr' invalid without /LARGEADDRESSAWARE:NO
LINK : fatal error LNK1165: link failed because of fixup errors

The documentation for that error code indicates that there are several constraints when using 32-bit addressing in 64-bit images: https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2017?view=msvc-160

• For a starter, LLD does not display an equivalent error when linking simple.obj without the proper options.
• If using /LARGEADDRESSAWARE:NO like suggested by Microsoft, it seems LLD doesn't disable /HIGHENTROPYVA (it is still on)
• The doc seems to suggest to use /FIXED in this scenario, but link.exe seems happy without. However if the image is rellocated, the 32-bit addressing might not work?
• The default base when using /LARGEADDRESSAWARE:NO should probably be 0x400000, but LLD still generates 0x140000000
• Finally, the doc suggests to "limit" the image to 3 GB, but the PE/spec mentions that the limit for a PE image size is already 2 GB: https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx (section 3.4) so I think there's nothing to do for this.

I feel the above issues should be fixed/mitigated if we wanted to land this patch.

Subsidiary question, do we need to fix llvm/lib/Object/RelocationResolver.cpp as well? That is, add support for IMAGE_REL_AMD64_ADDR32 in supportsCOFFX86_64() and resolveCOFFX86_64()?

Hi @aganea ,

Thanks for pointing at these missing pieces. I noticed the problem when I was testing some inline ASM instructions. There is a ticket already for the silent issue related to /LARGEADDRESSAWARE:NO (32669). Although @mstorsjo prefers to upload this patch first, eventually we will need to address all these issues. I agree to fix these problems first in order to upload this patch.

[aganea]

Agreed with @mstorsjo, let's go ahead with this patch and we'll treat PR32669 separately?

[aganea]

LGTM. Please address the system-windows comment below before landing.

[ayrivera]

Fix for the observation made by @mstorsjo

[ayrivera]

Hi @aganea and @mstorsjo ,

The test case was fixed. Since I don't have commit access, can you commit the changes for me? Thanks for your help.

[mstorsjo]

In D96619#2773722, @ayrivera wrote:

Hi @aganea and @mstorsjo ,

The test case was fixed. Since I don't have commit access, can you commit the changes for me? Thanks for your help.

I can do that later today. Can you provide your preferred git author line, i.e. “Real Name <email@address>”?

[ayrivera]

In D96619#2773762, @mstorsjo wrote:

In D96619#2773722, @ayrivera wrote:

Hi @aganea and @mstorsjo ,

The test case was fixed. Since I don't have commit access, can you commit the changes for me? Thanks for your help.

I can do that later today. Can you provide your preferred git author line, i.e. “Real Name <email@address>”?

Thanks @mstorsjo, please use this:

"Axel Y. Rivera <[e-mail]>"

[ayrivera]

Thanks for uploading the changes @mstorsjo .

lld/COFF/Chunks.cpp

void SectionChunk::addAssociative(SectionChunk *child) {
   child->assocChildren = next;
 }
 
 static uint8_t getBaserelType(const coff_relocation &rel) {
   switch (config->machine) {
   case AMD64:
     if (rel.Type == IMAGE_REL_AMD64_ADDR64)
       return IMAGE_REL_BASED_DIR64;
+    if (rel.Type == IMAGE_REL_AMD64_ADDR32)
+      return IMAGE_REL_BASED_HIGHLOW;
     return IMAGE_REL_BASED_ABSOLUTE;
   case I386:
     if (rel.Type == IMAGE_REL_I386_DIR32)
       return IMAGE_REL_BASED_HIGHLOW;
     return IMAGE_REL_BASED_ABSOLUTE;
   case ARMNT:
     if (rel.Type == IMAGE_REL_ARM_ADDR32)
       return IMAGE_REL_BASED_HIGHLOW;

lld/test/COFF/reloc-x64-add32.s

+# REQUIRES: x86
+# RUN: llvm-mc %s -filetype=obj -o %t.obj -triple=x86_64-windows-msvc
+# RUN: lld-link /entry:main /out:%t.exe /LARGEADDRESSAWARE:NO %t.obj
+# RUN: llvm-readobj --coff-basereloc %t.exe | FileCheck %s --check-prefix=CHECKPASS
+
+# This test case checks that the "ADDR32" relocation symbol is collected
+# when linking a 64bit executable, and the output contains the HIGHLOW
+# relocated symbol.
+
+# Check that the HIGHLOW relocation base type is in the generated executable
+# CHECKPASS: Format: COFF-x86-64
+# CHECKPASS: Arch: x86_64
+# CHECKPASS: AddressSize: 64bit
+# CHECKPASS: BaseReloc
+# CHECKPASS:   Entry {
+# CHECKPASS-NEXT:     Type: HIGHLOW
+
+    .text
+    .def     main;
+    .scl    2;
+    .type   32;
+    .endef
+    .intel_syntax noprefix
+    .globl  main
+    .p2align        4, 0x90
+
+main: # @main
+    sub rsp, 40
+
+    mov dword ptr [rip + arr + 24], 7
+
+    mov eax, 1
+    mov ecx, 20
+    mov eax, dword ptr [rcx + 4 * rax + arr]
+
+    ret
+
+    .globl  arr
+    .p2align        4
+arr:
+    .zero 40
+