Last Comment Bug 725584 - IonMonkey: refixulate OSI on arm
: IonMonkey: refixulate OSI on arm
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-09 00:12 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-02-17 19:46 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/refixOSI-r0.patch (37.23 KB, patch)
2012-02-09 00:12 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review-
Details | Diff | Splinter Review
/home/mrosenberg/patches/refixOSI-r1.patch (40.44 KB, patch)
2012-02-10 15:44 PST, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
/home/mrosenberg/patches/refixOSI-r2.patch (45.81 KB, patch)
2012-02-10 15:51 PST, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-02-09 00:12:52 PST
Created attachment 595675 [details] [diff] [review]
/home/mrosenberg/patches/refixOSI-r0.patch

The initial attempt at getting the new OSI to work on arm wasn't sufficient. I've fixed the issues, and even made some of the memory-stomping code use a nicer interface that I wrote for it.
Comment 1 Jacob Bramley [:jbramley] 2012-02-09 08:22:29 PST
Comment on attachment 595675 [details] [diff] [review]
/home/mrosenberg/patches/refixOSI-r0.patch

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

I'm not entirely confident about reviewing the code that interacts with the Ion Monkey internals. Specifically, I'm not sure quite how the IM bail-out interfaces work, and what the stack is supposed to look like at various points. Other than that, see my review for detailed comments.

::: js/src/ion/CodeGenerator.cpp
@@ +666,2 @@
>      masm.call(thunk);
> +#endif

I must admit, I don't understand why ARM pushes the LR before invalidateEpilogueData, whilst everything else pushed the return address after it. (The last thing that generateBailoutTail does on ARM is "pop(pc)". The last thing that it does on x86 is "ret", which does effectively the same thing.)

@@ +1206,5 @@
>                                   cacheList_.length(), safepoints_.size());
>      if (!script->ion)
>          return false;
> +    invalidateEpilogueData_.fixup(&masm);
> +        Assembler::patchDataWithValueCheck(CodeLocationLabel(code, invalidateEpilogueData_),

Incorrect indentation.

::: js/src/ion/IonCaches.cpp
@@ +77,5 @@
>  
>       raw_ = code->raw() + new_off;
>       markAbsolute(true);
>  }
> +void

Blank line.

@@ +81,5 @@
> +void
> +CodeOffsetLabel::fixup(MacroAssembler *masm)
> +{
> +     JS_ASSERT(masm != NULL);
> +     offset_ = (size_t)masm->actualOffset((uint8*)offset_);

It doesn't make any sense to cast offset_ to a pointer type, then cast it back to an integral type (inside actualOffset). The fault here is in actualOffset, I think, but it's a very strange use of a pointer type.

::: js/src/ion/IonFrames.cpp
@@ +430,5 @@
>      // Set the result.
>      *scriptRes = script;
>      *pcRes = pc;
>  }
> +void

Blank line.

@@ +431,5 @@
>      *scriptRes = script;
>      *pcRes = pc;
>  }
> +void
> +OsiIndex::fixupOffset(MacroAssembler &masm)

We use 'fixUpOffsets' in the macro assembler. Keeping the capitalization consistent is probably wise, even though it's a different instruction.

@@ +433,5 @@
>  }
> +void
> +OsiIndex::fixupOffset(MacroAssembler &masm)
> +{
> +    returnPointDisplacement_ = (uint32)masm.actualOffset((uint8*)returnPointDisplacement_);

You should probably assert that actualOffset is positive and within range of a uint32 before casting it as such.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +1939,5 @@
>      return;
>  }
> +// The size of an arbitrary 32-bit call in the instruction stream.
> +// On ARM this sequence is |pc = ldr pc - 4; imm32| given that we
> +// never reach the imm32.

A near call is not an arbitrary 32-bit call. Also, the example sequence is not a call. The code looks right, but I think the comment is wrong.

@@ +1955,5 @@
> +    //inst[0] = 0xe51ff004;
> +    //inst[1] = uintptr_t(toCall.raw());
> +    uint8 *dest = toCall.raw();
> +    new (inst) InstBLImm(BOffImm(dest - (uint8*)inst) , Always);
> +    JSC::ExecutableAllocator::cacheFlush(inst, sizeof(uintptr_t));

sizeof(uint32_t) would be semantically better, though they are the same on ARM.

@@ +1965,5 @@
> +    Register dest;
> +    Assembler::RelocStyle rs;
> +    uint32 *val = getPtr32Target(ptr, &dest, &rs);
> +    JS_ASSERT((uint32)val == expectedValue.value);
> +    reinterpret_cast<MacroAssemblerARM*>(dummy)->ma_movPatchable(Imm32(newValue.value), dest, Always, rs, ptr);

