Bug 153737

Summary: [JSC] Introduce BytecodeIntrinsic constant rep like @undefined
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Yusuke Suzuki 2016-01-31 23:37:39 PST
[JSC] Introduce BytecodeIntrinsic constant rep like @undefined
Comment 1 Yusuke Suzuki 2016-02-01 00:38:20 PST
Created attachment 270379 [details]
Patch
Comment 2 Yusuke Suzuki 2016-02-01 03:02:15 PST
The other plan (additional) is, converting GetGlobalVar in DFG to JSConstant if the property is NonWritable and NonConfigurable.
What do you think of?
Comment 3 Darin Adler 2016-02-01 10:13:28 PST
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
Comment 4 Saam Barati 2016-02-01 11:15:35 PST
(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.
Comment 5 Geoffrey Garen 2016-02-01 11:36:11 PST
> > 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.
Comment 6 Yusuke Suzuki 2016-02-02 11:08:16 PST
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.
Comment 7 Yusuke Suzuki 2016-02-02 11:14:29 PST
(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.
Comment 8 Yusuke Suzuki 2016-02-02 11:16:54 PST
Opened the issue for that. https://bugs.webkit.org/show_bug.cgi?id=153792
Comment 9 Yusuke Suzuki 2016-02-02 11:33:15 PST
Committed r196022: <http://trac.webkit.org/changeset/196022>