Adds support for xray on mips/mipsel/mips64/mips64el.
Patches for llvm and clang:
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
Removed patching of instruction "addiu t9, t9, 44" as it was unnecessary since it will always be there at the end of the sled when xray instrumentation is enabled.
Comment Actions
The sled has an issue in that it requires 64bit atomic operations to disable it. MIPS32 doesn't natively support such operations and MIPS64 would need alignment guarantees to perform a 64 bit write. Instead if we use a sled like this:
addiu sp, sp, -8 ;create stack frame nop ;avoid the need for 64bit atomics. sw ra, 4(sp) ;save return address sw t9, 0(sp) ;save register t9 lui t9, <Upper half of address of TracingHook> ori t9, t9, <Lower half of address of TracingHook> jalr t9 ;call Tracing hook addiu t0 , zero, #<function ID> ;pass function id (delay slot) lw t9, 0(sp) ;restore register t9 lw ra, 4(sp) ;restore return address addiu sp, sp, 8 ;delete stack frame
We can disable or re-enable the sled with a single 32 bit write by changing addiu sp, sp, -8 to b.
| lib/xray/xray_mips.cc | ||
|---|---|---|
| 28 | That comment should be "LUI T9, (<Address> >> 16) & 0xffff | |
| 29 | // ORI T9, T9, <Address> & 0xffff | |
| 31 | That opcode is wrong for mips, it should be 0x240c0000. Disassembling the existing one shows the instruction to be "addiu $8, $zero, 0". | |
| 54–55 | There needs to be a nop between the addiu and the sw ra, 4(sp) | |
| 59 | You need an "lui t9, %hi(FunctionID)" here. | |
| 60 | Likewise, change this to "ori t0, t0, %lo(FunctionID). | |
| 108–115 | You can't achieve memory safety with any ordering of these atomic writes. If program executes the branch before the nop is written, the branch will be executed and the sw ra, 4(sp) will be executed in the delay slot. Reverse the order and the program could enter the tracing functionality without having saved the ra register. 64 bit atomic writes are usually not available on MIPS, and we'd have to rely on OS intervention before the program enters the sled to perform an atomic write. See my above comment on the sled design. | |
| lib/xray/xray_mips64.cc | ||
| 32 | That opcode is wrong here as well. | |
| 54–67 | The comment here is wrong. It needs to be the MIPS64 version for stack manipulations, spills and reloads. | |
| 116–122 | See my comments about the memory safety issue. | |
| lib/xray/xray_trampoline_mips.S | ||
| 24 | .cpload needs to be wrapped in .set noreorder/.set reorder block. | |
| 25 | "Save argument registers before doing any actual work" is clearer IMHO. | |
| 27 | After this, save $ra to the stack, and write: .cfi_cfa_offset 31, <stack pointer adjustment + offset of saved ra> to get the correct cfi information, IIUC. | |
| 32–35 | You can't access $f13, $f15 in no-oddsp mode. Instead, use sdc1 $f12 / sdc $f14 account for floating point arguments. That will handle both single and double precision cases. | |
| 37 | There is no need to do this, as the .cpload macro has computed this object's gp pointer. We only need the gp pointer to compute the address of _ZN6__xray19XRayPatchedFunctionE's got entry, so there is no reason to save it. This also applies to __xray_FunctionExit. | |
| 45 | "move $a1, $zero" is clearer. | |
| 47 | "move $a0, $t0" is clearer. | |
| 52 | We don't need to restore gp here. | |
| 54–57 | See my comment above about using sdc1, we need ldc $f12 / ldc $f14 here. | |
| 73–77 | See my first two comments above about on the function entry prologue. | |
| 75 | "Save return registers before doing any actual work" is clearer IMHO. | |
| 78–81 | We need to save v0, v1, a0, a1 in the integer register set as they contain the return values. | |
| 82–85 | We need to save $f0, $f2 in the floating point register set. | |
| 96 | "move $a0, $t0" | |
| 103–110 | These reloads need to match the spills above. | |
| lib/xray/xray_trampoline_mips64.S | ||
| 26–27 | See my comments on the 32 bit version, This applies to most of them, so I'll highlight the MIPS64 specific differences. | |
| 31 | For N64/N32 you also need to store $a4-$a7. | |
| 35 | For N64/N32 you also have to save $f16-$f19. | |
| 54–61 | The corresponding reloads are required. | |
| 78–87 | For return values on N64/N32 we need to save $a0, $v0 for f128s on soft-float systems, $f0, $f1 for structs containing a long double, and $f0, $f2 for long doubles otherwise. This is in addition the standard return registers. | |
Comment Actions
Addressed review comments
| lib/xray/xray_mips.cc | ||
|---|---|---|
| 31 | We intend it to be "addiu $t0, $zero, 0" since we need to pass function id in register $t0 ($8 for MIPS32). But the opcode is surely wrong for the MIPS64 part, it should be 0x240C0000 because $t0 = $12 on MIPS64. | |
| lib/xray/xray_mips64.cc | ||
| 32 | Changed it to 0x240C0000 | |
Comment Actions
LGTM with nits addressed.
| lib/xray/xray_mips.cc | ||
|---|---|---|
| 109 | You don't need to overwrite a nop with a nop. | |
| lib/xray/xray_mips64.cc | ||
| 109 | You don't need to overwrite a nop with a nop. | |
| lib/xray/xray_trampoline_mips.S | ||
| 24–26 | Formatting nit: alignment of the .set (no)reorders should match that of .cpload. | |
| lib/xray/xray_trampoline_mips64.S | ||
| 42–45 | Nit: Formatting. Align the operands with the ones above. | |
That comment should be "LUI T9, (<Address> >> 16) & 0xffff