IonMonkey: make new OSI mechanism build on ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 593605 [details] [diff] [review]
Get OSI building on ARM.

We'll see how correct it is once the TI enabling patch lands.
Attachment #593605 - Flags: review?
Attachment #593605 - Flags: review? → review?(mrosenberg)
Comment on attachment 593605 [details] [diff] [review]
Get OSI building on ARM.

Review of attachment 593605 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/Assembler-arm.h
@@ +1621,5 @@
> +        // imm32
> +        inst[0] = 0xe51ffffc;
> +        inst[1] = uintptr_t(toCall.raw());
> +        JSC::ExecutableAllocator::cacheFlush(inst, 2 * sizeof(uintptr_t));
> +    }

That is not pretty, but it'll get the job done.

::: js/src/ion/arm/Bailouts-arm.cpp
@@ -147,5 @@
> -class InvalidationBailoutStack
> -{
> -    double fpregs_[FloatRegisters::Total];
> -    uintptr_t regs_[Registers::Total];
> -    uintptr_t pad[2];

I assume the 2 words of padding are the original return address, and the original padding?

::: js/src/ion/arm/IonFrames-arm.h
@@ +64,5 @@
>      FrameType prevType() const {
>          return FrameType(descriptor_ & ((1 << FRAMETYPE_BITS) - 1));
>      }
> +    void changePrevType(FrameType type) {
> +        descriptor_ &= ~(1 << uintptr_t(FRAMETYPE_BITS));

This looks like it is only going to mask out a single bit, not the lower FRAMETYPE_BITS bits

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +428,5 @@
> +    CodeOffsetLabel pushWithPatch(ImmWord imm) {
> +        CodeOffsetLabel label = currentOffset();
> +        push(Imm32(imm.value));
> +        return label;
> +    }

What values can imm be when this is called, and how is the value going to be patched later?
I'm worried that the load into r12 is going to only be a single instruction long, which cannot be patched into a longer sequence later.
Attachment #593605 - Flags: review?(mrosenberg) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/18d45f6608d8

Fixed the FRAMETYPE_BITS problem, and the rest are definitely valid concerns, but the we'll need to fix when we get OSI correctness. For some reason I can't set breakpoints on the ARM board that I have, so correctness will have to be a followup once I can figure out a setup that works.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Chris Leary [:cdleary] from comment #2)
> https://hg.mozilla.org/projects/ionmonkey/rev/18d45f6608d8
> 
>  For some
> reason I can't set breakpoints on the ARM board that I have

Try, try again.
I've noticed when gdb prints out
"[Thread debugging using libthread_db enabled]" right after running the executable, breakpoints work
otherwise, they are quite hosed.
You need to log in before you can comment on or make changes to this bug.