[regexp] Fix two more possible shape changes on fast path

src/builtins/builtins-regexp-gen.cc

 // Copyright 2017 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/builtins/builtins-regexp-gen.h"
 
 #include "src/builtins/builtins-constructor-gen.h"
 #include "src/builtins/builtins-utils-gen.h"
 #include "src/builtins/builtins.h"
 #include "src/code-factory.h"
@@ skipping 2285 matching lines @@
 
   CSA_ASSERT(this, IsFastRegExp(context, regexp));
   CSA_ASSERT(this, IsString(string));
 
   // TODO(jgruber): Even if map checks send us to the fast path, we still need
   // to verify the constructor property and jump to the slow path if it has
   // been changed.
 
   // Convert {maybe_limit} to a uint32, capping at the maximal smi value.
   Variable var_limit(this, MachineRepresentation::kTagged);
-  Label if_limitissmimax(this), limit_done(this);
+  Label if_limitissmimax(this), limit_done(this), runtime(this);
 
   GotoIf(IsUndefined(maybe_limit), &if_limitissmimax);
 
+  Node* const limit = ToUint32(context, maybe_limit);

[Camillo Bruni, 2017/04/06 14:54:15]

future-nit: something like ToUint32(..., &was_fast) would avoid the subsequent
more expensive BranchIfFastRegExp checks. Basically you can skip if you get a
Number as input.

[jgruber, 2017/04/06 15:21:12]

Sounds reasonable. I want to keep this CL small and noninvasive though since it
will need to be backmerged. I added a fast path for non-negative smis above.

   {
-    Node* const limit = ToUint32(context, maybe_limit);
+    // ToUint32(limit) could potentially change the shape of the RegExp
+    // object. Recheck that we are still on the fast path and bail to runtime
+    // otherwise.
+    {
+      Label next(this);
+      BranchIfFastRegExp(context, regexp, LoadMap(regexp), &next, &runtime);

[Camillo Bruni, 2017/04/06 14:54:15]

site-note: You'd probably be better off with a GotoIfSlowRegexp(..., &runtime)
since the fast-path code is mostly inline.

[jgruber, 2017/04/06 15:21:12]

Acknowledged.

+      Bind(&next);
+    }
+
     GotoIfNot(TaggedIsSmi(limit), &if_limitissmimax);
 
     var_limit.Bind(limit);
     Goto(&limit_done);
   }
 
   Bind(&if_limitissmimax);
   {
     // TODO(jgruber): In this case, we can probably avoid generation of limit
     // checks in Generate_RegExpPrototypeSplitBody.
     var_limit.Bind(SmiConstant(Smi::kMaxValue));
     Goto(&limit_done);
   }
 
   Bind(&limit_done);
   {
     Node* const limit = var_limit.value();
     RegExpPrototypeSplitBody(context, regexp, string, limit);
   }
+
+  Bind(&runtime);
+  {
+    // The runtime call passes in limit to ensure the second ToUint32(limit)
+    // call is not observable.
+    CSA_ASSERT(this, IsHeapNumberMap(LoadReceiverMap(limit)));

[Camillo Bruni, 2017/04/06 14:54:15]

nit: CSA_ASSERT(this, IsNumber(limit));

[jgruber, 2017/04/06 15:21:12]

Can do in a follow-up (don't want to touch c-s-a.{h,cc} in this one).

+    Return(CallRuntime(Runtime::kRegExpSplit, context, regexp, string, limit));
+  }
 }
 
 // ES#sec-regexp.prototype-@@split
 // RegExp.prototype [ @@split ] ( string, limit )
 TF_BUILTIN(RegExpPrototypeSplit, RegExpBuiltinsAssembler) {
   Node* const maybe_receiver = Parameter(Descriptor::kReceiver);
   Node* const maybe_string = Parameter(Descriptor::kString);
   Node* const maybe_limit = Parameter(Descriptor::kLimit);
   Node* const context = Parameter(Descriptor::kContext);
 
@@ skipping 351 matching lines @@
   GotoIf(TaggedIsSmi(replace_value), &checkreplacestring);
   Branch(IsCallableMap(LoadMap(replace_value)), &if_iscallable,
          &checkreplacestring);
 
   // 3. Does ToString({replace_value}) contain '$'?
   Bind(&checkreplacestring);
   {
     Node* const replace_string =
         CallBuiltin(Builtins::kToString, context, replace_value);
 
+    // ToString(replaceValue) could potentially change the shape of the RegExp
+    // object. Recheck that we are still on the fast path and bail to runtime
+    // otherwise.
+    {
+      Label next(this);
+      BranchIfFastRegExp(context, regexp, LoadMap(regexp), &next, &runtime);
+      Bind(&next);
+    }
+
     Node* const dollar_string = HeapConstant(
         isolate()->factory()->LookupSingleCharacterStringFromCode('$'));
     Node* const dollar_ix =
         CallBuiltin(Builtins::kStringIndexOf, context, replace_string,
                     dollar_string, SmiConstant(0));
     GotoIfNot(SmiEqual(dollar_ix, SmiConstant(-1)), &runtime);
 
     Return(
         ReplaceSimpleStringFastPath(context, regexp, string, replace_string));
   }
@@ skipping 99 matching lines @@
   Bind(&if_matched);
   {
     Node* result =
         ConstructNewResultFromMatchInfo(context, regexp, match_indices, string);
     Return(result);
   }
 }
 
 }  // namespace internal
 }  // namespace v8