The 'dummy' field seems wrong to me, and casting it first is even weirder. Why not just make a static method to handle the cases where this behaviour is required?

@@ +1973,5 @@
> +// This just stomps over memory with 32 bits of raw data. Its purpose is to
> +// overwrite the call of JITed code with 32 bits worth of an offset. This will
> +// is only meant to function on code that has been invalidated, so it should
> +// be totally safe. Since that instruction will never be executed again, a
> +// ICache flush should not be necessary

It is not clear from the function name that the code will never be executed again. This looks dangerous to me, because someone might assume that it is Ok to use this to emit code and suchlike. (The comment is clear, of course.)

@@ +1977,5 @@
> +// ICache flush should not be necessary
> +void
> +Assembler::patchWrite_Imm32(CodeLocationLabel label, Imm32 imm) {
> +    uint32 *raw = (uint32*)label.raw();
> +    *(raw-1) = imm.value;

It's not clear that you actually write to the word _before_ the label.

::: js/src/ion/arm/Assembler-arm.h
@@ +886,5 @@
>          return data != -1U;
>      }
>  };
>  
> +// A BOffImm is a branch off immediate.  This is the only sane way of constructing a branch.

I read it as "branch offset immediate", or a branch with an immediate offset. "branch off immediate" sounds as if it's doing something slightly different, though I'm not quite sure what.

@@ +1230,5 @@
>      // Given the start of a Control Flow sequence, grab the value that is finally branched to
>      static uint32 * getCF32Target(Instruction *jump);
>      // given the start of a function that loads an address into a register get the address that
>      // ends up in the register.
> +    static ImmWord getPointer(uint8 *);

This separate getPtr32Target from its comment.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +221,5 @@
>  {
>      as_nop();
>  }
> +
> +Instruction *nextInst(Instruction *i)

Following local conventions, the return type should have its own line.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +315,3 @@
>      void ma_call(void *dest);
> +
> +    // functions for generating patchable code and patching the code

Where? Was this comment left behind by accident?

@@ +412,5 @@
>      }
> +    void branch(IonCode *c) {
> +        BufferOffset bo = m_buffer.nextOffset();
> +        addPendingJump(bo, c->raw(), Relocation::IONCODE);
> +        ma_mov(Imm32((uint32)c->raw()), ScratchRegister);

I thought ma_mov couldn't take an arbitrary constant. (A quick grep seems to confirm this.) Also, if a non-patchable branch is required, why not use B? The only reason to do it this way is if you need more range than B can provide, or if you need interworking. In both cases, using LDR into the PC would surely be best.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +331,5 @@
>                                     sizeof(void *) * Registers::Total;
>  
>      masm.ma_ldr(Address(sp, 0), r1);
>      // Add 8 to make up for the 8 bytes of padding that were added to align the stack after
>      // the return-val was allocated.

This comment is now out of date.

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +869,5 @@
>      }
> +    static ImmWord getPointer(uint8 *instPtr) {
> +        uintptr_t *ptr = ((uintptr_t *) instPtr) - 1;
> +        return ImmWord(*ptr);
> +    }

It is not clear from the function name that this actually returns the word _before_ the one that instPtr points to.
Comment 2 Jacob Bramley [:jbramley] 2012-02-09 08:27:26 PST
(In reply to Jacob Bramley [:jbramley] from comment #1)
> @@ +412,5 @@
> >      }
> > +    void branch(IonCode *c) {
> > +        BufferOffset bo = m_buffer.nextOffset();
> > +        addPendingJump(bo, c->raw(), Relocation::IONCODE);
> > +        ma_mov(Imm32((uint32)c->raw()), ScratchRegister);
> 
> I thought ma_mov couldn't take an arbitrary constant. (A quick grep seems to
> confirm this.) Also, if a non-patchable branch is required, why not use B?
> The only reason to do it this way is if you need more range than B can
> provide, or if you need interworking. In both cases, using LDR into the PC
> would surely be best.

Sorry, I was wrong about ma_mov, of course. It still might not generate
a patchable move, though, in case that's important.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-02-10 15:44:42 PST
Created attachment 596200 [details] [diff] [review]
/home/mrosenberg/patches/refixOSI-r1.patch

Handing this off to dvander, since he knows how OSI works.  Fixed most of the easy to fix nits. I'd like to delay some of the more invasive changes until there aren't any known bugs on arm.  You may wish to only look at che changes between the two patches.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-02-10 15:51:24 PST
Created attachment 596201 [details] [diff] [review]
/home/mrosenberg/patches/refixOSI-r2.patch

