[regexp] Properly handle HeapNumbers in AdvanceStringIndex

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 1510 matching lines @@
 
     // Return true iff exec matched successfully.
     Node* const result =
         SelectBooleanConstant(WordNotEqual(match_indices, NullConstant()));
     Return(result);
   }
 }
 
 Node* RegExpBuiltinsAssembler::AdvanceStringIndex(Node* const string,
                                                   Node* const index,
-                                                  Node* const is_unicode) {
+                                                  Node* const is_unicode,
+                                                  bool is_fastpath) {
+  CSA_ASSERT(this, IsHeapNumberMap(LoadReceiverMap(index)));

[Camillo Bruni, 2017/04/06 15:10:57]

nit: add IsNumber()

[jgruber, 2017/04/06 16:28:33]

Acknowledged - pushing to a follow-up to keep this CL small for backmerges.

+  if (is_fastpath) CSA_ASSERT(this, TaggedIsSmi(index));

[Camillo Bruni, 2017/04/06 15:10:57]

This should probably be TaggedIsPositiveSmi(index)

[jgruber, 2017/04/06 16:28:33]

Done.

+  Variable var_index(this, MachineRepresentation::kTagged, index);
+  if (!is_fastpath) {
+    // Slow Smi normalization. Binds var_index to a Smi representation if
+    // {index} is in Smi range, and to a HeapNumber otherwise.
+    Label next(this);
+    GotoIf(TaggedIsSmi(index), &next);
+
+    CSA_ASSERT(this, IsHeapNumber(index));
+    Node* const value = LoadHeapNumberValue(index);
+    var_index.Bind(ChangeFloat64ToTagged(value));
+    Goto(&next);

[Camillo Bruni, 2017/04/06 15:10:58]

as discussed offline: probably not worth the effort here. Smi-range heap numbers
should always be normalized, feel free to keep this.

[jgruber, 2017/04/06 16:28:33]

Done. I'll add a corresponding assert in a follow-up.

+
+    Bind(&next);
+  }
+
   // Default to last_index + 1.
-  Node* const index_plus_one = SmiAdd(index, SmiConstant(1));
+  Node* const index_plus_one = NumberInc(var_index.value());
   Variable var_result(this, MachineRepresentation::kTagged, index_plus_one);
 
+  // Advancing the index has some subtle issues involving the distinction
+  // between Smis and HeapNumbers. There's three cases:
+  // * {index} is a Smi, {index_plus_one} is a Smi. The standard case.
+  // * {index} is a Smi, {index_plus_one} overflows into a HeapNumber.
+  //   In this case we can return the result early, because
+  //   {index_plus_one} > {string}.length.
+  // * {index} is a HeapNumber, {index_plus_one} is a HeapNumber. This can only
+  //   occur when {index} is outside the Smi range since we normalize
+  //   explicitly. Again we can return early.
+  if (is_fastpath) {
+    // Must be in Smi range on the fast path. We control the value of {index}
+    // on all call-sites and can never exceed the length of the string.
+    STATIC_ASSERT(String::kMaxLength + 2 < Smi::kMaxValue);
+    CSA_ASSERT(this, TaggedIsSmi(index_plus_one));

[Camillo Bruni, 2017/04/06 15:10:57]

TaggedIsPositiveSmi(index_plus_one)

[jgruber, 2017/04/06 16:28:34]

Done.

+  }
+
   Label if_isunicode(this), out(this);
-  Branch(is_unicode, &if_isunicode, &out);
+  GotoIfNot(is_unicode, &out);
+
+  // Keep this unconditional (even on the fast path) just to be safe.
+  Branch(TaggedIsSmi(index_plus_one), &if_isunicode, &out);

[jgruber, 2017/04/06 16:28:33]

Also changed this to TaggedIsPositiveSmi.

 
   Bind(&if_isunicode);
   {
     Node* const string_length = LoadStringLength(string);
     GotoIfNot(SmiLessThan(index_plus_one, string_length), &out);
 
-    Node* const lead = StringCharCodeAt(string, index);
+    Node* const lead = StringCharCodeAt(string, var_index.value());

[Camillo Bruni, 2017/04/06 15:10:57]

Please add CSA_ASSERT(this, TaggedIsPositiveSmi(index)) in StringCharCodeAt

[jgruber, 2017/04/06 16:28:33]

Will do in a follow-up.

     GotoIfNot(Word32Equal(Word32And(lead, Int32Constant(0xFC00)),
                           Int32Constant(0xD800)),
               &out);
 
     Node* const trail = StringCharCodeAt(string, index_plus_one);
     GotoIfNot(Word32Equal(Word32And(trail, Int32Constant(0xFC00)),
                           Int32Constant(0xDC00)),
               &out);
 
     // At a surrogate pair, return index + 2.
-    Node* const index_plus_two = SmiAdd(index, SmiConstant(2));
+    Node* const index_plus_two = NumberInc(index_plus_one);
     var_result.Bind(index_plus_two);
 
     Goto(&out);
   }
 
   Bind(&out);
   return var_result.value();
 }
 
 namespace {
@@ skipping 265 matching lines @@
         GotoIfNot(SmiEqual(match_length, smi_zero), &loop);
 
         Node* last_index = LoadLastIndex(context, regexp, is_fastpath);
         if (is_fastpath) {
           CSA_ASSERT(this, TaggedIsPositiveSmi(last_index));
         } else {
           last_index = CallBuiltin(Builtins::kToLength, context, last_index);
         }
 
         Node* const new_last_index =
-            AdvanceStringIndex(string, last_index, is_unicode);
+            AdvanceStringIndex(string, last_index, is_unicode, is_fastpath);
 
         if (is_fastpath) {
           // On the fast path, we can be certain that lastIndex can never be
           // incremented to overflow the Smi range since the maximal string
           // length is less than the maximal Smi value.
           STATIC_ASSERT(String::kMaxLength < Smi::kMaxValue);
           CSA_ASSERT(this, TaggedIsPositiveSmi(new_last_index));
         }
 
         StoreLastIndex(context, regexp, new_last_index, is_fastpath);
@@ skipping 284 matching lines @@
 
     // Advance index and continue if the match is empty.
     {
       Label next(this);
 
       GotoIfNot(SmiEqual(match_to, next_search_from), &next);
       GotoIfNot(SmiEqual(match_to, last_matched_until), &next);
 
       Node* const is_unicode = FastFlagGetter(regexp, JSRegExp::kUnicode);
       Node* const new_next_search_from =
-          AdvanceStringIndex(string, next_search_from, is_unicode);
+          AdvanceStringIndex(string, next_search_from, is_unicode, true);
       var_next_search_from.Bind(new_next_search_from);
       Goto(&loop);
 
       Bind(&next);
     }
 
     // A valid match was found, add the new substring to the array.
     {
       Node* const from = last_matched_until;
       Node* const to = match_from;
@@ skipping 610 matching lines @@
   Bind(&if_matched);
   {
     Node* result =
         ConstructNewResultFromMatchInfo(context, regexp, match_indices, string);
     Return(result);
   }
 }
 
 }  // namespace internal
 }  // namespace v8