1360254 - kill WasmActivation::prevWasm_ and JSContext::wasmActivationStack_

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Luke Wagner [:luke]]

These are some of the oldest asm.js impl details and with a bit of care around signal handling could be changed to just use Activation::prev_/cx->activation_.

Comment 1

[Luke Wagner [:luke]]

Attachment: rm-wasm-activation-stack

Actually, surprisingly easy after bug 1334504 and related work.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8863055 [details] [diff] [review]
rm-wasm-activation-stack

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

Thanks, the patch looks good.

I've tried to look through the blame history, but I can't find a proper answer to this question: why did we need the wasmActivationStack in the first place? This patch suggests that it was for signal-safety; which kind of signal-unsafe situations could happen?

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1557,5 @@
>  {
>      void* pc = (void*)get_pc();
>      uint8_t* fp = (uint8_t*)get_register(r11);
>  
> +    WasmActivation* activation = wasm::MaybeActiveActivation(cx_);

Maybe assert !!activation?

::: js/src/vm/Stack.h
@@ +1730,1 @@
>  //    from the profiler at arbitrary points

This comment needs an update.

::: js/src/wasm/WasmBuiltins.cpp
@@ +68,5 @@
> +static WasmActivation*
> +CallingActivation()
> +{
> +    Activation* act = TlsContext.get()->activation();
> +    MOZ_ASSERT(!act->asJit()->isActive(), "WasmCall pushes an inactive JitActivation");

It can't happen that the interrupt handler is called while we're in the JIT exit prologue/epilogue (just before/after we set the JitActivation as active/inactive), because the execution interrupt happens only when we're in function code, not in JIT exit prologues/epilogues, right?

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +1309,4 @@
>  
> +    // Only interrupt in function code so that the frame iterators have the
> +    // invariant that resumePC always has a function CodeRange and we can't
> +    // get into any weird interrupt-during-interrupt-stub cases. Note that

I do note that... something is missing here, or this stub sentence should be removed.

@@ +1336,5 @@
> +    if (activation->interrupted())
> +        return false;
> +
> +    activation->startInterrupt(pc, fp);
> +    *ContextToPC(context) = code->segment().interruptCode();

Much more readable, thanks!

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8863055 [details] [diff] [review]
rm-wasm-activation-stack

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

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +834,4 @@
>      if (!activation)
>          return false;
>  
>      const Code* code = activation->compartment()->wasm.lookupCode(pc);

Also, why don't we need to invert MaybeActiveActivation and lookupCode here too?

Comment 4

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> I've tried to look through the blame history, but I can't find a proper
> answer to this question: why did we need the wasmActivationStack in the
> first place? This patch suggests that it was for signal-safety; which kind
> of signal-unsafe situations could happen?

Well the vague concern was that cx->activation_ can be updated in multiple places and not all are signal safe.  This concern is addressed in the current code by only probing MaybeActiveActivation when pc is known to be in wasm code in RedirectJitCodeToInterruptCheck().

> > +    WasmActivation* activation = wasm::MaybeActiveActivation(cx_);
> 
> Maybe assert !!activation?

I usually leave these off when the next line dereferences the pointer since it'll be just as easy to diagnose in crashes.

> > +    MOZ_ASSERT(!act->asJit()->isActive(), "WasmCall pushes an inactive JitActivation");
> 
> It can't happen that the interrupt handler is called while we're in the JIT
> exit prologue/epilogue (just before/after we set the JitActivation as
> active/inactive), because the execution interrupt happens only when we're in
> function code, not in JIT exit prologues/epilogues, right?

Exactly

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e087957c2c6
Baldr: remove JSContext::wasmActivationStack (r=bbouvier)

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fede60d21809
Backed out changeset 3e087957c2c6

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e5a6e086f8
Baldr: remove JSContext::wasmActivationStack (r=bbouvier)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a5e5a6e086f8

Comment 9

[menantea]

Hi there, not sure if I should reopen this bug but since this commit, firefox crash during start in pcc64le architecture.

Tests have been done on fedora 27 with firefox-57.0.4-1.fc27 (https://koji.fedoraproject.org/koji/buildinfo?buildID=1013399) and related bugs reported as https://bugzilla.redhat.com/show_bug.cgi?id=1529572 https://bugzilla.redhat.com/show_bug.cgi?id=1498561 https://bugzilla.redhat.com/show_bug.cgi?id=1486737

It seems a signal is received soon in the process, this signal is processed thru RedirectJitCodeToInterruptCheck() function in WasmSignalHandlers.cpp and when it reach *ContextToPC(context) , it crashes because we are defined as JS_CODEGEN_NONE for powerpc.
Before the change, this call to *ContextToPC(context) was protected by following test:
if (WasmActivation* activation = cx->wasmActivationStack()) { 

I can get a firefox running fine so far with a quick dirty patch like:
--- firefox-57.0.4/js/src/wasm/WasmSignalHandlers.cpp.ori
+++ firefox-57.0.4/js/src/wasm/WasmSignalHandlers.cpp
@@ -1482,6 +1482,9 @@
 #ifdef JS_SIMULATOR
     uint8_t* pc = cx->simulator()->get_pc_as<uint8_t*>();
 #else
+    WasmActivation* is_activated = ActivationIfInnermost(cx);
+    if (!is_activated)
+        return false;
     uint8_t* pc = *ContextToPC(context);
 #endif

I am not fluent with firefox code and comments about activation, few lines after in the code, let me guess this patch is not the right way to do and it could create side effects.
Please, could you help on this ?
Thanks

Comment 10

[Luke Wagner [:luke]]

By any lucky chance, is the crash fixed with the recent landing of
  https://hg.mozilla.org/mozilla-central/rev/16740cf93a77
?  In particular, if KNOWS_MACHINE_STATE gets defined:
  https://hg.mozilla.org/mozilla-central/rev/16740cf93a77#l2.12
then ContextToPC() should work instead of crash.

Comment 11

[menantea]

for ppc64le, KNOWS_MACHINE_STATE is not defined then ContextToPC still crash. In fact ContextToPC should not be called at all. I don't know why we have a signal before the activation process but it seems it was always there, it was just ignored because of test of wasmActivationStack() in the previous code.

Comment 12

[Luke Wagner [:luke]]

The signal is used to halt iloops.  Could you add the necessary defines so that KNOWS_MACHINE_STATE is defined?