1435756 - Wasm x86/x64: probable incorrect fallthrough in outOfLineTruncateWhatever emitters when isUnsigned = true

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

I'm not 100% sure about this one yet but outOfLineWasmTruncateDoubleToInt32() and outOfLineWasmTruncateFloat32ToInt32() in MacroAssembler-x86-shared.cpp both return in the isUnsigned case instead of jumping to the rejoin label.

Adding the jumps there introduces a lot of crashes, because the rejoin label is unbound in visitWasmTruncateToInt32() in CodeGenerator-x86-shared.cpp in the isUnsigned case.

Fixing that, there are now test failures in wasm/errors.js, wasm/conversion.js, and wasm/spec/conversions.wast.js.

It's possible that this is the way it's supposed to be but in that case a pattern like this deserves a comment or two!

Comment 1

[Lars T Hansen [:lth]]

OK, this may be by design but the lack of comments around how it works makes it a bug, more or less.

What happens is that in the isUnsigned case the code just falls through rather than emitting a jump to the rejoin label.  Because there is an RAII instance of AutoHandleWasmTruncateToIntErrors in scope, it generates code on the way out of the masm function.  This code consists of two jumps.  The first of these jumps jumps to IntegerOverflow, the second jumps to InvalidConversionToInteger.  Both jumps have a label, so the naive reader would consider it fair game to reorder them.  This would be wrong, because in this case, the isUnsigned case code would fall through to the wrong jump.  As it is, it falls through to the IntegerOverflow label, which is the right error case.

What is actually the case in the isUnsigned case is not that everything is OK, as the comment in the code might lead one to believe, but that everything is an overflow error, and we should jump to the overflow case always.  The apparent bug is not a missing jump to rejoin, it's a missing jump to a failure label.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1435756-fallthrough-clarification.patch

Add comments and rename a variable to clarify (and bless) the behavior.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8948493 [details] [diff] [review]
bug1435756-fallthrough-clarification.patch

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

Thanks for adding these comments! (The fact I couldn't remember why I did it that way in the first place is a strong hint that comments were needed)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95245d5d7560
Clarify fallthrough in generated code for wasm truncate.  r=bbouvier

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/95245d5d7560