Last Comment Bug 736419 - IonMonkey: Code is not traced on ARM
: IonMonkey: Code is not traced on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-16 03:17 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-27 18:23 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/moreGCTracing-r0.patch (3.54 KB, patch)
2012-03-16 03:17 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Review
/home/mrosenberg/patches/moreGCTracing-r1.patch (6.20 KB, patch)
2012-04-06 06:54 PDT, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Review

Description Marty Rosenberg [:mjrosenb] 2012-03-16 03:17:46 PDT
Created attachment 606514 [details] [diff] [review]
/home/mrosenberg/patches/moreGCTracing-r0.patch

In addition to not always marking code that we branch to, the markers are never copied into the final object, so we never trace through code that is only referenced through jitted code.
Comment 1 Jacob Bramley [:jbramley] 2012-03-16 06:50:05 PDT
Comment on attachment 606514 [details] [diff] [review]
/home/mrosenberg/patches/moreGCTracing-r0.patch

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

I can't vouch that the GC tracing is now done correctly because I'm not sure how that works, but the patch looks sane. It'd probably be a good idea to get a second review.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1163,5 @@
>      ma_mov(Imm32(0xdeadbeef), ScratchRegister);
>  #endif
>      Push(ScratchRegister); // padding
>      Push(Imm32(descriptor)); // descriptor
>      // TODO: Use relocation here.

Is the TODO still valid?
Comment 2 Marty Rosenberg [:mjrosenb] 2012-04-06 06:54:55 PDT
Created attachment 612878 [details] [diff] [review]
/home/mrosenberg/patches/moreGCTracing-r1.patch

It looks like I forgot to land this, so I've updated it with the code to fix our latest v8 woes.
Comment 3 David Anderson [:dvander] 2012-04-06 11:26:17 PDT
Comment on attachment 612878 [details] [diff] [review]
/home/mrosenberg/patches/moreGCTracing-r1.patch

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

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1187,5 @@
>  #endif
>      Push(ScratchRegister); // padding
>      Push(Imm32(descriptor)); // descriptor
>      // TODO: Use relocation here.
> +    addPendingJump(m_buffer.nextOffset(), target->raw(), Relocation::IONCODE);

It looks like you can remove this :TODO: now. It seems like it would be better, though, to package this (addPendingJump; move; call) sequence into its own helper.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-07-27 18:23:29 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/572d5f29400e

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