The default bug view has changed. See this FAQ.

IonMonkey: Code is not traced on ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
Attachment #606514 - Flags: review?(Jacob.Bramley)
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?
Attachment #606514 - Flags: review?(Jacob.Bramley) → review+
(Reporter)

Comment 2

5 years ago
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.
Attachment #612878 - Flags: review?(dvander)
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.
Attachment #612878 - Flags: review?(dvander) → review+
(Reporter)

Comment 4

5 years ago
landed: http://hg.mozilla.org/projects/ionmonkey/rev/572d5f29400e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.