Bug 204998 – [WebAssembly] Fix LLIntGenerator's checkConsistency contract

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

[Tadeu Zagallo]

...

[Tadeu Zagallo]

<rdar://problem/57733405>

[Tadeu Zagallo]

Created attachment 385124 [details]
Patch

[Mark Lam]

Comment on attachment 385124 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        In this patch I also removed the consistency check from calls to push in addArguments and addLocal, since at
> +        that point the expression stack must be empty. If that wasn't the case, those calls would be incorrect for the
> +        same reason as above: we perform several pushes in a row, which means that the stacks would be out of sync.

At the top of FunctionParser<Context>::parse(), I suggest ASSERT(m_expressionStack.isEmpty()).  This ensures and documents that a consistency check is not needed there.  Alternatively, you can just call checkConsistency() there anyway, which makes it even clearer.  It's effectively a no-op, but will clearly document that we know the expression stack is consistent at that point.

[Tadeu Zagallo]

Comment on attachment 385124 [details]
Patch

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

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:18
>> +        same reason as above: we perform several pushes in a row, which means that the stacks would be out of sync.
> 
> At the top of FunctionParser<Context>::parse(), I suggest ASSERT(m_expressionStack.isEmpty()).  This ensures and documents that a consistency check is not needed there.  Alternatively, you can just call checkConsistency() there anyway, which makes it even clearer.  It's effectively a no-op, but will clearly document that we know the expression stack is consistent at that point.

That makes sense. I just added the checkConsistency call at the beginning of addArguments and addLocal.

[Tadeu Zagallo]

Created attachment 385128 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 385128 [details]
Patch for landing

Clearing flags on attachment: 385128

Committed r253280: <https://trac.webkit.org/changeset/253280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.