Add support for resolving MIPS64r2 and MIPS64r6 relocations in MCJIT.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
This supersedes D5913 doesn't it? If so, could you mark that review abandoned.
I believe testcases similar to test/ExecutionEngine/RuntimeDyld/{AArch64,ARM,X86}/* will be required so that we can test the application of relocations without needing to execute JIT'ed code.
As with D5913, there are a couple big issues with this patch. The first is that checking for Triple::mips64/Triple::mips64el is insufficient since this (currently, see below) includes both N32 and N64 and these ABI's need to be treated differently. For example, the 3-in-1 reloc encoding is not used on N32. I don't mind if N32 support arrives in a later patch but this patch needs to identify N32 and N64 and error on N32 while handling N64 rather than quietly doing the wrong thing. The second issue is that using Triple::mips/Triple::mipsel vs Triple::mips64/Triple::mips64el to distinguish 32-bit and 64-bit ABI's is incorrect. It's valid to execute O32 binaries with a 64-bit triple, and N32/N64 binaries with a 32-bit triple. Admittedly, LLVM for Mips targets already has a number of issues in this area but I'd like to avoid adding more.
unittests/ExecutionEngine/MCJIT/MCJITTestBase.h | ||
---|---|---|
295 | I think the answer is 'no' but: Does test/ExecutionEngine/MCJIT/lit.local.cfg need updating? |
As dsanders noted, this needs some llvm-rtdyld checker tests along the lines of test/ExecutionEngine/RuntimeDyld/{AArch64,ARM,X86}/* .
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h | ||
---|---|---|
124–139 | Could you please note in the comments that these data structures are MIPS64 specific? At some point in the future we would like to split RuntimeDyldELF into multiple target-specific subclasses along the same lines as RuntimeDyldMachO. When that happens it will be very helpful to know that these data structures can be moved into the RuntimeDyldELF_MIPS64 subclass. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h | ||
---|---|---|
124–139 | Done. | |
unittests/ExecutionEngine/MCJIT/MCJITTestBase.h | ||
295 | We don't have to change test/ExecutionEngine/MCJIT/lit.local.cfg, because 'Mips' contains all Mips triples. mips-* | mips64-*) host_arch="Mips" ;; mipsel-* | mips64el-*) host_arch="Mips" ;; |
Comments addressed. Added llvm-rtdyld checker test, marked MCJIT and OrcMCJIT PIC tests as XFAIL only for mips32 architecture, and added an error report when using N32/O32 ABI's with Mips64 triple.
Comments addressed. Added llvm-rtdyld checker test, marked MCJIT and OrcMCJIT PIC tests as XFAIL only for mips32 architecture, and added an error report when using N32/O32 ABI's with Mips64 triple.
There's still several triple checks that should be ABI checks. Using the triple for more than distinguishing the Mips target from the others is incorrect.
Also, the llvm-rtdyld test doesn't seem to cover many relocations. Could you expand on it?
lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp | ||
---|---|---|
777 | Target triple -> ABI checks. Only N64 should take this path. | |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
526–529 | This particular overload of resolveMIPS64Relocation is only called from this function. So if we rename it to evaluateMIPS64Relocation(), stop it applying the reloc, and have it take/return the current calculated value we can use it as per this pseudo-code: int64_t calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type, RE.Addend, RE.SectionID, RE.SymOffset, calculatedValue); if (r_type2 == ELF::R_MIPS_NONE) applyMIPS64Relocation(Section.Address + RE.Offset, calculatedValue); This is neater than passing values in side-channels such as member variables. | |
578–579 | Endian bug. You can't use reinterpret_cast since the host and target may have different endians (particularly for test-cases). Use the endian-aware writeBytesUnaligned() or similar instead. Likewise for the other instances below. | |
1289 | Triple checks -> ABI checks. | |
1603–1606 | Not 100% sure about this one, but I think it's only correct for N64 due to pointer size differences. If this is the case, then we need an ABI check | |
1667 | Triple check -> ABI check. | |
1670–1671 | mips64/mips64el and O32 is not an error case, it's valid. Admittedly it doesn't work in LLVM at this point (this is a bug) so maybe including O32 in this error is ok for now. Could you add a comment explaining that. | |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h | ||
124–132 | These three shouldn't be members variables. See below. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | My understanding of Mips64 ABI is there can be more relocation entries for the same instruction, so i'm using applyRelocation, useCalculatedValue and calculatedValue as members variables to keep them for the following relocation entry if that's the case. Mips64 ABI allows us e.g.: 000000000000000c R_MIPS_GPREL16 main So if this is the case (theoretically), we should use result of r_type3 from the first relocation entry, for r_type in the second relocation entry (in our case we should use calculatedValue). From Mips64 ABI: Up to three operations may be specified per record, by the fields r_type , r_type2 , and r_type3 . They are applied in that order, and a zero field implies no further operations from this record. (The following record may continue the sequence if it references the same offset.) | |
1670–1671 | Should i change triple checks for mips and mipsel to O32 ABI checks and report an error only for N32 ABI? |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | There are multiple ABI's that can be used on Mips64. When you refer to the 'Mips64 ABI', I believe you are referring to the N64 ABI. Is this correct? There is also an N32 ABI which behaves differently. For example, the 3-in-1 reloc encoding only applies to the N64 ABI and not the N32 ABI, and pointers/symbols are 4-bytes for N32 and 8-bytes for N64. My main issue with this chunk of code was its use of sideband mechanisms to implement argument/return value passing instead of using a normal function call. It passes arguments in by writing to a member, returns them by writing to a member, and reads the result by reading a member. Using function arguments and return values is much clearer and does not increase the size of the object unnecessarily. Here's the example code with all three relocs accounted for and tidied up: int64_t calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type, RE.Addend, RE.SectionID, RE.SymOffset, calculatedValue); if (r_type2 != ELF::R_MIPS_NONE) calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type2, RE.Addend, RE.SectionID, RE.SymOffset, calculatedValue); if (r_type3 != ELF::R_MIPS_NONE) calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type3, RE.Addend, RE.SectionID, RE.SymOffset, calculatedValue); applyMIPS64Relocation(Section.Address + RE.Offset, calculatedValue); | |
1670–1671 | Yes. It's ok to use triples to distinguish Mips from other targets, but not to determine the ABI. The only one we need to emit an error for is N32. The combination of O32 and a mips64/mips64el triple won't work for now because the code generator will assert up front rather than allow the various bugs to confuse the user. However, we should still accept this combination here because we do the right thing in these layers. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | Yes, i'm referring to Mips N64 ABI. This may not be the end of calculating, so we need to check if the next relocation entry is for the same instruction. If it is for the same instruction, we need to send calculatedValue to next relocation entry. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | Ah, I see the complicating issue. I'm going to have to look up the details of relocation composition to comment on that. The main thing I'm wondering is why Mips needs to do this and other targets don't. I know that some relocation mechanisms would write the calculated value to the section data after one relocation and read it back for the next relocation. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | I've now looked it up. The 64-bit ELF Object Represents an addend obtained as the value of the field being relocated prior to relocation (.rel), from a .rela addend field, or as the preceding result for a composed relocation (either). Since N64 uses RELA relocations, I believe my example above should be passing in calculatedValue instead of RE.Addend for all except the first relocation for that address, like so: int64_t calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type, RE.Addend, RE.SectionID, RE.SymOffset); if (r_type2 != ELF::R_MIPS_NONE) calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type2, calculatedValue, RE.SectionID, RE.SymOffset); if (r_type3 != ELF::R_MIPS_NONE) calculatedValue = evaluateMIPS64Relocation(Section, RE.Offset, Value, r_type3, calculatedValue, RE.SectionID, RE.SymOffset); applyMIPS64Relocation(Section.Address + RE.Offset, calculatedValue); It's possible that the two calculatedValue arguments should be 'calculatedValue + RE.Addend' but the spec was a bit unclear on this so I'm not 100% sure. Your current patch implements the former so I've gone with that.
Why not handle composition of multiple relocs in resolveRelocationList?
To partially answer my own question, relocation composition isn't particularly unique to to Mips but it seems that we may use it more often than other targets. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | What Vladimir is trying to say is that - theoretically speaking - getting calculatedValue for r_type can require calculatedValue that we calculated from r_type3 in the previous RelocationEntry. |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
526–529 | Yes, I understood that. What I'm trying to say is that you don't need sideband data to achieve that. resolveRelocationList can iterate over all the composed relocations, evaluating it as it goes, and then it can apply the calculated value. |
Thanks, that's much cleaner. Most of my comments on the latest patch are nits or notes on things that will need to be followed up on. There is one question among them though, as well as one more comment below.
We seem to have lost the ability to compose distinct relocs in this patch. This should be ok in the immediate term (because we do handle our common composition case of the 3-in-1 relocs) but it's something we'll need to follow up on.
Assuming it's safe to only check r_type for R_MIPS_CALL16, R_MIPS_GOT_PAGE, or R_MIPS_GOT_DISP : LGTM with the nits fixed.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp | ||
---|---|---|
189–191 | Nit: Seems unnecessary. It's about to be destroyed anyway and we don't seem to be managing memory. | |
515–516 | For follow-up: It would be preferable to use a MipsABIInfo object. It's essentially an enum representing the ABI but you can ask it questions (e.g. Does the current ABI use REL or RELA relocations? etc.) using the member functions. I don't think we should do this now though. I think it should wait until Architectures have their own subclasses. If we tried to do it now it would probably require Mips support to always be compiled into LLVM and that's undesirable. | |
519–524 | For follow-up: This is a good example of knowledge that would be suitable for MipsABIInfo. There are multiple places that need to know how to map ELF e_flags to the ABI. | |
539 | Nit: Variables should begin with a capital. r_type/r_type2/r_type3 are arguably ok since they are trying to match member names of an ELF64 structure so there's no need to change those. | |
562 | Nit: Capitalization | |
564 | Nit: Doesn't match the function name (the 'resolve' and the 'Mips') | |
582–584 | Optional nit: Move the return into the cases. E.g: case ELF::R_MIPS_64: return Value + Addend; | |
1055 | Nit: else llvm_unreachable("Mips ABI not handled"); | |
1290–1293 | Is it definitely only r_type that matters? Is it possible to have these relocs in r_type2 or r_type3? | |
1613 | Nit: else llvm_unreachable(...); | |
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h | ||
68 | Nit: calculatedValue should begin with a capital |
Yes, we should not forget that. Vladimir, can you create a ticket for this one?
lgtm.
Target triple -> ABI checks.
Only N64 should take this path.