Bug 204998 - [WebAssembly] Fix LLIntGenerator's checkConsistency contract
Summary: [WebAssembly] Fix LLIntGenerator's checkConsistency contract
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-08 10:30 PST by Tadeu Zagallo
Modified: 2019-12-08 17:35 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.59 KB, patch)
2019-12-08 10:36 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (4.73 KB, patch)
2019-12-08 16:52 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-12-08 10:30:57 PST
...
Comment 1 Tadeu Zagallo 2019-12-08 10:31:48 PST
<rdar://problem/57733405>
Comment 2 Tadeu Zagallo 2019-12-08 10:36:17 PST
Created attachment 385124 [details]
Patch
Comment 3 Mark Lam 2019-12-08 12:55:35 PST
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.
Comment 4 Tadeu Zagallo 2019-12-08 16:51:40 PST
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.
Comment 5 Tadeu Zagallo 2019-12-08 16:52:08 PST
Created attachment 385128 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2019-12-08 17:35:23 PST
Comment on attachment 385128 [details]
Patch for landing

Clearing flags on attachment: 385128

Committed r253280: <https://trac.webkit.org/changeset/253280>
Comment 7 WebKit Commit Bot 2019-12-08 17:35:24 PST
All reviewed patches have been landed.  Closing bug.