This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ayrivera on Feb 12 2021, 10:47 AM.

Details

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.

Diff Detail

Event Timeline

ayrivera requested review of this revision.Feb 12 2021, 10:47 AM
ayrivera created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 10:47 AM
rnk edited reviewers, added: mstorsjo, aganea; removed: rnk, ruiu.May 7 2021, 3:04 PM
rnk added a subscriber: 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.

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 added a comment.EditedMay 8 2021, 6:43 AM

@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()?

@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
1

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 : 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()?

@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.

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

aganea accepted this revision.May 21 2021, 5:21 AM

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

This revision is now accepted and ready to land.May 21 2021, 5:21 AM
ayrivera updated this revision to Diff 347019.May 21 2021, 7:06 AM

Fix for the observation made by @mstorsjo

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.

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>”?

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 <axel.y.rivera@intel.com>"

Thanks for uploading the changes @mstorsjo .