Bug 164100 – Wasm starts a new stack whenever it adds a new block and has return types for blocks.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

We do this wrong at the moment.

[Keith Miller]

Created attachment 293693 [details]
WIP

[Keith Miller]

Created attachment 293753 [details]
Patch

[JF Bastien]

Comment on attachment 293753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293753&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        return value. Most of the control flow operators needed to be rewritten in able to

"in able to"

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:84
> +        ControlData(Procedure& proc, Type signature, BlockType type, BasicBlock* continuation, BasicBlock* special = nullptr)

What's special?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:571
> +        entry.expressionStack.append(m_currentBlock->appendNew<VariableValue>(m_proc, B3::Get, Origin(), result));

This can fail, right? I think we should use all the `try` variants everywhere so we soft-fail and bail out.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:54
> +        ExpressionList expressionStack;

Rename this to avoid the comment above.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:158
> +        returnValues.append(returnValue);

Same on allocation.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:314
> +        m_controlStack.append({ WTFMove(m_expressionStack), m_context.addBlock(inlineSignature) });

ditto (and then the other ones below, I'm lazy)

> Source/JavaScriptCore/wasm/WasmValidate.cpp:319
> +    // Think of this as penance for the sin of bad error messages.

?

[Mark Lam]

Comment on attachment 293753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293753&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        return value. Most of the control flow operators needed to be rewritten in able to
> 
> "in able to"

/rewritten in able to/rewritten to/.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:132
> +        void makeBlock()
> +        {
> +            ASSERT(type() == BlockType::If);
> +            blockType = BlockType::Block;
> +            special = nullptr;

Why is this assert true?  Seems pretty random.  If the sole purpose of this function is to convert an If ControlData to a Block ControlData, I suggest renaming the function to "convertIfToBlock" or "makeBlockFromIf".  That would make the assertion meaningful.

[Keith Miller]

Comment on attachment 293753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293753&action=review

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:132
>> +            special = nullptr;
> 
> Why is this assert true?  Seems pretty random.  If the sole purpose of this function is to convert an If ControlData to a Block ControlData, I suggest renaming the function to "convertIfToBlock" or "makeBlockFromIf".  That would make the assertion meaningful.

Sure, I'll change it to convertIfToBlock.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:571
>> +        entry.expressionStack.append(m_currentBlock->appendNew<VariableValue>(m_proc, B3::Get, Origin(), result));
> 
> This can fail, right? I think we should use all the `try` variants everywhere so we soft-fail and bail out.

Yeah, that's a fair point. I think we should do that in a follow up patch though.

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:54
>> +        ExpressionList expressionStack;
> 
> Rename this to avoid the comment above.

Fair enough. I changed it to enclosedExpressionStack.

[Keith Miller]

Created attachment 293771 [details]
Patch

[Saam Barati]

Comment on attachment 293771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293771&action=review

r=me with a couple comments

> Source/JavaScriptCore/testWasm.cpp:310
> +        Vector<uint8_t> vector = {

It'll be nice once we can make tests not using this API.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:611
> +    for (size_t i = 0; i < result.size(); ++i)

This loop doesn't look right to me. I think you want to be going from top of stack downwards on both, for result.size() iterations.
Maybe something like:
(unsigned i = 0; i < result.size(); i++) unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - i)

Can you add a test for this?

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146
> +    RELEASE_ASSERT_NOT_REACHED();

Why is this RELEASE_ASSERT_NOT_REACHED, if it is, maybe the above if (!parseUnreachableExpression(static_cast<OpType>(op))) should be an assertion.

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:476
> +    if (m_expressionStack.size()) {

that makes sense.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:319
> +    // Think of this as penance for the sin of bad error messages.

lol

[Keith Miller]

Comment on attachment 293771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293771&action=review

>> Source/JavaScriptCore/testWasm.cpp:310
>> +        Vector<uint8_t> vector = {
> 
> It'll be nice once we can make tests not using this API.

+1

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:611
>> +    for (size_t i = 0; i < result.size(); ++i)
> 
> This loop doesn't look right to me. I think you want to be going from top of stack downwards on both, for result.size() iterations.
> Maybe something like:
> (unsigned i = 0; i < result.size(); i++) unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - i)
> 
> Can you add a test for this?

Yeah, that's right. It's a little tricky to add a test for this since the only time this is wrong is when the sizes are not equal. The current version of the ML-proto I'm using to generate tests does not support extra stack values. I'll file a bug for it though in the future.

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146
>> +    RELEASE_ASSERT_NOT_REACHED();
> 
> Why is this RELEASE_ASSERT_NOT_REACHED, if it is, maybe the above if (!parseUnreachableExpression(static_cast<OpType>(op))) should be an assertion.

We don't want the if to be an assert since it might fail in the validator. Also, every exit from the above loop is a return so unless there is a compiler bug this assert is trivially dead.

[Keith Miller]

(In reply to comment #8)
> Comment on attachment 293771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293771&action=review
> 
> >> Source/JavaScriptCore/testWasm.cpp:310
> >> +        Vector<uint8_t> vector = {
> > 
> > It'll be nice once we can make tests not using this API.
> 
> +1
> 
> >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:611
> >> +    for (size_t i = 0; i < result.size(); ++i)
> > 
> > This loop doesn't look right to me. I think you want to be going from top of stack downwards on both, for result.size() iterations.
> > Maybe something like:
> > (unsigned i = 0; i < result.size(); i++) unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - i)
> > 
> > Can you add a test for this?
> 
> Yeah, that's right. It's a little tricky to add a test for this since the
> only time this is wrong is when the sizes are not equal. The current version
> of the ML-proto I'm using to generate tests does not support extra stack
> values. I'll file a bug for it though in the future.

I should note that updating the ml-proto would require changing all the opcode numbers in the existing tests. That would be an unfortunately large amount of work.

> 
> >> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146
> >> +    RELEASE_ASSERT_NOT_REACHED();
> > 
> > Why is this RELEASE_ASSERT_NOT_REACHED, if it is, maybe the above if (!parseUnreachableExpression(static_cast<OpType>(op))) should be an assertion.
> 
> We don't want the if to be an assert since it might fail in the validator.
> Also, every exit from the above loop is a return so unless there is a
> compiler bug this assert is trivially dead.

[Keith Miller]

Created attachment 293786 [details]
Patch for landing

[Keith Miller]

Created attachment 293796 [details]
Patch for landing

[Keith Miller]

Initially committed in r208341: <http://trac.webkit.org/changeset/208341>

Changelog was fixed in r208343: <http://trac.webkit.org/changeset/208343>