Last Comment Bug 723290 - IonMonkey: make new OSI mechanism build on ARM
: IonMonkey: make new OSI mechanism build on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM All
: -- normal (vote)
: ---
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 14:06 PST by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2012-02-02 13:37 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Get OSI building on ARM. (14.70 KB, patch)
2012-02-01 14:06 PST, Chris Leary [:cdleary] (not checking bugmail)
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Chris Leary [:cdleary] (not checking bugmail) 2012-02-01 14:06:27 PST
Created attachment 593605 [details] [diff] [review]
Get OSI building on ARM.

We'll see how correct it is once the TI enabling patch lands.
Comment 1 Marty Rosenberg [:mjrosenb] 2012-02-01 14:44:24 PST
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.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2012-02-02 13:22:34 PST
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.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-02-02 13:37:38 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.