Bug 175347 - JSTests/slowMicrobenchmarks/spread-small-array.js fails with JSC_validateExceptionChecks=1
Summary: JSTests/slowMicrobenchmarks/spread-small-array.js fails with JSC_validateExce...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Trivial
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-08 15:10 PDT by Robin Morisset
Modified: 2017-08-08 17:26 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch (7.89 KB, patch)
2017-08-08 15:17 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Same patch, with the whitespace issues fixed (7.93 KB, patch)
2017-08-08 15:30 PDT, Robin Morisset
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
Same patch, with Changelog fix (7.93 KB, patch)
2017-08-08 15:43 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Same patch, with Mark's suggestion (7.95 KB, patch)
2017-08-08 16:10 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
exception_scope => exceptionScope, as suggested by Mark (7.95 KB, patch)
2017-08-08 16:28 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
exceptionScope => throwScope (7.94 KB, patch)
2017-08-08 16:44 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2017-08-08 15:10:46 PDT
This is due to finishCreation not explicitely checking for exceptions between the calls to setConstantRegisters and setConstantIdentifiersSetRegisters.
Comment 1 Robin Morisset 2017-08-08 15:17:54 PDT
Created attachment 317626 [details]
Proposed patch
Comment 2 Build Bot 2017-08-08 15:20:22 PDT
Attachment 317626 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:881:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:883:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:889:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:922:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 10 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Robin Morisset 2017-08-08 15:30:22 PDT
Created attachment 317631 [details]
Same patch, with the whitespace issues fixed
Comment 4 Saam Barati 2017-08-08 15:32:30 PDT
Comment on attachment 317631 [details]
Same patch, with the whitespace issues fixed

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

r=me with changelog fix

> Source/JavaScriptCore/ChangeLog:10
> +        This is done by making finishCreation explicitely check for exceptions after setConstantRegister and setConstantIdentifiersSetRegisters.
> +        I chose to have this check replace the boolean returned previously by these functions for readability. The performance impact should be
> +        negligible considering how much more finishCreation does.
> +        This fix then caused another issue to appear as it was now clear that finishCreation can throw. And since it is called by ProgramCodeBlock::create(),
> +        FunctionCodeBlock::create() and friends, that are in turn called by ScriptExecutable::newCodeBlockFor, this last function also required a few tweaks.

Style nit: This explanation should go below the "Reviewed by ..."
Comment 5 Build Bot 2017-08-08 15:33:12 PDT
Attachment 317631 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:881:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:883:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:889:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:922:  Extra space before )  [whitespace/parens] [2]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Robin Morisset 2017-08-08 15:43:49 PDT
Created attachment 317634 [details]
Same patch, with Changelog fix
Comment 7 Build Bot 2017-08-08 15:51:06 PDT
Attachment 317634 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:881:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:883:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:889:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:922:  Extra space before )  [whitespace/parens] [2]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Mark Lam 2017-08-08 15:55:32 PDT
Comment on attachment 317634 [details]
Same patch, with Changelog fix

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:881
> +        RETURN_IF_EXCEPTION(scope, );

This looks weird.  Let's do this instead:
     RETURN_IF_EXCEPTION(scope, void());

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:883
> +        RETURN_IF_EXCEPTION(scope, );

Ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:889
> +            RETURN_IF_EXCEPTION(scope, );

Ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:922
> +                RETURN_IF_EXCEPTION(scope, );

Ditto.
Comment 9 Robin Morisset 2017-08-08 16:10:58 PDT
Created attachment 317638 [details]
Same patch, with Mark's suggestion
Comment 10 Robin Morisset 2017-08-08 16:28:05 PDT
Created attachment 317641 [details]
exception_scope => exceptionScope, as suggested by Mark
Comment 11 Mark Lam 2017-08-08 16:30:11 PDT
Comment on attachment 317641 [details]
exception_scope => exceptionScope, as suggested by Mark

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:404
> +    auto exceptionScope = DECLARE_THROW_SCOPE(vm);

can we call this throwScope instead since it comes from DECLARE_THROW_SCOPE?
Comment 12 Robin Morisset 2017-08-08 16:44:23 PDT
Created attachment 317648 [details]
exceptionScope => throwScope
Comment 13 WebKit Commit Bot 2017-08-08 17:26:07 PDT
Comment on attachment 317648 [details]
exceptionScope => throwScope

Clearing flags on attachment: 317648

Committed r220432: <http://trac.webkit.org/changeset/220432>
Comment 14 WebKit Commit Bot 2017-08-08 17:26:09 PDT
All reviewed patches have been landed.  Closing bug.