Bug 178726 – [JSC] Introduce @toObject

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Introduce @toObject

[Yusuke Suzuki]

Created attachment 324672 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 324690 [details]
Patch

[Yusuke Suzuki]

Ping?

[Saam Barati]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r224280: <https://trac.webkit.org/changeset/224280>

[Joseph Pecoraro]

Nice!

[Yusuke Suzuki]

(In reply to Joseph Pecoraro from comment #9)
> Nice!

:D

[Yusuke Suzuki]

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.

[Ryan Haddad]

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

[Yusuke Suzuki]

Committed r224335: <https://trac.webkit.org/changeset/224335>

[Radar WebKit Bug Importer]

<rdar://problem/35568496>