Bug 150333 – JS Built-ins functions should be able to assert

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[youenn fablet]

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

[youenn fablet]

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").

[youenn fablet]

Created attachment 264853 [details]
Patch

[WebKit Commit Bot]

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.

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

(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.

[Geoffrey Garen]

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?

[Alex Christensen]

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.

[Blaze Burg]

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.

[Blaze Burg]

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.

[Yusuke Suzuki]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 264941 [details]
WIP

[Blaze Burg]

(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.

[youenn fablet]

Created attachment 265035 [details]
build fix

[youenn fablet]

Created attachment 265036 [details]
iOS build fix

[youenn fablet]

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()

[youenn fablet]

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.

[Yusuke Suzuki]

Comment on attachment 265036 [details]
iOS build fix

r=me :)

[WebKit Commit Bot]

Comment on attachment 265036 [details]
iOS build fix

Clearing flags on attachment: 265036

Committed r192155: <http://trac.webkit.org/changeset/192155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

(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.