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

Sat May 8 12:50:41 PDT 2021

mstorsjo added a comment.

In D96619#2746012 <https://reviews.llvm.org/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).

================
Comment at: lld/test/COFF/reloc-x64-add32.s:1
+# REQUIRES: x86, system-windows
+# RUN: llvm-mc %s -filetype=obj -o %t.obj -triple=x86_64-windows-msvc
----------------
`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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96619/new/

https://reviews.llvm.org/D96619