Bug 150333 - JS Built-ins functions should be able to assert
Summary: JS Built-ins functions should be able to assert
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 150496
  Show dependency treegraph
 
Reported: 2015-10-19 09:54 PDT by youenn fablet
Modified: 2015-11-09 06:38 PST (History)
7 users (show)

See Also:


Attachments
Patch (38.53 KB, patch)
2015-11-05 05:19 PST, youenn fablet
no flags Details | Formatted Diff | Diff
WIP (13.68 KB, patch)
2015-11-06 09:51 PST, youenn fablet
no flags Details | Formatted Diff | Diff
build fix (14.02 KB, patch)
2015-11-09 02:21 PST, youenn fablet
no flags Details | Formatted Diff | Diff
iOS build fix (14.01 KB, patch)
2015-11-09 02:36 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-10-19 09:54:27 PDT
Currently JS built-ins cannot assert.
It might be good to add this feature, as illustrated by TODOs in Source/WebCore/Modules/streams/ReadableStreamInternals.js
Comment 1 youenn fablet 2015-10-19 10:20:00 PDT
One potential implementation is to insert in js built-ins code such as @assert(boolean_expression...).

The built-in js generator would remove these lines in release mode.
In debug mode, they might insert lines such as @assert(boolean_expression "boolean_expression") or if(boolean_expression) @wtfCrash("boolean_expression").
Comment 2 youenn fablet 2015-11-05 05:19:38 PST
Created attachment 264853 [details]
Patch
Comment 3 WebKit Commit Bot 2015-11-05 05:21:43 PST
Attachment 264853 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:71:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_embedded_code_string_section_for_function' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:71:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsCombinedImplementationGenerator'  [pylint/E0602] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:76:  [generate_bindings_for_builtins_files] Undefined variable 'BuiltinsSeparateImplementationGenerator'  [pylint/E0602] [5]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Xabier Rodríguez Calvar 2015-11-05 06:27:57 PST
Comment on attachment 264853 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        Updated build system so that Debug CMake builds enable --generate-assertions flag.

Is this going to work only for CMake based builds? Nothing for Mac or Win?

> Source/JavaScriptCore/Scripts/builtins/builtins_model.py:47
> +assertRegExp = re.compile(r"^(\s*)ASSERT\(\"([^\"]+)\"\);\n", re.MULTILINE | re.S)

