Bug 202475 – [JSC] Implement RegExp Match Indices proposal

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

[Ron Buckton]

Proposal Repository: https://github.com/tc39/proposal-regexp-match-indices

This proposal is currently at Stage 3 and is seeking implementations.

[Radar WebKit Bug Importer]

<rdar://problem/69797861>

[Michael Saboff]

Created attachment 410711 [details]
Work in progress patch

[Michael Saboff]

Comment on attachment 410711 [details]
Work in progress patch

This implements a straightforward implementation.  Uploaded here for EWS testing.

[Michael Saboff]

Created attachment 413897 [details]
Patch

[Michael Saboff]

Created attachment 414055 [details]
Updated Patch addressing test failures

[Yusuke Suzuki]

Comment on attachment 414055 [details]
Updated Patch addressing test failures

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

> JSTests/stress/dfg-strength-reduction-for-regexp-should-indices-property.js:12
> +    // shouldBe(m.indices[0][1], 1);

Is this commented out intentionally?

> LayoutTests/ChangeLog:11
> +        * js/regexp-named-capture-groups-expected.txt:
> +        * js/script-tests/regexp-named-capture-groups.js:

Can we add these tests in JSTests/stress, and skip them for 32bit architectures? We will see failures in watchOS ARMv7k.

[Michael Saboff]

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 414055 [details]
> Updated Patch addressing test failures
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414055&action=review
> 
> > JSTests/stress/dfg-strength-reduction-for-regexp-should-indices-property.js:12
> > +    // shouldBe(m.indices[0][1], 1);
> 
> Is this commented out intentionally?

The leading comment should be removed.  It was for patch testing.

> > LayoutTests/ChangeLog:11
> > +        * js/regexp-named-capture-groups-expected.txt:
> > +        * js/script-tests/regexp-named-capture-groups.js:
> 
> Can we add these tests in JSTests/stress, and skip them for 32bit
> architectures? We will see failures in watchOS ARMv7k.

Yeah, I can do that.

[Yusuke Suzuki]

Comment on attachment 414055 [details]
Updated Patch addressing test failures

Removing r? for now since the spec will be changed.

[Yusuke Suzuki]

https://github.com/tc39/proposal-regexp-match-indices/pull/49 Now, "d" flag is merged, so we need to update to integrate "d" flag.

[Michael Saboff]

Created attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

[Yusuke Suzuki]

Comment on attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

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

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:106
> +            array->putDirect(vm, vm.propertyNames->indices, indicesArray);

I think this is causing GC and failure in mac-debug-wk1 bot.

[Yusuke Suzuki]

Comment on attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

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

>> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:106
>> +            array->putDirect(vm, vm.propertyNames->indices, indicesArray);
> 
> I think this is causing GC and failure in mac-debug-wk1 bot.

We need to set this property at the end of createRegExpMatchesArray (after ensuring GC is ok).

[Yusuke Suzuki]

Since RegExpMatchesArray's structure is different based on 'd' flag. This means that we also need to change DFG AbstractInterpreter's code.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #13)
> Since RegExpMatchesArray's structure is different based on 'd' flag. This
> means that we also need to change DFG AbstractInterpreter's code.

  2558     case RegExpExec:
  2559     case RegExpExecNonGlobalOrSticky:
  2560         if (node->op() == RegExpExec) {
  2561             // Even if we've proven known input types as RegExpObject and String,
  2562             // accessing lastIndex is effectful if it's a global regexp.
  2563             clobberWorld();
  2564         }
  2565
  2566         if (JSValue globalObjectValue = forNode(node->child1()).m_value) {
  2567             if (JSGlobalObject* globalObject = jsDynamicCast<JSGlobalObject*>(m_vm, globalObjectValue)) {
  2568                 if (!globalObject->isHavingABadTime()) {
  2569                     m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
  2570                     RegisteredStructureSet structureSet;
  2571                     structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesArrayStructure()));
  2572                     setForNode(node, structureSet);
  2573                     forNode(node).merge(SpecOther);
  2574                     break;
  2575                 }
  2576             }
  2577         }
  2578         setTypeForNode(node, SpecOther | SpecArray);
  2579         break;

Whether we can use regExpMatchesArrayStructure() depends on "d" flag of RegExp.

[Yusuke Suzuki]

Comment on attachment 418302 [details]
Patch using 'd' flag conforming to latest proposal

Clearing the flag for now since we have several things which need to be solved.

[Michael Saboff]

Created attachment 420713 [details]
Updated patch with 'd' flag and recommended fixes

[Yusuke Suzuki]

Comment on attachment 420713 [details]
Updated patch with 'd' flag and recommended fixes

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2628
> +                    // If regExp is unknown, we need to put both regExp MatchesArray structure variants in our set.
> +                    if (!regExp || !regExp->hasIndices())
> +                        structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesArrayStructure()));
> +                    if (!regExp || regExp->hasIndices())
> +                        structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesIndicesArrayStructure()));

`regExpMatchesIndicesArrayStructure` is structure for array of indices property. This is not the result array's structure.
We need to add `regExpMatchesArrayWithIndicesStructure`, and need to pre-bake the structure which includes indices property, and use this structure when creating the result array.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:35
>  static const PropertyOffset RegExpMatchesArrayGroupsPropertyOffset = 102;

We should add PropertyOffset for RegExpMatchesArrayIndicesPropertyOffset. And we should pre-bake structure for that RegExpMatchesArray.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:89
>      Structure* matchStructure = globalObject->regExpMatchesArrayStructure();

matchStructure should be different if indices property exist.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:221
> +    if (createIndices)
> +        array->putDirect(vm, vm.propertyNames->indices, indicesArray);

We should set this inside setProperties by using RegExpMatchesArrayIndicesPropertyOffset.

[Michael Saboff]

Created attachment 420842 [details]
Updated patch addressing review comments

[Yusuke Suzuki]

Comment on attachment 420842 [details]
Updated patch addressing review comments

r=me

[Michael Saboff]

Committed r273086 (234284@main): <https://commits.webkit.org/234284@main>