whoops, forgot qrefresh.  All previous comments still apply.
Comment 5 David Anderson [:dvander] 2012-02-10 17:31:13 PST
Comment on attachment 596201 [details] [diff] [review]
/home/mrosenberg/patches/refixOSI-r2.patch

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

::: js/src/ion/CodeGenerator.cpp
@@ +1341,5 @@
>  
>      script->ion->setInvalidationEpilogueDataOffset(invalidateEpilogueData_.offset());
>      script->ion->setOsrPc(gen->info().osrPc());
>      script->ion->setOsrEntryOffset(getOsrEntryOffset());
> +    ptrdiff_t real_invalidate = masm.actualOffset((uint8 *)invalidate_.offset());

Can we get rid of this cast somehow?

::: js/src/ion/Ion.cpp
@@ +525,5 @@
>  {
>      memcpy(osiIndices(), oi, osiIndexEntries_ * sizeof(OsiIndex));
> +    for (int i = 0; i < osiIndexEntries_; i++) {
> +        osiIndices()[i].fixUpOffset(masm);
> +    }

No braces needed here.

::: js/src/ion/IonCaches.cpp
@@ +82,5 @@
> +void
> +CodeOffsetLabel::fixup(MacroAssembler *masm)
> +{
> +     JS_ASSERT(masm != NULL);
> +     offset_ = (size_t)masm->actualOffset((uint8*)offset_);

NULL assert not needed - just let it crash if it's NULL.

::: js/src/ion/Safepoints.cpp
@@ +180,5 @@
>  CodeLocationLabel
>  SafepointReader::InvalidationPatchPoint(IonScript *script, const SafepointIndex *si)
>  {
>      SafepointReader reader(script, si);
> +    // The size of a call is subtracted off, because the return address of the call to the

nit: newline above comment start

::: js/src/ion/arm/Assembler-arm.cpp
@@ +526,5 @@
>          JS_ASSERT(top->checkDest(temp));
>          uint32 *value = (uint32*) (targ_bot.decode() | (targ_top.decode() << 16));
>          if (dest != NULL)
>              *dest = temp;
> +        if (style != NULL)

style nit: if (dest), if (style)

::: js/src/ion/arm/Assembler-arm.h
@@ +1228,5 @@
> +    enum RelocStyle {
> +        L_MOVWT,
> +        L_LDR
> +    };
> +  public:

nit: newline above public:

@@ +1234,5 @@
>      // given the start of a function that loads an address into a register get the address that
>      // ends up in the register.
> +    static uint32 * getCF32Target(Instruction *jump);
> +
> +    static ImmWord getPointer(uint8 *);

For simplicity, let's make getPointer return a uintptr_t or a void * or uint8 * instead of ImmWord.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +1345,5 @@
> +    for (size_t i = 0; i < sizeof(void *); i+= Assembler::nopSize())
> +        masm.nop();
> +
> +    masm.bind(&invalidate_);
> +    // On arm, a NOP is 4 bytes.

nit: This and the other comments should be preceded by a newline.

@@ +1355,5 @@
> +#ifdef DEBUG
> +    // We should never reach this point in JIT code -- the invalidation thunk should
> +    // pop the invalidated JS frame and return directly to its caller.
> +    masm.breakpoint();
> +#endif

Actually, maybe we should just take the DEBUG off. If we execute past that point we're royally screwed anyway. Better to trap.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +222,5 @@
>      as_nop();
>  }
> +
> +Instruction *
> +nextInst(Instruction *i)

Convention for statics is to capitalize the first letter.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +310,5 @@
>      // callso an Ion function, assuming that sp has already been decremented
>      void ma_callIonNoPush(const Register reg);
>      // calls an ion function, assuming that the stack is currently not 8 byte aligned
>      void ma_callIonHalfPush(const Register reg);
> +    //void ma_callIonHalfPush(const IonCode *code);

Can this be removed?

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +334,2 @@
>      // Add an additional 16 to make up the difference between the bottom and top of the frame.
> +    masm.ma_add(sp, Imm32(BailoutDataSize + sizeof(size_t)*2), sp);

style nit: Spaces around *

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +306,1 @@
>          encodeSafepoint(safepoint);

Can you move this fixupOffset call into encodeSafepoint? And, no need to take in a masm - it's a member of CG-shared

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +989,5 @@
> +#ifdef DEBUG
> +    // We should never reach this point in JIT code -- the invalidation thunk should
> +    // pop the invalidated JS frame and return directly to its caller.
> +    masm.breakpoint();
> +#endif

Should be able to remove the DEBUG here too.

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