1597271 - Missing resumeAfter in inlined jit-to-wasm call

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Lars T Hansen [:lth]]

Presumably this is related to bug 1524373.

The context is IonBuilder::inlineWasmCall in MCallOptimize.cpp (ca line 4230) as well as patch https://bugzilla.mozilla.org/show_bug.cgi?id=1524373 where Benjamin commented that I may need a resumeAfter after an inserted allocation (https://phabricator.services.mozilla.com/D50031#inline-321643). I then observed that there were several other effectful conversions there that did not have a resumeAfter, and asked whether they too needed them.

Benjamin then said that the resumeAfter that is there after the call was the result of a two-week-long sec bug bughunt this spring and started worrying about there being more problems, hence this bug.

I will ni? some people who I hope can answer the question, "do we need a resumeAfter after each of these effectful conversions"?

(Filing as JIT and not wasm since this is technically on the JIT side.)

Comment 1

[Jan de Mooij [:jandem]]

The rules are:

1. resumeAfter must be called after every observable (from JS) MIR instruction. This ensures that after a bailout to Baseline Interpreter we don't resume before the effectful operation (and perform it twice).

2. resumeAfter, when used, must be called after the last MIR instruction for a given bytecode op. This is because bailouts have bytecode instruction 'granularity' (we can't bailout in the middle of a bytecode instruction).

So what this means for inlined calls: I'd expect there to be 0..N non-observable-from-JS instructions followed by the final resumeAfter call instruction.

Does that help?

Comment 2

[Lars T Hansen [:lth]]

Yes, that's very helpful, though for inlined calls there are effectful representation conversion operations that are emitted as part of the inlined call, so likely what we're looking at here is one resumeAfter after each of those conversions (rule 1) and then one resumeAfter after the call (rule 2, but also rule 1 because the call is effectful). Is that sensible?

Edit: I need to check whether the effects of the representation conversions are expressed as type policies, because if they are then the conversions are not effectful.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #2)

Yes, that's very helpful, though for inlined calls there are effectful representation conversion operations that are emitted as part of the inlined call, so likely what we're looking at here is one resumeAfter after each of those conversions

I don't think that works (conflicts with rule 2). Imagine what happens if the effectful conversion happens to trigger a GC (or similar) that invalidates the Ion code on the stack. We'd then bail out in a weird state (right after the JSOP_CALL I expect) but we haven't performed the call yet. There should be at most one resumeAfter per bytecode op.

Comment 4

[Nicolas B. Pierron [:nbp]]

Ok, looking at the code of WasmValueBox::create, there is no need for a ResumeAfter resume point. This function only changes internal data of the GC, which are not observable from the JavaScript execution point-of-view.

A similar example is small arrays/object allocations with only copied fields (no JS re-entry).

Newly allocated objects (beyond OOMs) is non-observable and can safely be replayed as long as the object is not stored in pre-existing locations. This property is used by Escape Analysis & Scalar Replacement, to replace object allocation by a recover instruction which remove/delay the object allocation.

Comment 5

[Lars T Hansen [:lth]]

nbp: Actually this is not about the new WasmValueBox, but about the old conversions that are already inserted in that code: MToDouble and so on.

What I'm reading from Jan's rules is that there may be at most one resumeAfter per "MIR instruction" (which is not a "MIR node", but really a "bytecode"?) and if there is one it must be at the end, and furthermore that if the bytecode gives rise to multiple MIR nodes then those must not have observable effects because there's no way of bailing out from the middle without restarting from the beginning.

Now MToDouble and MToFloat32 have ToDoublePolicy so that should be OK. MTruncateToInt32 however is clearly effectful and has no interesting type policy, so it may bail out. Since there can be several arguments that have this conversion, it seems that some conversions may be replayed if there's a bailout?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #5)

Since there can be several arguments that have this conversion, it seems that some conversions may be replayed if there's a bailout?

Correct, these conversion instructions inserted before the 'actual' MIR instruction are typically non-effectful. If they bail out we resume at the last resume point (some earlier bytecode instruction).

Comment 7

[Lars T Hansen [:lth]]

OK, so it sounds like there's possibly a bug here with the MTruncateToInt32 node, but I will investigate more deeply in the morning.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #5)

Now MToDouble and MToFloat32 have ToDoublePolicy so that should be OK. MTruncateToInt32 however is clearly effectful and has no interesting type policy, so it may bail out. Since there can be several arguments that have this conversion, it seems that some conversions may be replayed if there's a bailout?

Indeed MTruncateToInt32 is effectful by re-entring JS in case of valueOf function on JS Objects.
Thus a simple test case which would highlight the problem might look something like:

bailAfter(1);
f({ valueOf: () => { console.log("1"); return 1; } }, { valueOf: () => { console.log("2"); return 2; } })

The simplest approach would be to rely on TI to refuse inlining the Wasm call if MTruncateToInt32 might be an object.
This can be done by repeating the condition which is in the MTruncateToInt32 constructor ( https://searchfox.org/mozilla-central/source/js/src/jit/MIR.h#4175 ) before generating any MIR instructions. Which implies to loop over the arguments to do this verification here ( https://searchfox.org/mozilla-central/source/js/src/jit/MCallOptimize.cpp#4269 )

Comment 9

[Lars T Hansen [:lth]]

Thanks, that's helpful.

Comment 10

[Lars T Hansen [:lth]]

I think this is not a problem. The way it is set up, all effectful conversions bail out immediately; no effects are visible without bailout. Therefore the first effectful conversion will always bail out, and the call is not inlined. Work, including allocation, may have been done before that but is not effectful and it doesn't matter if it's redone. The MTruncateToInt32 has a case for Value that handles everything that is effectful. That lowers to an LValueToInt32, which bails out for almost anything but especially for Object.

I'll add a comment to this effect when I land my other patch, so that we can remember that this is an invariant.