This is an archive of the discontinued LLVM Phabricator instance.

Fix PREL31 relocation on ARM
ClosedPublic

Authored by yuyichao on Sep 29 2016, 9:58 PM.

Details

Summary

This is a 31bits relative relocation instead of a 32bits absolute relocation.

Diff Detail

Event Timeline

yuyichao updated this revision to Diff 72978.Sep 29 2016, 9:58 PM
yuyichao retitled this revision from to Fix PREL31 relocation on ARM.
yuyichao updated this object.
yuyichao added reviewers: rengolin, t.p.northover.
yuyichao added a subscriber: llvm-commits.
yuyichao updated this revision to Diff 72979.Sep 29 2016, 10:00 PM
peter.smith edited edge metadata.Sep 30 2016, 12:25 PM

Your solution is the same way that R_ARM_PREL31 is handled in lld, so this looks fine from a correctness point of view.

I'm not familiar with this area so I think you'll need approval from a maintainer, they may also insist on a test being written.

Your solution is the same way that R_ARM_PREL31 is handled in lld, so this looks fine from a correctness point of view.

Good to know.... Would have saved me some time....

they may also insist on a test being written.

The failure shows up as invalid unwind info. Any pointer to a similar test / how can I write one?

Apologies I'm not familiar with mcjit to know how to write a test for this. Speculating:

  • To get a prel31 relocation that will fail there will need to be a negative offset. If the .arm.exidx section is after the executable code or any link to a .ARM.extab section in memory/file the offsets will be negative will show up problems.
  • If there is anyway to test the top-bit of the relocated entry you should be able to detect it.
yuyichao added inline comments.Oct 1 2016, 3:54 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
468

I think I made a stupid mistake here... Should be 0x80000000 instead of 0x8000000 (7 zeros instead of 6 ......) Will update later.

yuyichao updated this revision to Diff 73203.Oct 1 2016, 4:05 PM
yuyichao edited edge metadata.

Fix bit31 mask....

yuyichao updated this revision to Diff 74091.Oct 9 2016, 7:36 PM

Add test.

peter.smith accepted this revision.Oct 10 2016, 1:44 AM
peter.smith edited edge metadata.

This looks good to me from an ARM perspective. I'm not a contributor in this area so you may want to wait a bit to see if anyone objects on style grounds.

This revision is now accepted and ready to land.Oct 10 2016, 1:44 AM

Ping. Does this still need more review or is this OK to merge? I'll also need someone else to push for me since I don't have commit access.

rengolin accepted this revision.Oct 15 2016, 10:04 AM
rengolin edited edge metadata.

There's nothing controversial about this patch, LGTM.

This revision was automatically updated to reflect the committed changes.