Page MenuHomePhabricator

AArch64 ILP32 relocations for assembly and ELF
ClosedPublic

Authored by joelkevinjones on Oct 1 2016, 10:20 AM.

Details

Summary

Add relocations for AArch64 ILP32. Includes:

  • Addition of definitions for R_AARCH32_*
  • Definition of new -target-abi: ilp32
  • Definition of data layout string
  • Tests for added relocations. Not comprehensive, but matches existing tests for 64-bit. Renames "CHECK-OBJ" to "CHECK-OBJ-LP64".
  • Tests for llvm-readobj

Diff Detail

Repository
rL LLVM

Event Timeline

joelkevinjones retitled this revision from to Add relocations for AArch64 ILP32. Includes: - definition of new -target-abi: ilp32 - Addition of definitions for R_AARCH32_* - Defnition of data layout string - tests for added relocations---not comprehensive, but matches existing....
joelkevinjones updated this object.
joelkevinjones added a reviewer: echristo.
joelkevinjones retitled this revision from Add relocations for AArch64 ILP32. Includes: - definition of new -target-abi: ilp32 - Addition of definitions for R_AARCH32_* - Defnition of data layout string - tests for added relocations---not comprehensive, but matches existing... to AArch64 ILP32 relocations for assembly and ELF.Oct 1 2016, 10:35 AM
joelkevinjones updated this object.
joelkevinjones removed reviewers: peter.smith, zatrazz.

Adding Tim as a reviewer as well. I'll try to get to this shortly.

Thanks!

peter.smith edited edge metadata.Oct 3 2016, 3:13 AM

Just some initial comments, I've not had a chance to go through the whole thing.

Have you got any reference to the target names chosen by gcc? It would help if they are the same.

