This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MC][Thumb] Revert relocation for some pc-relative fixups.
ClosedPublic

Authored by MarkMurrayARM on Apr 16 2020, 7:46 AM.

Details

Summary

This commit reverts https://reviews.llvm.org/D75039. Concensus appears to
be in favour of assembly-time resolution of these ADR and LDR relocations,
in line with GNU.

Diff Detail

Event Timeline

MarkMurrayARM created this revision.Apr 16 2020, 7:46 AM

That is my understanding too from my comment in https://reviews.llvm.org/D72892#1923502
I have removed the tests from LLD that used the relocations coming from these instructions and have replaced them with .inst and .reloc. Will be worth making sure that ninja check-lld passes with this change applies. I'm happy for this to be applied. Will need to check with the other reviewers to make sure they are happy.

MaskRay added a comment.EditedApr 16 2020, 9:05 AM

Interestingly I received some feedback from my earlier GNU as bug report yesterday (https://sourceware.org/bugzilla/show_bug.cgi?id=25406). GNU as will not emit relocations.
I still want to check whether they think rejecting non-STB_LOCAL labels in a potentially preemptible context is acceptable.
I remember that we've seen such reports twice. One is some FreeBSD ARM folks (but at least another two FreeBSD folks told me I can persist). One is a Linux kernel assembly which may have been fixed now. I should add that allowing some Linux kernel in LLVM 9/10 while rejecting some in LLVM 10/11 is ok. There are still a few assembly files not assembled with MC. So being more rigid than GNU as may not cause churn/perceivable problems to Linux kernel builders.

For the patch itself, I am now ok with either way. But please make use of this opportunity to clean up/enhance tests, rather than perform a simple revert.

MaskRay added a comment.EditedApr 16 2020, 10:06 AM

[ARM][MD] Revert relocation for some pc-relative fixups.

Did you mean MC? Also please mention Thumb. It greatly improves the information conveyed by the commit subject with just 6 more characters.

Improve commit message slightly.

MarkMurrayARM retitled this revision from [ARM][MD] Revert relocation for some pc-relative fixups. to [ARM][MC][Thumb] Revert relocation for some pc-relative fixups..Apr 17 2020, 2:04 AM

LGTM, but I'll defer the approval to Peter.

llvm/test/MC/ARM/thumb1-relax-adr.s
9

delete empty line

llvm/test/MC/ARM/thumb1-relax-ldrlit.s
9

delete empty line

psmith accepted this revision.Apr 17 2020, 11:36 AM

LGTM with MaskRay's test comments addressed.

This revision is now accepted and ready to land.Apr 17 2020, 11:36 AM
MaskRay accepted this revision.Apr 17 2020, 6:46 PM

Address review nits.

MarkMurrayARM marked 2 inline comments as done.Apr 20 2020, 1:39 AM
This revision was automatically updated to reflect the committed changes.

This broke check-lld everywhere, e.g. http://lab.llvm.org:8011/builders/lld-x86_64-win/builds/71/steps/test-check-all/logs/FAIL%3A%20lld%3A%3Aarm-thumb-undefined-weak.s or http://45.33.8.238/linux/15676/step_11.txt

Please take a look, and revert while you investigate if it's not an easy fix.

If you can give me a an hour or two I can fix, am away from computer at moment. The LLD test will need the adr and ldr instructions replaced with .inst and .reloc I thought I'd done all of these in arm-thumb-adr.s and arm-thumb-ldrlit.s but it looks like I missed this one.

This broke check-lld everywhere, e.g. http://lab.llvm.org:8011/builders/lld-x86_64-win/builds/71/steps/test-check-all/logs/FAIL%3A%20lld%3A%3Aarm-thumb-undefined-weak.s or http://45.33.8.238/linux/15676/step_11.txt

Please take a look, and revert while you investigate if it's not an easy fix.

If you can give me a an hour or two I can fix, am away from computer at moment. The LLD test will need the adr and ldr instructions replaced with .inst and .reloc I thought I'd done all of these in arm-thumb-adr.s and arm-thumb-ldrlit.s but it looks like I missed this one.

Committed fix to LLD tests under 61bccda9d9d920c72f49025f11e8601daeb096ec and checked it worked with this patch.

@MarkMurrayARM For MC changes, you need ninja check-llvm check-lld. From my experience most affected tests are under llvm/test/{CodeGen,MC,DebugInfo} lld/test.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFObjectWriter.cpp