[Webkit-unassigned] [Bug 175447] Implement MacroAssembler::probe support on MIPS.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=175447

--- Comment #8 from Stanislav Ocovaj <stanislav.ocovaj at rt-rk.com> ---
Comment on attachment 327416
  --> https://bugs.webkit.org/attachment.cgi?id=327416
MIPS implementation of MacroAssembler::probe()

View in context: https://bugs.webkit.org/attachment.cgi?id=327416&action=review

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:91
>> +#define PROBE_CPU_PC_OFFSET (PROBE_FIRST_SPREG_OFFSET + (32 * GPREG_SIZE))
> 
> How about saving FCSR too? It would fit well at PROBE_FIRST_SPREG_OFFSET + (33 * GPREG_SIZE) which is empty.

I will add saving/restoring of FCSR and other control registers

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:95
>> +#define FPREG_SIZE 8
> 
> Since we save the fp registers as doubles, we indeed need to save 8 bytes per *even* register. Which means all the space reserved for odd registers bellow is wasted. I suggest only saving space for even registers:
> 
> #define PROBE_CPU_F0_OFFSET (PROBE_FIRST_FPREG_OFFSET + (0 * FPREG_SIZE))
> #define PROBE_CPU_F2_OFFSET (PROBE_FIRST_FPREG_OFFSET + (1 * FPREG_SIZE))
> #define PROBE_CPU_F4_OFFSET (PROBE_FIRST_FPREG_OFFSET + (2 * FPREG_SIZE))
> etc...

The problem here is that these offsets must match register ID's defined by the FPRegisterID enum. To save the space in the context we would have to modify those enums, but then they would not match real FPregister ID's as they are used in MIPS assembly, so I suggest that we leave this as it is.

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:132
>> +#define PROBE_SIZE_PLUS_EXTRAS              (PROBE_SIZE + (2 * PTR_SIZE))
> 
> A comment would be welcome exlpaining what the extras are. Looks like it's SAVED_PROBE_RETURN_PC + something else. What something else? Or is it padding for alignment?

The extras are SAVED_PROBE_RETURN_PC plus the padding

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:265
>> +static_assert(!(sizeof(RARestorationRecord) & 0x7), "LRRestorationRecord must be 8-byte aligned");
> 
> I'm confused. Why does it need to be 8-byte aligned? IIRC, we need the $sp to point to an 8-aligned address at the time of a function call, but I don't see why we need the alignment here (though I might be forgetting about something). Also, I think you mean RARestorationRecord not LRRestorationRecord.

MIPS calling convention requires that the stack is 8-byte aligned at all times, not only at the time of a function call. This is because the code might be interrupted at any time by a signal, and signal handler might need to push a double value to the stack

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:339
>> +    "addiu     $ra, $ra, 2*4" "\n" // The PC after the probe is at 2 instructions past the return point.
> 
> I think we should use defines here. E.g. "addiu $ra, $ra, PROBE_INSTRUCTIONS_AFTER_CALL * PTR_SIZE"
> And maybe also add an assert (or at least a comment) at the end of MacroAssembler::probe() to make sure that PROBE_INSTRUCTIONS_AFTER_CALL is in sync with the number of instructions after the trampoline call.

I will change it to 2 * PTR_SIZE. This is how it's done on ARM64. I don't see a simple way to check if this is in sync with the actual number of instructions

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:410
>> +    // out of the Probe::State before returning, except for zero, k0 and k1.
> 
> I am wondering: is it safe to restore $at too?

I don't see why it wouldn't be safe, as long as the probe's user knows what he's doing. The real question here is whether it is *possible* to save/restore some registers. Zero obviously cannot be set to any value, and k0 and k1 may be modified by the kernel at any time, so that's the main reason why these registers are excluded.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171122/6a74c88a/attachment.html>