Closed Bug 1527259 Opened 2 years ago Closed 2 years ago

Fix wasm-baseline stackmap boundaries at function calls


(Core :: Javascript: WebAssembly, defect)

Not set



Tracking Status
firefox67 --- fixed


(Reporter: jseward, Assigned: jseward)




(1 file)

The boundaries of baseline-created stackmaps, specifically the lowest and highest addressed stack words covered by the map, are not quite correct. For baseline-to-baseline calls, this isn't a problem; the map boundaries in this domain are internally self-consistent. But when calling to or from Ion-generated code, it becomes apparent that the Ion maps (bug 1517924) disagree with Baseline maps in such a way that, depending on the direction of the call, either there could be words on the stack covered by neither map (caller nor callee), or there could be words covered by both maps.

In detail: baseline so far assumes that the size of the outbound argument area is stackArgAreaSize(callee arg types). That however 16-aligns the size and in the process can introduce up to 12 bytes of padding. In baseline that doesn't matter; via Ion, however, that 12 bytes can (and has been seen to) belong to the caller's spill area. So it shouldn't be covered by the callee's map.

The fix is simple in essence: split stackArgAreaSize into pre- and post-alignment parts and use the pre- rather than the post-alignment part, as required.

Blocks: 1508559

A candidate patch. This fixes the problem and renames a couple of variables
(see commit message). It doesn't however abstract out and assert about, the
alignment value 16, as mentioned near the end of bug 1517924 comment 3 -- that
requires further study and would fit well into the alignment fixes discussed
in bug 1527274.

Assignee: nobody → jseward
Attachment #9043259 - Flags: review?(lhansen)
Comment on attachment 9043259 [details] [diff] [review]

Review of attachment 9043259 [details] [diff] [review]:

Seems fine, modulo misc naming nits.  I can live with "inbound" and "outbound" but I would probably prefer "incoming" and "outgoing".  Do argue, if you feel strongly about it (or for any reason at all).

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +2225,5 @@
> +    // and comprises two components:
> +    //
> +    // * call->frameAlignAdjustment
> +    // * the padding applied to the stack arg area itself.  That is:
> +    //   StackArgAreaSize(argTys) - StackArgAreaSizeUnpadded(argTys)

StackArgAreaSize is not defined.

@@ +4050,5 @@
>      // add entries to mst_ as appropriate.
>      const ValTypeVector& argTys = env_.funcTypes[func_.index]->args();
> +    size_t nInboundStackArgBytes = StackArgAreaSizeUnaligned(argTys);

As a general nit, I think "incoming" and "outgoing" are more common than "inbound" and "outbound".

@@ +4410,5 @@
> +        // However much we've pushed so far
> +        masm.framePushed() +
> +        // Extra space we'll push to get the frame aligned
> +        call->frameAlignAdjustment +
> +        // Extra space we'll push to get the arg area 16-aligned

How about "the outbound arg area" (or "outgoing", if appropriate).

@@ +4432,5 @@
>    // builds.
>    //
>    // The bulk of the work here (60%) is in the next() call, though.
>    //
> +  // Notably, since next() is so expensive, StackArgAreaSize() becomes

StackArgAreaSize() is not defined.

@@ +4438,5 @@
>    //
>    // Somehow there could be a trick here where the sequence of
>    // argument types (read from the input stream) leads to a cached
> +  // entry for StackArgAreaSize() and for how to pass arguments...
> +  //

Nor here.

@@ +4439,5 @@
>    // Somehow there could be a trick here where the sequence of
>    // argument types (read from the input stream) leads to a cached
> +  // entry for StackArgAreaSize() and for how to pass arguments...
> +  //
> +  // But at least we could reduce the cost of StackArgAreaSize() by

Nor here.
Attachment #9043259 - Flags: review?(lhansen) → review+
Blocks: 1517924
Pushed by
Fix wasm-baseline stackmap boundaries at function calls.  r=lhansen.
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.