I know you can easily do ASSERT("false"), but it would be cool to be able to do ASSERT_NOT_REACHED() too. I guess this could be done in a follow up patch too.
Comment 5 youenn fablet 2015-11-05 06:51:48 PST
(In reply to comment #4)
> Comment on attachment 264853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264853&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        Updated build system so that Debug CMake builds enable --generate-assertions flag.
> 
> Is this going to work only for CMake based builds? Nothing for Mac or Win?

Win is CMake based and Mac is planned to go to CMake at some point IIANM.
To add support for it now, we probably need to touch the scripts using DerivedSources.make.
I would be tempted to just wait but would be glad to see that happening...

> I know you can easily do ASSERT("false"), but it would be cool to be able to
> do ASSERT_NOT_REACHED() too. I guess this could be done in a follow up patch
> too.

I am not sure how often we will have use for ASSERT_NOT_REACHED in JS built-ins. If that will happen often, we should probably add it at some point.

In the laundry list, it would also be cool to add JS line number in the assert message.
Comment 6 Geoffrey Garen 2015-11-05 10:42:21 PST
Comment on attachment 264853 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Built-in generator translates ASSERT("...") instructions to @assert calls when --generate-assertions flag is set.
> +        Otherwise ASSERT instructions are stripped.

Why not just put "@assert" in the source directly?

> Source/JavaScriptCore/Scripts/tests/builtins/expected/JavaScriptCore-Builtin.Promise-Separate-Debug.js-result:119
> +    "    if (@assert) @assert(!!this, \"doSomething\", \"!!this\");\n" \

Why do we have to check for presence of @assert, if we only generate this code when it is present?
Comment 7 Alex Christensen 2015-11-05 11:07:08 PST
Comment on attachment 264853 [details]
Patch

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

I think it's strange to remove ASSERT(...) in release builds.  Is there some other way we could do this?

>>> Source/JavaScriptCore/ChangeLog:11
>>> +        Updated build system so that Debug CMake builds enable --generate-assertions flag.
>> 
>> Is this going to work only for CMake based builds? Nothing for Mac or Win?
> 
> Win is CMake based and Mac is planned to go to CMake at some point IIANM.
> To add support for it now, we probably need to touch the scripts using DerivedSources.make.
> I would be tempted to just wait but would be glad to see that happening...

There are not official plans to make Mac officially CMake based.  If that does ever happen, it will be a while.

> Source/JavaScriptCore/Scripts/tests/builtins/JavaScriptCore-Builtin.Promise-Separate-Release.js:13
> + * THIS SOFTWARE IS PROVIDED BY Canon INC. ``AS IS'' AND ANY

Canon is not all-caps.  Also, sometimes APPLE is used in this patch.  I'm not sure what the policy about this is.
Comment 8 BJ Burg 2015-11-05 11:17:24 PST
Comment on attachment 264853 [details]
Patch

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

Overall this looks good, but it needs a little more work to be less of a hazard. In particular, the generator should error out on the following cases:

 * assert inside control flow block without braces
 * multi-line assert
 * assert with other statements before/after it on the same line

You can look at the inspector's assert stripper (Source/WebInspectorUI/Scripts/remove-console-asserts.pl) for inspiration how to do this.

As for DerivedSources.make, you should be able to do something similar to CMake, by doing something like:

ifeq ($(CONFIGURATION),Debug)
ENABLE_JSBUILTIN_ASSERTIONS = --enable-assertions
else
ENABLE_JSBUILTIN_ASSERIONS = 
endif

However, it may be better to just pass the configuration itself (i.e., --configuration debug) and let the generator decide what to do for each. Otherwise, we could end up with a lot of flags.

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:71
> +            sections.append(self.generate_embedded_code_string_section_for_function(function, self._enable_assertions))

See note below.

> Source/JavaScriptCore/Scripts/builtins/builtins_model.py:120
> +    def source(self, enable_assertions):

Please move this code into the BuiltinsGenerator base class. BuiltinFunction is a model object, it shouldn't be responsible for source transformations during code generation. Then you won't need to pass any flags either.

> Source/JavaScriptCore/Scripts/generate-js-builtins.py:-63
> -                (_, object_name, _) = file_name.split('-')

It would be easier to read this if you added another placeholder destructuring argument instead of slicing it.

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:55
> +    ASSERT("stream.@state === @streamErrored");

I don't like quotes here. It will break syntax highlighting.

> Tools/Scripts/webkitpy/codegen/main.py:41
> +    def generate_from_js_builtins(self, builtins_file, output_directory, framework_name="", combined_outputs=False, build_mode="Release"):

I would call it the 'configuration' or 'build_type' to match other build script code. On OS X, valid values are Debug, Release, and Production.

> Tools/Scripts/webkitpy/codegen/main.py:111
> +            (framework_name, test_case, output_mode) = parameters[:3]

Please just add another destructuring variable to the LHS. This change is harder to read.
Comment 9 BJ Burg 2015-11-05 11:26:14 PST
Comment on attachment 264853 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        Otherwise ASSERT instructions are stripped.
> 
> Why not just put "@assert" in the source directly?

I agree, it is more consistent to use @assert and just delete the whole line if we need to strip (but see caveats in my other comment). This is what WebInspectorUI does for Production builds.

>> Source/JavaScriptCore/Scripts/tests/builtins/JavaScriptCore-Builtin.Promise-Separate-Release.js:13
>> + * THIS SOFTWARE IS PROVIDED BY Canon INC. ``AS IS'' AND ANY
> 
> Canon is not all-caps.  Also, sometimes APPLE is used in this patch.  I'm not sure what the policy about this is.

Please use the license block checked in here: http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE (notably, APPLE INC AND ITS CONTRIBUTORS differs here)

You should only need to alter the copyright lines at the top.
Comment 10 Yusuke Suzuki 2015-11-05 20:23:56 PST
Comment on attachment 264853 [details]
Patch

I think adding @assert bytecode instrinsics is better than adding @assert function.
It can completely control the bytecodes emitted by @assert(...);

For example,
+ under the debug build, @assert will emit assertion
+ under the release build, it will emit nothing

See bytecode/BytecodeIntrinsicRegistry.cpp and @putByValDirect implementation for more details.
Comment 11 youenn fablet 2015-11-06 08:08:08 PST
Thanks everybody for your comments.
I will try to prepare a new version of the patch taking into account all comments.

The intrinsic bytecode approach is intriguing to me since I do not know much about this. I will definitely look at it.
It could resolve around creating a new assert bytecode that would only happen in debug mode.
Comment 12 youenn fablet 2015-11-06 09:51:01 PST
Created attachment 264941 [details]
WIP
Comment 13 BJ Burg 2015-11-06 10:59:59 PST
(In reply to comment #10)
> Comment on attachment 264853 [details]
> Patch
> 
> I think adding @assert bytecode instrinsics is better than adding @assert
> function.
> It can completely control the bytecodes emitted by @assert(...);
> 
> For example,
> + under the debug build, @assert will emit assertion
> + under the release build, it will emit nothing
> 
> See bytecode/BytecodeIntrinsicRegistry.cpp and @putByValDirect
> implementation for more details.

This seems like a much cleaner approach with less macro-like magic.
Comment 14 youenn fablet 2015-11-09 02:21:21 PST
Created attachment 265035 [details]
build fix
Comment 15 youenn fablet 2015-11-09 02:36:07 PST
Created attachment 265036 [details]
iOS build fix
Comment 16 youenn fablet 2015-11-09 03:14:13 PST
The above patch would create a trace as follow if an assertion was failing:

WebKitBuild/Debug/layout-test-results/streams/readable-stream-reader-read-stderr.txt ASSERTION FAILED: JS assertion failed at line 25 in:
function (stream)
{
    "use strict";

    if (!@isReadableStream(stream))
       throw new @TypeError("ReadableStreamReader needs a ReadableStream");
    if (@isReadableStreamLocked(stream))
       throw new @TypeError("ReadableStream is locked");

    this.@state = stream.@state;
    this.@readRequests = [];
    if (stream.@state === @streamReadable) {
        this.@ownerReadableStream = stream;
        this.@storedError = undefined;
        stream.@reader = this;
        this.@closedPromiseCapability = @newPromiseCapability(@Promise);
        return this;
    }
    if (stream.@state === @streamClosed) {
        this.@ownerReadableStream = null;
        this.@storedError = undefined;
        this.@closedPromiseCapability = { @promise: @Promise.@resolve() };
        return this;
    }
    @assert(stream.@state !== @streamErrored);
    this.@ownerReadableStream = null;
    this.@storedError = stream.@storedError;
    this.@closedPromiseCapability = { @promise: @Promise.@reject(stream.@storedError) };

    return this;
}

OP(1).jsValue().asBoolean()
Comment 17 youenn fablet 2015-11-09 03:17:54 PST
Comment on attachment 265036 [details]
iOS build fix

Marking patch as r?

I am wondering whether there is a way to test that feature in WebKit. If not, is it worth beefing-up WK test infrastructure?

Also, I am not sure all added code should be under #ifdef NDEBUG?
In the current patch, this is limited to only the bytecode generation.
Comment 18 Yusuke Suzuki 2015-11-09 03:38:46 PST
Comment on attachment 265036 [details]
iOS build fix

r=me :)
Comment 19 WebKit Commit Bot 2015-11-09 06:15:20 PST
Comment on attachment 265036 [details]
iOS build fix

Clearing flags on attachment: 265036

Committed r192155: <http://trac.webkit.org/changeset/192155>
Comment 20 WebKit Commit Bot 2015-11-09 06:15:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Yusuke Suzuki 2015-11-09 06:38:17 PST
(In reply to comment #17)
> Comment on attachment 265036 [details]
> iOS build fix
> 
> Marking patch as r?
> 
> I am wondering whether there is a way to test that feature in WebKit. If
> not, is it worth beefing-up WK test infrastructure?

I think this is OK.

> 
> Also, I am not sure all added code should be under #ifdef NDEBUG?
> In the current patch, this is limited to only the bytecode generation.

Making ifdefed code small is better I think (as long as it does not incur performance regression). Keeping the differences small between Debug / Release reduces accidental build breaks in debug builds.