This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] [COFF] Properly produce cross-section relative relocations
ClosedPublic

Authored by mstorsjo on Mar 30 2021, 3:26 AM.

Details

Summary

This fixes breakage on Windows/ARM64 after D94355.

Modelled after the corresponding code for X86; not entirely familiar
with those aspects of that layer otherwise.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 30 2021, 3:26 AM
mstorsjo requested review of this revision.Mar 30 2021, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 3:26 AM
gulfem added a subscriber: gulfem.Mar 30 2021, 12:11 PM
rnk accepted this revision.Apr 13 2021, 1:52 PM

lgtm, although I don't have deep expertise to do this with great confidence.

This revision is now accepted and ready to land.Apr 13 2021, 1:52 PM
In D99572#2686887, @rnk wrote:

lgtm, although I don't have deep expertise to do this with great confidence.

Yeah it's a bit nonobvious code, but at least it's essentially equivalent to what we have on X86 (which I'd assume has been a fair bit more battle tested than these arch/OS combos).

In D99572#2686887, @rnk wrote:

lgtm, although I don't have deep expertise to do this with great confidence.

Yeah it's a bit nonobvious code, but at least it's essentially equivalent to what we have on X86 (which I'd assume has been a fair bit more battle tested than these arch/OS combos).

Just for reference, these codepaths seems to originate in ed16477cb9f907bd3525ae99d787c5d448f8dc0b, and was refactored into its current place in 1da4529b15ed80e5d6c0b21d3ac46538fbcad87f and touched a bit more in 58173b97209bfc907dd0b47ecfc5ab7c1fe1f036.

This revision was landed with ongoing or failed builds.Apr 14 2021, 2:57 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 5 2021, 5:17 AM

Sounds like the commit that turned this on by default broke Edge on win/arm64: https://bugs.chromium.org/p/chromium/issues/detail?id=1204788

I'm guessing we'll get a standalone repro for that in time, but to save others time tracking down the bug from the enablement, maybe you want to undo it for now?

(Not this change here, but https://github.com/llvm/llvm-project/commit/57b259a852a6383880f5d0875d848420bb3c2945 )

Bug opened to track the Windows arm64 thing (only a full reproducer available at the moment). https://bugs.llvm.org/show_bug.cgi?id=50227

Sounds like the commit that turned this on by default broke Edge on win/arm64: https://bugs.chromium.org/p/chromium/issues/detail?id=1204788

I'm guessing we'll get a standalone repro for that in time, but to save others time tracking down the bug from the enablement, maybe you want to undo it for now?

(Not this change here, but https://github.com/llvm/llvm-project/commit/57b259a852a6383880f5d0875d848420bb3c2945 )

Ok, reverted it for now. I'd be interested to hear once you're able to pin down the issue closer, whether it's an issue with the relative relocations themselves, or with the rewrite done by the pass.

Sounds like the commit that turned this on by default broke Edge on win/arm64: https://bugs.chromium.org/p/chromium/issues/detail?id=1204788

I'm guessing we'll get a standalone repro for that in time, but to save others time tracking down the bug from the enablement, maybe you want to undo it for now?

(Not this change here, but https://github.com/llvm/llvm-project/commit/57b259a852a6383880f5d0875d848420bb3c2945 )

Ok, reverted it for now. I'd be interested to hear once you're able to pin down the issue closer, whether it's an issue with the relative relocations themselves, or with the rewrite done by the pass.

Also CC @gulfem regarding the pass itself.