Page MenuHomePhabricator

[Mips] Add support for MCJIT for MIPS32r6
ClosedPublic

Authored by vradosavljevic on Jun 24 2015, 5:37 AM.

Details

Summary

Add support for resolving MIPS32r6 relocations in MCJIT.

Diff Detail

Repository
rL LLVM

Event Timeline

vradosavljevic retitled this revision from to [Mips] Add support for MCJIT for MIPS32r6.
vradosavljevic updated this object.
vradosavljevic edited the test plan for this revision. (Show Details)
vradosavljevic added reviewers: dsanders, petarj.
vradosavljevic set the repository for this revision to rL LLVM.
vradosavljevic added a subscriber: Unknown Object (MLST).
dsanders added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1312–1315 ↗(On Diff #28335)

I'll need to look up the details of the Mips32r6 relocs to review this patch.

However, I believe I've noticed a significant bug in the nearby code that I ought to mention. The handling of R_MIPS_HI16 and R_MIPS_LO16 is incorrect. These relocs have the unusual property that their addend is 32-bit with Hi16 and LO16 each contributing 16-bits. This means that to handle a HI16, we must scan forwards to the corresponding LO16 to retrieve the other half of the addend. Similarly, LO16 must scan backwards to find the top half of the addend. We probably have bugs when the addend causes the result to cross a 16-bit boundary since the carry from the LO16 won't propagate into the HI16:

foo = 65532
R_MIPS_HI16(Symbol = foo, Addend = 0): Value is 65532 >> 16 = 0 (but should be 1)
R_MIPS_LO16(Symbol = foo, Addend = 8): Value is 65540 & 0xffff = 4

@atanasyan: I think you've implemented HI16/LO16 in lld. Is my understanding of this correct?

@dsanders You are absolutely right. Here is a quote from MIPS ABI:

The AHL addend is a composite computed from the addends of two consecutive relocation
entries. Each relocation type of R_MIPS_HI16 must have an associated
R_MIPS_LO16 entry immediately following it in the list of relocations.

These relocation entries are always processed as a pair and both addend fields
contribute to the AHL addend. If AHI and ALO are the addends from the
paired R_MIPS_HI16 and R_MIPS_LO16 entries, then the addend AHL is computed
as (AHI << 16) + (short)ALO. R_MIPS_LO16 entries without an
R_MIPS_HI16 entry immediately preceding are orphaned and the previously defined
R_MIPS_HI16 is used for computing the addend.

But if addends are recorded in the SHT_RELA section we can use them as is and do not
have to calculate a combined AHL addend.

Also please note that in case of O32 ABI if R_MIPS_HI16 / R_MIPS_LO16 relocations
reference the _gp_disp symbol they should be calculated using special formulas:

R_MIPS_HI16: (AHL + GP – P) – (short) (AHL + GP – P)) >> 16
R_MIPS_LO16: AHL + GP – P + 4
vradosavljevic added inline comments.Jun 24 2015, 7:42 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1312–1315 ↗(On Diff #28335)

I can add FIXME in this patch, and a fix for this bug can be part of another patch. What are your thoughts?

dsanders edited edge metadata.Jun 25 2015, 7:22 AM

Also please note that in case of O32 ABI if R_MIPS_HI16 / R_MIPS_LO16 relocations
reference the _gp_disp symbol they should be calculated using special formulas:

R_MIPS_HI16: (AHL + GP – P) – (short) (AHL + GP – P)) >> 16
R_MIPS_LO16: AHL + GP – P + 4

Hmm, that could be tricky. At the moment, I'm not sure the name is available.

I can add FIXME in this patch, and a fix for this bug can be part of another patch. What are your thoughts?

I agree. It's not something that needs fixing in this patch, it's just something I noticed in passing.

Also please note that in case of O32 ABI if R_MIPS_HI16 / R_MIPS_LO16 relocations
reference the _gp_disp symbol they should be calculated using special formulas:

R_MIPS_HI16: (AHL + GP – P) – (short) (AHL + GP – P)) >> 16
R_MIPS_LO16: AHL + GP – P + 4

Hmm, that could be tricky. At the moment, I'm not sure the name is available.

There is no support for resolving PIC relocations for O32, so this won't be the case.

I'm thinking of when we do support that but until then we should at least catch and fail on the unsupported case rather than silently doing the wrong thing.

dsanders accepted this revision.Jul 6 2015, 1:51 AM
dsanders edited edge metadata.

LGTM with R_MIPS_PC19_S2 corrected and R_MIPS_PCHI16's alignment mask added.

Fixing the addend for HI16/LO16/PCHI16/PCLO16 can be in a later patch.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
522 ↗(On Diff #28335)

Are you sure about the '>> 2'? This isn't mentioned in the documentation I'm reading and it isn't present on the PCLO16. I suspect it's a copy/paste error from the *_S2 relocs below

527 ↗(On Diff #28335)

The documentation says '(sign_extend(A) + S - (P & ~0x3)) >> 2' so I believe there should be a '& ~0x3' on FinalAddress.

This only applies to R_MIPS_PC19_S2. R_MIPS_PC18_S3 is similar (with 0x7) but the other *_S2's don't do the same thing.

547–560 ↗(On Diff #28335)

These two have the same quirk as R_MIPS_HI and R_MIPS_LO. Their addends are taken together as a 32-bit value. This can be fixed at the same time as R_MIPS_HI/R_MIPS_LO.

1312–1315 ↗(On Diff #28335)

That's ok.

The same applies to PCHI16 and PCLO16.

This revision is now accepted and ready to land.Jul 6 2015, 1:51 AM
vradosavljevic marked 2 inline comments as done.Jul 6 2015, 3:38 AM
vradosavljevic added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
522 ↗(On Diff #28335)

Yes, I'm sure that R_MIPS_PC16 relocation has '>> 2'. You can check that in lld or bfd. I don't know why that isn't documented.

vradosavljevic edited edge metadata.

Comments addressed.

dsanders added inline comments.Jul 6 2015, 3:56 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
522 ↗(On Diff #29071)

You're right. I'll correct the docs.

This revision was automatically updated to reflect the committed changes.