Bug 153737 – [JSC] Introduce BytecodeIntrinsic constant rep like @undefined

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

[Yusuke Suzuki]

[JSC] Introduce BytecodeIntrinsic constant rep like @undefined

[Yusuke Suzuki]

Created attachment 270379 [details]
Patch

[Yusuke Suzuki]

The other plan (additional) is, converting GetGlobalVar in DFG to JSConstant if the property is NonWritable and NonConfigurable.
What do you think of?

[Darin Adler]

Comment on attachment 270379 [details]
Patch

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

What prevents us from accidentally using "undefined" instead of "@undefined"? Seems like it might be easy to make this kind of error.

> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:37
> -#define INITIALISE_BYTECODE_INTRINSIC_NAMES_TO_SET(name) m_bytecodeIntrinsicMap.add(propertyNames.name##PrivateName.impl(), &BytecodeIntrinsicNode::emit_intrinsic_##name);
> +#define INITIALISE_BYTECODE_INTRINSIC_NAMES_TO_SET(name) m_bytecodeIntrinsicMap.add(vm.propertyNames->name##PrivateName.impl(), &BytecodeIntrinsicNode::emit_intrinsic_##name);

INITIALISE is the British spelling. We should move to INITIALIZE in the future.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:860
> +            return 0; \

nullptr

[Saam Barati]

(In reply to comment #3)
> Comment on attachment 270379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270379&action=review
> 
> What prevents us from accidentally using "undefined" instead of
> "@undefined"? Seems like it might be easy to make this kind of error.

I agree that this would be good to have. Maybe we can do some
post-parsing validation only in debug builds. I think we already
do some post-parsing validation for builtins, but I forget for what.

[Geoffrey Garen]

> > What prevents us from accidentally using "undefined" instead of
> > "@undefined"? Seems like it might be easy to make this kind of error.
> 
> I agree that this would be good to have. Maybe we can do some
> post-parsing validation only in debug builds. I think we already
> do some post-parsing validation for builtins, but I forget for what.

I propose: Once we have constants available for all "@" constructs -- and perhaps we do already -- we should treat any use of a non-locally-resolved variable in a builtin as a compile-time error.

[Yusuke Suzuki]

Comment on attachment 270379 [details]
Patch

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

Thanks for your review.
We have some post parsing pass in BuiltinExecutable.cpp's createExecutableInternal. So, we should check this here.
I'll create it in another patch :)

>> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:37
>> +#define INITIALISE_BYTECODE_INTRINSIC_NAMES_TO_SET(name) m_bytecodeIntrinsicMap.add(vm.propertyNames->name##PrivateName.impl(), &BytecodeIntrinsicNode::emit_intrinsic_##name);
> 
> INITIALISE is the British spelling. We should move to INITIALIZE in the future.

ok. here, I'll rename this to INITIALIZE.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:860
>> +            return 0; \
> 
> nullptr

Thanks! Fixed.

[Yusuke Suzuki]

(In reply to comment #5)
> > > What prevents us from accidentally using "undefined" instead of
> > > "@undefined"? Seems like it might be easy to make this kind of error.
> > 
> > I agree that this would be good to have. Maybe we can do some
> > post-parsing validation only in debug builds. I think we already
> > do some post-parsing validation for builtins, but I forget for what.
> 
> I propose: Once we have constants available for all "@" constructs -- and
> perhaps we do already -- we should treat any use of a non-locally-resolved
> variable in a builtin as a compile-time error.

Nice. To do so, I think we need some interesting phase in CodeBlock.
Since UnlinkedCodeBlock is not linked to JSGlobalObject, we cannot use any constant values that belongs to the realm.
Currently, in this patch, all the constants are primitive values.

One idea is that,

1. In the parsing phase, when we see @xxx (like, `@Object`), we look up it from the bytecode intrinsic registry. And if there is none, we should assert it!
2. And filling constant buffer with placeholder value.
3. Later, in CodeBlock phase, we will fill it with the actual constant value linked with the specific JSGlobalObject.

And luckly, we already have some mechanizm for that., called link time constant (see emitMoveLinkTimeConstant, that is used to fill `defineProperty` method in constant register).
Extending this mechanizm to use it genericly and easily is nice. And as the result, we can purge all the hacky JSGlobalObject's private properties.

[Yusuke Suzuki]

Opened the issue for that. https://bugs.webkit.org/show_bug.cgi?id=153792

[Yusuke Suzuki]

Committed r196022: <http://trac.webkit.org/changeset/196022>