Add support for resolving MIPS32r6 relocations in MCJIT.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
1309–1314 | 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
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
1309–1314 | I can add FIXME in this patch, and a fix for this bug can be part of another patch. What are your thoughts? |
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.
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.
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 | 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 | 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 | 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. | |
1309–1314 | That's ok. The same applies to PCHI16 and PCLO16. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
522 | 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. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
522 | You're right. I'll correct the docs. |
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