Bug 178726 - [JSC] Introduce @toObject
Summary: [JSC] Introduce @toObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 178737
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-24 07:29 PDT by Yusuke Suzuki
Modified: 2017-11-15 12:57 PST (History)
9 users (show)

See Also:


Attachments
Patch (67.20 KB, patch)
2017-10-24 07:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (82.11 KB, patch)
2017-10-24 11:40 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-10-24 07:29:44 PDT
[JSC] Introduce @toObject
Comment 1 Yusuke Suzuki 2017-10-24 07:47:15 PDT
Created attachment 324672 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2017-10-24 11:40:22 PDT
Created attachment 324690 [details]
Patch
Comment 3 Yusuke Suzuki 2017-10-26 20:52:21 PDT
Ping?
Comment 4 Saam Barati 2017-10-27 11:29:40 PDT
Comment on attachment 324690 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/ChangeLog:9
> +        Previously we emulated @toObject behavior in builtin JS. But it consumes much bytecode size while @toObject

consumes much => consumes a lot

> Source/JavaScriptCore/builtins/ArrayConstructor.js:55
> +    var arrayLike = @toObject(items, "Array.from requires an array-like object - not null or undefined");

why is this OK? `items` us used in the code below so this seems like it changed behavior

> Source/JavaScriptCore/builtins/DatePrototype.js:43
> +            options = @toObject(opts, "");

Why switch this? We know opts is not null/undefined here

> Source/JavaScriptCore/builtins/DatePrototype.js:100
> +            options = @toObject(opts, "");

Why switch this? We know opts is not null/undefined here

> Source/JavaScriptCore/builtins/DatePrototype.js:150
> +            options = @toObject(opts, "");

Why switch this? We know opts is not null/undefined here

> Source/JavaScriptCore/builtins/GlobalOperations.js:93
> +    let from = @toObject(source, "");

Why switch this given there is already null/undefined check above.

> Source/JavaScriptCore/builtins/GlobalOperations.js:118
> +    let from = @toObject(source, "");

ditto

> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:62
> +    let arrayLike = @toObject(items, "TypedArray.from requires an array-like object - not null or undefined");

This seems like a semantic change given items is used below.

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:342
> +        var speciesConstructor = @toObject(constructor, "").@speciesSymbol;

This seems like a semantic change given constructor might be null. Why change this?

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:383
> +        var speciesConstructor = @toObject(constructor, "").@speciesSymbol;

ditto

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1044
> +    return generator.moveToDestinationIfNeeded(dst, generator.emitToObject(generator.tempDestination(dst), src.get(), message));

Don't you want to capture tempDestination result in RefPtr?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2197
> +        if (node->op() == ToObject)
> +            clobberWorld(node->origin.semantic, clobberLimit);

Why clobberWorld?

> Source/JavaScriptCore/dfg/DFGClobberize.h:527
> +        write(Heap);

Is this really true?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:292
> +        if (errorMessage->length()) {
> +            throwVMTypeError(exec, scope, errorMessage);
> +            return nullptr;
> +        }

I'm really not a fan of this API. The responsibility of this node should be to always thrown an error message in the case of undefined/null. Builtins that don't want this shouldn't use this API.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-4022
> -    RELEASE_ASSERT(node->child1().useKind() == UntypedUse);

Why remove this? It seems like the code below is wrong if this isn't true since you don't perform type checks

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:445
> +        if (!ident.isEmpty())
> +            THROW(createTypeError(exec, ident.impl()));

Same comment. I'm really not a fan of these semantics. We should use the old @Object if this is what we want.
Comment 5 Yusuke Suzuki 2017-11-01 07:02:44 PDT
Comment on attachment 324690 [details]
Patch

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

>> Source/JavaScriptCore/builtins/ArrayConstructor.js:55
>> +    var arrayLike = @toObject(items, "Array.from requires an array-like object - not null or undefined");
> 
> why is this OK? `items` us used in the code below so this seems like it changed behavior

If `items` is null or undefined, @toObject / the previous code throw an error.
And if it's not, we perform ToObject abstract operation on `items` when accessing @iteratorSymbol property.
So, the semantics is not changed.

>> Source/JavaScriptCore/builtins/DatePrototype.js:43
>> +            options = @toObject(opts, "");
> 
> Why switch this? We know opts is not null/undefined here

Just because @toObject is more verbose, and it is strictly aligned to the spec.

>> Source/JavaScriptCore/builtins/DatePrototype.js:100
>> +            options = @toObject(opts, "");
> 
> Why switch this? We know opts is not null/undefined here

Just because @toObject is more verbose, and it is strictly aligned to the spec.

>> Source/JavaScriptCore/builtins/DatePrototype.js:150
>> +            options = @toObject(opts, "");
> 
> Why switch this? We know opts is not null/undefined here

Just because @toObject is more verbose, and it is strictly aligned to the spec.

>> Source/JavaScriptCore/builtins/GlobalOperations.js:93
>> +    let from = @toObject(source, "");
> 
> Why switch this given there is already null/undefined check above.

