Closed Bug 723290 Opened 8 years ago Closed 8 years ago

IonMonkey: make new OSI mechanism build on ARM

Categories

(Core :: JavaScript Engine, defect)

ARM
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cdleary, Assigned: cdleary)

Details

Attachments

(1 file)

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
Closed: 8 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.