Bug 173226 – RegExp.prototype[@@search] should use SameValue

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Alexey Shvayka]

Created attachment 396650 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 396650 [details]
Patch

Please add DFG and FTL implementations.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #2)
> Please add DFG and FTL implementations.

I am probably missing something, but don't we already have those?

  SpeculativeJIT::compileSameValue() (at dfg/DFGSpeculativeJIT.cpp:6673)
  FTLLowerDFGToB3::compileSameValue() (at ftl/FTLLowerDFGToB3.cpp:8831)

[Yusuke Suzuki]

(In reply to Alexey Shvayka from comment #3)
> (In reply to Yusuke Suzuki from comment #2)
> > Please add DFG and FTL implementations.
> 
> I am probably missing something, but don't we already have those?
> 
>   SpeculativeJIT::compileSameValue() (at dfg/DFGSpeculativeJIT.cpp:6673)
>   FTLLowerDFGToB3::compileSameValue() (at ftl/FTLLowerDFGToB3.cpp:8831)

No, this is implementation of SameValue DFG node. But DFGByteCodeParser is not handling op_same_value to generate this node.
I think we can see crash if we compile the code including op_same_value with DFG. Can you add a test for this?

[Alexey Shvayka]

Created attachment 396708 [details]
Patch

Add test, handle op_same_value in DFGByteCodeParser, add missing baseline JIT ops definitions.

[Yusuke Suzuki]

Comment on attachment 396708 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        Int32 and String comparisons, which are exempt of handling NaN and negative zero values.

I think we should implement @sameValue as a function call instead of bytecode.
Let's define sameValue function like this,

1. Define link-time-constant @sameValue: See m_arrayProtoValuesFunction case for example. Define `m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::sameValue)].initLater(...)`.
2. Initialize this with Object.is definition: Performing putWithoutTransition with LinkTimeConstant::sameValue's value.
3. Put this function as `Object.is` in ObjectConstructor
4. Call this @sameValue from builtins.

So, then, DFG/FTL handles it as SameValue DFG node if this function has SameValue intrinsic appropriately.

[Alexey Shvayka]

Created attachment 396821 [details]
Patch

Expose Object.is function as @sameValue.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #6)
> I think we should implement @sameValue as a function call instead of
> bytecode.
> Let's define sameValue function like this,

I am not sure I've strictly followed your instructions.
I didn't manage to find any precedent of a method put to a prototype/constructor from LinkTimeConstant,
yet I've seen quite a few prototype/constructor methods that were put to LinkTimeConstant (like `thisTimeValue`).
Also it would be nice to keep everything in `objectConstructorTable` in case we would want to reorder methods per spec.

[Yusuke Suzuki]

Comment on attachment 396821 [details]
Patch

r=me. I don't think putting `is` in ObjectConstructor table is important compared to allocating one more object since it requires memory, but maybe both does not matter much.

[EWS]

Committed r260312: <https://trac.webkit.org/changeset/260312>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396821 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61973276>

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 396821 [details]
> Patch
> 
> r=me. I don't think putting `is` in ObjectConstructor table is important
> compared to allocating one more object since it requires memory, but maybe
> both does not matter much.

Apart from Object.is, there are a few link-time-constants that duplicate built-in methods:
* Date.prototype.{valueOf,getTime} == LinkTimeConstant::thisTimeValue
* Math.trunc == LinkTimeConstant::trunc
* String.prototype.repeat == LinkTimeConstant::repeatCharacter

I've tried adding

```
putDirectWithoutTransition(vm, Identifier::fromString(vm, "is"), globalObject->linkTimeConstant(LinkTimeConstant::sameValue), static_cast<unsigned>(PropertyAttribute::DontEnum));
```

to ObjectConstructor::finishCreation(), but it looks it runs before link-time-constant is initialized:

.../WebKit/Source/JavaScriptCore/runtime/JSGlobalObject.h(917) : JSC::JSCell *JSC::JSGlobalObject::linkTimeConstant(JSC::LinkTimeConstant) const
1   0x10acff479 WTFCrash
2   0x10b4c565b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x10b78d2ff JSC::JSGlobalObject::linkTimeConstant(JSC::LinkTimeConstant) const
4   0x10c6f08cb JSC::ObjectConstructor::finishCreation(JSC::VM&, JSC::JSGlobalObject*, JSC::ObjectPrototype*)
5   0x10c5ac80f JSC::ObjectConstructor::create(JSC::VM&, JSC::JSGlobalObject*, JSC::Structure*, JSC::ObjectPrototype*)
6   0x10c5a29ba JSC::JSGlobalObject::init(JSC::VM&)
7   0x10c5b8804 JSC::JSGlobalObject::finishCreation(JSC::VM&)
8   0x107616a01 GlobalObject::finishCreation(JSC::VM&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
9   0x1076154e7 GlobalObject::create(JSC::VM&, JSC::Structure*, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
10  0x1075f1241 int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, bool, jscmain(int, char**)::$_6 const&)
11  0x1075efc58 jscmain(int, char**)
12  0x1075ef9be main
13  0x7fff70ef7cc9 start
14  0x2

No luck with Date.prototype and String.prototype either. Math.trunc works though.