Just because @toObject is more verbose, and it is strictly aligned to the spec.

>> Source/JavaScriptCore/builtins/GlobalOperations.js:118
>> +    let from = @toObject(source, "");
> 
> ditto

Ditto. The spec uses @toObject here.

>> Source/JavaScriptCore/builtins/TypedArrayConstructor.js:62
>> +    let arrayLike = @toObject(items, "TypedArray.from requires an array-like object - not null or undefined");
> 
> This seems like a semantic change given items is used below.

As is explained in Array.from, the semantics is not changed here.

>> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:342
>> +        var speciesConstructor = @toObject(constructor, "").@speciesSymbol;
> 
> This seems like a semantic change given constructor might be null. Why change this?

In the spec, we throw an error if @isObject() condition is not met. So rather this fixes the existing bug.
https://tc39.github.io/ecma262/#sec-speciesconstructor
For here, we can just do `constructor.@speciesSymbol`. Fixed.

>> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:383
>> +        var speciesConstructor = @toObject(constructor, "").@speciesSymbol;
> 
> ditto

Ditto

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2197
>> +            clobberWorld(node->origin.semantic, clobberLimit);
> 
> Why clobberWorld?

It may throw an error.

>> Source/JavaScriptCore/dfg/DFGClobberize.h:527
>> +        write(Heap);
> 
> Is this really true?

Yes, it can throw an error. If we prove that the given ToObject does not throw any errors (with edge filters etc.), we lower it to CallObjectConstructor.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:292
>> +        }
> 
> I'm really not a fan of this API. The responsibility of this node should be to always thrown an error message in the case of undefined/null. Builtins that don't want this shouldn't use this API.

It's just a workaround for verbose messages. If "not an object" error is ok, we can just call it.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-4022
>> -    RELEASE_ASSERT(node->child1().useKind() == UntypedUse);
> 
> Why remove this? It seems like the code below is wrong if this isn't true since you don't perform type checks

Reverted this part. Originally, my WIP patch has some edges.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:445
>> +            THROW(createTypeError(exec, ident.impl()));
> 
> Same comment. I'm really not a fan of these semantics. We should use the old @Object if this is what we want.

I don't think using @Object is better. It is not aligned to the spec. This is only for verbose error messages in the current butilins.
I'm ok to change the current builtin messages to something "failed to convert null/undefined to an object".
Comment 6 Yusuke Suzuki 2017-11-01 07:03:23 PDT
Comment on attachment 324690 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:445
>>> +            THROW(createTypeError(exec, ident.impl()));
>> 
>> Same comment. I'm really not a fan of these semantics. We should use the old @Object if this is what we want.
> 
> I don't think using @Object is better. It is not aligned to the spec. This is only for verbose error messages in the current butilins.
> I'm ok to change the current builtin messages to something "failed to convert null/undefined to an object".

I'll drop this part, and make the current builtin error messages a bit less verbose.
Comment 7 Yusuke Suzuki 2017-11-01 10:13:42 PDT
Comment on attachment 324690 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1044
>> +    return generator.moveToDestinationIfNeeded(dst, generator.emitToObject(generator.tempDestination(dst), src.get(), message));
> 
> Don't you want to capture tempDestination result in RefPtr?

It's ok as long as we do not allocate a newTemprary(), so this code is OK. But it's safe to do that. Fixed.
Comment 8 Yusuke Suzuki 2017-11-01 10:32:44 PDT
Committed r224280: <https://trac.webkit.org/changeset/224280>
Comment 9 Joseph Pecoraro 2017-11-01 11:13:50 PDT
Nice!
Comment 10 Yusuke Suzuki 2017-11-01 11:25:10 PDT
(In reply to Joseph Pecoraro from comment #9)
> Nice!

:D
Comment 11 Yusuke Suzuki 2017-11-01 11:25:55 PDT
Comment on attachment 324690 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        Previously we emulated @toObject behavior in builtin JS. But it consumes much bytecode size while @toObject
> 
> consumes much => consumes a lot

Oops! I missed this comment.
Comment 12 Ryan Haddad 2017-11-02 08:50:58 PDT
stress/to-object-intrinsic.js is failing on debug bots:

stress/to-object-intrinsic.js.default: ERROR: Unchecked JS exception:
stress/to-object-intrinsic.js.default:     This scope can throw a JS exception: toObjectSlowCase @ ./runtime/JSCJSValue.cpp:97
stress/to-object-intrinsic.js.default:         (ExceptionScope::m_recursionDepth was 5)
stress/to-object-intrinsic.js.default:     But the exception was unchecked as of this scope: operationToObject @ ./dfg/DFGOperations.cpp:283
stress/to-object-intrinsic.js.default:         (ExceptionScope::m_recursionDepth was 4)

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20(Tests)/builds/174/steps/jscore-test/logs/stdio
Comment 13 Yusuke Suzuki 2017-11-02 09:39:03 PDT
Committed r224335: <https://trac.webkit.org/changeset/224335>
Comment 14 Radar WebKit Bug Importer 2017-11-15 12:57:53 PST
<rdar://problem/35568496>