include/llvm/Support/ELFRelocs/AArch64.def
6 ↗(On Diff #73200)

Should this be 1.1-beta? This would match the revision history in the document http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056c/IHI0056C_beta_aaelf64.pdf

Although it still says 1.0 on the cover page, I think it would be more helpful to say 1.1-beta as on the website the 1.0 document does not have any ILP-32 information in it.

12 ↗(On Diff #73200)

The names for the relocations don't match what is in the ABI document. Instead of R_AARCH32_* it should be R_AARCH64_P32_*

See section 4.6.3.1 Relocation Names and Classes of ELF for the 64-bit ARM Architecture http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056c/IHI0056C_beta_aaelf64.pdf

I believe binutils has followed this convention: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=include/elf/aarch64.h;h=fd5ecd8d1e7fc614b0e854846b19bfb625327711;hb=HEAD#l124

Peter:

Thanks for your comments. I'll change the relocation names and change the citation to use the internal revision history name and make a comment about the discrepancy with the cover page.

t.p.northover added inline comments.Oct 3 2016, 12:11 PM
include/llvm/Support/ELFRelocs/AArch64.def
12 ↗(On Diff #73200)

Agreed, this is definitely not AArch32 (which is ARM or Thumb mode).

lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
80 ↗(On Diff #73200)

'not' supported.

139–141 ↗(On Diff #73200)

I'm not convinced by this. Initializing a 64-bit quantity with a 32-bit address statically seems like a pretty reasonable thing to me, especially as we already allow the far dodgier 16-bit and 8-bit versions. It's allowed on i386 too, producing an R_386_32 relocation as I'd expect.

Note that we might have endianness complications here though: the R_AARCH64_P32_ABS32 would have to be placed on the last 4 bytes in a big-endian context.

Actually, I'd probably go a bit further and suggest adding an R_AARCH64_P32_PREL64 relocation to the ELF document so that the construct works in PC-relative situations too.

joelkevinjones added inline comments.Oct 3 2016, 1:20 PM
lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
139–141 ↗(On Diff #73200)

I have to admit I'm confused. When you refer to 8-bit versions, which relocations are you referring to? As to the reasonableness of initializing a 64-bit quantity with a 32-bit quantity, sure, it fits, but what would an ILP32 loader do with R_AARCH64_ABS64? Otherwise, there isn't a relocation type appropriate, as there is not a R_AARCH64_P32_ABS64, at least in the document at on the ARM web site.

t.p.northover added inline comments.Oct 3 2016, 1:46 PM
lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
139–141 ↗(On Diff #73200)

I have to admit I'm confused. When you refer to 8-bit versions, which relocations are you referring to?

R_AARCH64_P32_ABS8. Used (I think) in constructs like

.byte foo

but what would an ILP32 loader do with R_AARCH64_ABS64.

The same as for an R_AARCH64_P32_ABS32 except that if it was linking for a big-endian target it would have to store the address bits at offsets 4-8 rather than 0-4. This could be faked by the compiler using only an R_AARCH64_P32_ABS32 and converting

.xword foo

into either (little-endian)

.word foo
.word 0

or (big-endian)

.word 0
.word foo

However, the PC-relative one can't be faked because the offset might be negative, in which case the high bits of the 64-bit quantity need to be set to the correct sign bits.

That's why I suggested adding an R_AARCH64_P32_PREL64 relocation (I suppose as an enhancement request to the ELF ABI).

t.p.northover added inline comments.Oct 3 2016, 1:47 PM
lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
139–141 ↗(On Diff #73200)

I have to admit I'm confused. When you refer to 8-bit versions, which relocations are you referring to?

Ah, sorry, I misremembered there being 8-bit versions but now see they only go down to 16. I think the overall point still stands though.

joelkevinjones added inline comments.Oct 3 2016, 2:13 PM
lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
139–141 ↗(On Diff #73200)

Just to be clear. Given that there isn't a definition for R_AARCH64_P32_ABS64, in either the ARM specification or in Linux ELF handling, are you suggesting that the code should:

  1. Check that the value will fit in 32 bits and issue a warning
  2. Else generate a R_AARCH64_32_ABS32?

To save anyone else the trouble I've checked the relocation codes match the ABI.

I'm not sure I understand the rationale for R_AARCH64_P32_PREL64. As I understand it, in ILP32 addresses are 32-bits so why would we want to initialise a 64-bit address from a 32-bit quantity, either absolute or relative.

From a look into my mail archive there was some discussion over including 64-bit relocation types in the ILP32 as at least one on X32. Our thoughts were that the reason this was needed didn't apply to AArch64 https://sourceware.org/ml/binutils/2011-08/msg00009.html
There isn't an equivalent X32 relocation for R_AARCH64_P32_PREL64 that I could find.

I think if a real concrete use case could be made for R_AARCH64_P32_PREL64 then it would be worth writing up a proposal (the contact address is on the cover page).

lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
72 ↗(On Diff #73200)

There is a lot of repeated code that is of the form return (IsILP32) ? R_AARCH32... : R_AARCH64; or
if (ILP32) then error not supported else return ...

I'm wondering if there is some scope to simplify. Possibilities include:

  • A macro of the form R_CLS_(PREL32) which would expand into return (IsILP32) ? ... : ... . Another could be used for the case when there is no ILP32.
  • Do some kind of table lookup rather than a switch.

I can't say whether these would improve the readability unless they were tried though.

275 ↗(On Diff #73200)

Is reporting VK_ABS_G3 helpful to a user that doesn't have the llvm source code available? Would it be better to report the AArch64 code as at least that can be looked up in the ABI docs. Similar comments apply to the other error messages.

Peter:

Thanks for your detailed comments. I have made the changes suggested earlier with regard to the proper names (R_AARCH32* -> R_AARCH64_P32). I agree with your comment about the repeated code. A macro that mimics the tables in the specification is probably clearer. I'll try that and see how it goes. I also concur with your comment about the text of the error messages.

As to R_AARCH64_P32_PREL64 (or any other 64 bit relocations), I'd like to land this with support for the standard (beta, I know) as it is now. Later commits can deal with any changes to the specification.

joelkevinjones updated this revision to Diff 73669.EditedOct 5 2016, 10:18 AM
joelkevinjones edited edge metadata.

AArch64 ILP32 relocations for assembly and ELF
Add relocations for AArch64 ILP32. Includes:

  • Addition of definitions for R_AARCH64_P32*
  • Definition of new -target-abi: ilp32
  • Defnition of data layout string
  • Tests for added relocations---not comprehensive, but matches existing tests for 64-bit. Renames "CHECK-OBJ" to "CHECK-OBJ-LP64".
  • Tests for llvm-readobj - Update with correct relocation names, spelling fix.

Does anyone have any comments on the code as it is now, i. e.:

  • Relocation names matching standard
  • Correct comment about specification version
  • Use of macros to reduce duplicate code (both "?:" and error strings)

I'm somewhat unconvinced about all of the plumbing of ilp32 down in the mc writer - especially AArch64ELFObjectWriter which is pretty much just the single function. I might just duplicate the code, but maybe it's readable enough the way it is?

Tim?

-eric

lib/Target/AArch64/AArch64TargetMachine.cpp
195 ↗(On Diff #73669)

You can remove the comment above this (in a separate patch please) since it doesn't appear to be valid anymore.

Thanks for using the ABI relocation names, and I think the use of the macro is an improvement in readability. I don't have any further comments or objections.

lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
51 ↗(On Diff #73669)

The double not in "...not PC relative MOV relocation not..." is a bit confusing. Perhaps "non PC-relative" or use absolute instead of not PC relative.

test/MC/AArch64/ilp32-diagnostics.s
5 ↗(On Diff #73669)

This is the only error message check that I could find. Are there any plans to test the mov relocation errors?

Add relocations for AArch64 ILP32. Includes:

  • Addition of definitions for R_AARCH32_*
  • Definition of new -target-abi: ilp32
  • Defnition of data layout string
  • Tests for added relocations---not comprehensive, but matches existing tests for 64-bit. Renames "CHECK-OBJ" to "CHECK-OBJ-LP64".
  • Tests for llvm-readobj
t.p.northover edited edge metadata.Oct 18 2016, 8:47 PM

Sorry Joel, I missed that you'd updated the patch. I think it's mostly looking reasonable now, you've certainly convinced me that you've made the right decisions feature-wise. A couple of comments:

include/llvm/Support/ELFRelocs/AArch64.def
13–14 ↗(On Diff #74257)

I think the P32 entries need to be separated here. They'll never be encountered in the same object file and the numbers jumping around is more confusing than separating the ABS32 variants (especially as the numbers don't actually match up anyway, 0x102 vs 0x1).

lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp
237–239 ↗(On Diff #74257)

I (still? can't remember if I said before) think that filtering out the G2s and G3s (plus any others thaat only apply to LP64) before this switch will improve the code clarity.

joelkevinjones edited edge metadata.

AArch64 ILP32 relocations for assembly and ELF

Add relocations for AArch64 ILP32. Includes:

  • Addition of definitions for R_AARCH32_*
  • Definition of new -target-abi: ilp32
  • Definition of data layout string
  • Tests for added relocations---not comprehensive, but matches existing tests for 64-bit. Renames "CHECK-OBJ" to "CHECK-OBJ-LP64".
  • Tests for llvm-readobj
t.p.northover accepted this revision.Oct 20 2016, 3:23 PM
t.p.northover edited edge metadata.

Thanks Joel. This looks good to me now.

This revision is now accepted and ready to land.Oct 20 2016, 3:23 PM
joelkevinjones edited edge metadata.
This revision was automatically updated to reflect the committed changes.