Bug 209375 – RegExp.prototype.exec must always access lastIndex

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

[Ross Kirsling]

RegExp.prototype.exec must always access lastIndex

[Ross Kirsling]

Created attachment 394161 [details]
Patch

[Ross Kirsling]

Comment on attachment 394161 [details]
Patch

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

A couple of notes:

1. I thought about proposing we change the spec here, but apparently somebody already tried that and it had to be reverted:
   https://github.com/tc39/ecma262/issues/777

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:803
> +            if (!hasIndeterminateLastIndex && convertToStatic())

2. I believe this does the trick, but please educate me if there's a better way.

(Near the top of this case block, we find the RegExpObject when the op is RegExpExec, so my original idea was to hang onto that so that I could check regExpObject->getLastIndex().isUInt32() in convertToStatic, but it seemed like regExpObject was disappearing on me. I suppose that explains the complexity of the existing lastIndex-verifying procedure. As such, I just added a boolean so that we don't do the verification twice -- i.e., we should only convert to RegExpExecNonGlobalOrSticky when lastIndex is determinate but the RegExp as a whole is non-constant.)

[Ross Kirsling]

Created attachment 394162 [details]
Patch

[Ross Kirsling]

Created attachment 394173 [details]
Patch

[Ross Kirsling]

(In reply to Ross Kirsling from comment #2)
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:803
> > +            if (!hasIndeterminateLastIndex && convertToStatic())
> 
> 2. I believe this does the trick, but please educate me if there's a better
> way.

Looks like I can just raise the lastIndex verification step up and out of foldConstant instead.

[Ross Kirsling]

Created attachment 394176 [details]
Patch

[Ross Kirsling]

Created attachment 394180 [details]
Patch

[Caio Lima]

Comment on attachment 394180 [details]
Patch

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

Informal review: Overall LGTM, but I left some comments.

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:567
> +                lastIndex = 0;

Nice catch here. Is there a specific reason to move this out of `foldToConstant`? This will make us always run `executeInsertionSet()` and iterate over instructions increasing compile time, even if we endup giving up on folding due to other reasons.

> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:49
>          RETURN_IF_EXCEPTION(scope, UINT_MAX);

Do we have a test case like the one you added, but it throws an exception instead of counting execution? I would be nice to test such code path as well.

[Ross Kirsling]

(In reply to Caio Lima from comment #8)
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:567
> > +                lastIndex = 0;
> 
> Nice catch here. Is there a specific reason to move this out of
> `foldToConstant`? This will make us always run `executeInsertionSet()` and
> iterate over instructions increasing compile time, even if we endup giving
> up on folding due to other reasons.

It needs block *both* strength reductions that follow. At first I just had foldToConstant set a boolean which would prevent entering convertToStatic, but beyond being weird in style, this wouldn't've been valid in the case where foldToConstant didn't even reach that check (e.g. due to massive string size or number of subpatterns).

My only concern was whether the ordering was too fragile to allow for raising this check above, but this doesn't appear to be the case.

> > Source/JavaScriptCore/runtime/RegExpObjectInlines.h:49
> >          RETURN_IF_EXCEPTION(scope, UINT_MAX);
> 
> Do we have a test case like the one you added, but it throws an exception
> instead of counting execution? I would be nice to test such code path as
> well.

I was going to add one where lastIndex is a Symbol and we catch the error, but I couldn't get it to tier up that way. I guess I can add it regardless though.

[Caio Lima]

Comment on attachment 394180 [details]
Patch

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

>> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:49
>>          RETURN_IF_EXCEPTION(scope, UINT_MAX);
> 
> Do we have a test case like the one you added, but it throws an exception instead of counting execution? I would be nice to test such code path as well.

I think it is fine if we don't tier up, but if you would like to do it, I think this could potentially work:

```
...
let throws = false;
r.lastIndex = {
  valueOf() {
     if(throws) 
        //throws here
     return 0;
   }
 };
...
// set throws to true after we tier up
```

[Ross Kirsling]

(In reply to Caio Lima from comment #10)
> I think it is fine if we don't tier up, but if you would like to do it, I
> think this could potentially work:
> 
> ```
> ...
> let throws = false;
> r.lastIndex = {
>   valueOf() {
>      if(throws) 
>         //throws here
>      return 0;
>    }
>  };
> ...
> // set throws to true after we tier up
> ```

That's a neat suggestion! I think it won't help though, if DFG compiles with the assumption that there won't be a TypeError? But it seems like we can tier up as long as we catch the error *outside* of the function itself, so I suppose I'll do it that way.

[Ross Kirsling]

Created attachment 394430 [details]
Patch

[Ross Kirsling]

Ping?

[Saam Barati]

Comment on attachment 394430 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:15
> +        That is, we're always obliged to verify lastIndex, even when we don't use the value.

what does verify mean?

> Source/JavaScriptCore/ChangeLog:16
> +        DFG, in particular, must make sure strength reductions don't apply when lastIndex isn't an unsigned integer.

why?

> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:71
> +        setLastIndex(globalObject, 0);

this seems like it could throw too, and override previously thrown exception?

> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:116
> +        setLastIndex(globalObject, 0);

ditto

[Ross Kirsling]

Created attachment 394967 [details]
Patch

[Saam Barati]

Comment on attachment 394967 [details]
Patch

r=me

[EWS]

Committed r259246: <https://trac.webkit.org/changeset/259246>

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

[Radar WebKit Bug Importer]

<rdar://problem/61080650>