[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

[Axel Y. Rivera via Phabricator via llvm-commits]

ayrivera 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.
>
> 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 <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.
>
> 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 <https://bugs.llvm.org/show_bug.cgi?id=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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96619