[regexp] Consistent expectations for output registers

schuay · Commit Bot · commit fe60913945bc · 2020-04-21T10:13:03.000Z
... between the interpreter and generated code. Prior to this CL, pre- and post conditions on the output register array differed between the interpreter and generated code. Interpreter Pre: `output` fits captures and temporary registers. Post: None. Generated code Pre: `output` fits capture registers. Post: `output` is modified if and only if the match succeeded. This CL changes the interpreter to match generated code pre- and post conditions by allocating space for temporary registers inside the interpreter. Drive-by: Add MaxRegisterCount, RegistersForCaptureCount helpers. Bug: chromium:1067270 Change-Id: I2900ef2f31207d817ec7ead3e0e2215b23b398f0 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2135642 Commit-Queue: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Leszek Swirski <leszeks@chromium.org> Cr-Commit-Position: refs/heads/master@{#67268}
diff --git a/src/builtins/builtins-regexp-gen.cc b/src/builtins/builtins-regexp-gen.cc
@@ -528,7 +528,7 @@ TNode<HeapObject> RegExpBuiltinsAssembler::RegExpExecInternal(
         data, JSRegExp::kIrregexpCaptureCountIndex));
     // capture_count is the number of captures without the match itself.
     // Required registers = (capture_count + 1) * 2.
-    STATIC_ASSERT(Internals::IsValidSmi((JSRegExp::kMaxCaptures + 1) << 1));
+    STATIC_ASSERT(Internals::IsValidSmi((JSRegExp::kMaxCaptures + 1) * 2));
     TNode<Smi> register_count =
         SmiShl(SmiAdd(capture_count, SmiConstant(1)), 1);
 
diff --git a/src/objects/js-regexp-inl.h b/src/objects/js-regexp-inl.h
@@ -34,7 +34,7 @@ JSRegExp::Type JSRegExp::TypeTag() const {
   return static_cast<JSRegExp::Type>(smi.value());
 }
 
-int JSRegExp::CaptureCount() {
+int JSRegExp::CaptureCount() const {
   switch (TypeTag()) {
     case ATOM:
       return 0;
@@ -45,6 +45,11 @@ int JSRegExp::CaptureCount() {
   }
 }
 
+int JSRegExp::MaxRegisterCount() const {
+  CHECK_EQ(TypeTag(), IRREGEXP);
+  return Smi::ToInt(DataAt(kIrregexpMaxRegisterCountIndex));
+}
+
 JSRegExp::Flags JSRegExp::GetFlags() {
   DCHECK(this->data().IsFixedArray());
   Object data = this->data();
diff --git a/src/objects/js-regexp.h b/src/objects/js-regexp.h
@@ -113,7 +113,11 @@ class JSRegExp : public TorqueGeneratedJSRegExp<JSRegExp, JSObject> {
   static constexpr int kMaxCaptures = 1 << 16;
 
   // Number of captures (without the match itself).
-  inline int CaptureCount();
+  inline int CaptureCount() const;
+  // Each capture (including the match itself) needs two registers.
+  static int RegistersForCaptureCount(int count) { return (count + 1) * 2; }
+
+  inline int MaxRegisterCount() const;
   inline Flags GetFlags();
   inline String Pattern();
   inline Object CaptureNameMap();
diff --git a/src/objects/objects.cc b/src/objects/objects.cc
@@ -4212,7 +4212,8 @@ Handle<RegExpMatchInfo> RegExpMatchInfo::ReserveCaptures(
     Isolate* isolate, Handle<RegExpMatchInfo> match_info, int capture_count) {
   DCHECK_GE(match_info->length(), kLastMatchOverhead);
 
-  int capture_register_count = (capture_count + 1) * 2;
+  int capture_register_count =
+      JSRegExp::RegistersForCaptureCount(capture_count);
   const int required_length = kFirstCaptureIndex + capture_register_count;
   Handle<RegExpMatchInfo> result = Handle<RegExpMatchInfo>::cast(
       EnsureSpaceInFixedArray(isolate, match_info, required_length));
diff --git a/src/regexp/regexp-compiler.cc b/src/regexp/regexp-compiler.cc
@@ -223,7 +223,7 @@ class RecursionCheck {
 // a fixed array or a null handle depending on whether it succeeded.
 RegExpCompiler::RegExpCompiler(Isolate* isolate, Zone* zone, int capture_count,
                                bool one_byte)
-    : next_register_(2 * (capture_count + 1)),
+    : next_register_(JSRegExp::RegistersForCaptureCount(capture_count)),
       unicode_lookaround_stack_register_(kNoRegister),
       unicode_lookaround_position_register_(kNoRegister),
       work_list_(nullptr),
diff --git a/src/regexp/regexp-interpreter.cc b/src/regexp/regexp-interpreter.cc
@@ -144,6 +144,48 @@ class BacktrackStack {
   DISALLOW_COPY_AND_ASSIGN(BacktrackStack);
 };
 
+// Registers used during interpreter execution. These consist of output
+// registers in indices [0, output_register_count[ which will contain matcher
+// results as a {start,end} index tuple for each capture (where the whole match
+// counts as implicit capture 0); and internal registers in indices
+// [output_register_count, total_register_count[.
+class InterpreterRegisters {
+ public:
+  using RegisterT = int;
+
+  InterpreterRegisters(int total_register_count, RegisterT* output_registers,
+                       int output_register_count)
+      : registers_(total_register_count),
+        output_registers_(output_registers),
+        output_register_count_(output_register_count) {
+    // TODO(jgruber): Use int32_t consistently for registers. Currently, CSA
+    // uses int32_t while runtime uses int.
+    STATIC_ASSERT(sizeof(int) == sizeof(int32_t));
+    DCHECK_GE(output_register_count, 2);  // At least 2 for the match itself.
+    DCHECK_GE(total_register_count, output_register_count);
+    DCHECK_LE(total_register_count, RegExpMacroAssembler::kMaxRegisterCount);
+    DCHECK_NOT_NULL(output_registers);
+
+    // Initialize the output register region to -1 signifying 'no match'.
+    std::memset(registers_.data(), -1,
+                output_register_count * sizeof(RegisterT));
+  }
+
+  const RegisterT& operator[](size_t index) const { return registers_[index]; }
+  RegisterT& operator[](size_t index) { return registers_[index]; }
+
+  void CopyToOutputRegisters() {
+    MemCopy(output_registers_, registers_.data(),
+            output_register_count_ * sizeof(RegisterT));
+  }
+
+ private:
+  static constexpr int kStaticCapacity = 64;  // Arbitrary.
+  base::SmallVector<RegisterT, kStaticCapacity> registers_;
+  RegisterT* const output_registers_;
+  const int output_register_count_;
+};
+
 IrregexpInterpreter::Result ThrowStackOverflow(Isolate* isolate,
                                                RegExp::CallOrigin call_origin) {
   CHECK(call_origin == RegExp::CallOrigin::kFromRuntime);
@@ -305,12 +347,12 @@ bool CheckBitInTable(const uint32_t current_char, const byte* const table) {
 #endif  // DEBUG
 
 template <typename Char>
-IrregexpInterpreter::Result RawMatch(Isolate* isolate, ByteArray code_array,
-                                     String subject_string,
-                                     Vector<const Char> subject, int* registers,
-                                     int current, uint32_t current_char,
-                                     RegExp::CallOrigin call_origin,
-                                     const uint32_t backtrack_limit) {
+IrregexpInterpreter::Result RawMatch(
+    Isolate* isolate, ByteArray code_array, String subject_string,
+    Vector<const Char> subject, int* output_registers,
+    int output_register_count, int total_register_count, int current,
+    uint32_t current_char, RegExp::CallOrigin call_origin,
+    const uint32_t backtrack_limit) {
   DisallowHeapAllocation no_gc;
 
 #if V8_USE_COMPUTED_GOTO
@@ -364,6 +406,8 @@ IrregexpInterpreter::Result RawMatch(Isolate* isolate, ByteArray code_array,
   const byte* pc = code_array.GetDataStartAddress();
   const byte* code_base = pc;
 
+  InterpreterRegisters registers(total_register_count, output_registers,
+                                 output_register_count);
   BacktrackStack backtrack_stack;
 
   uint32_t backtrack_count = 0;
@@ -471,6 +515,7 @@ IrregexpInterpreter::Result RawMatch(Isolate* isolate, ByteArray code_array,
     BYTECODE(SUCCEED) {
       isolate->counters()->regexp_backtracks()->AddSample(
           static_cast<int>(backtrack_count));
+      registers.CopyToOutputRegisters();
       return IrregexpInterpreter::SUCCESS;
     }
     BYTECODE(ADVANCE_CP) {
@@ -952,24 +997,25 @@ IrregexpInterpreter::Result RawMatch(Isolate* isolate, ByteArray code_array,
 
 // static
 IrregexpInterpreter::Result IrregexpInterpreter::Match(
-    Isolate* isolate, JSRegExp regexp, String subject_string, int* registers,
-    int registers_length, int start_position, RegExp::CallOrigin call_origin) {
-  if (FLAG_regexp_tier_up) {
-    regexp.TierUpTick();
-  }
+    Isolate* isolate, JSRegExp regexp, String subject_string,
+    int* output_registers, int output_register_count, int start_position,
+    RegExp::CallOrigin call_origin) {
+  if (FLAG_regexp_tier_up) regexp.TierUpTick();
 
   bool is_one_byte = String::IsOneByteRepresentationUnderneath(subject_string);
   ByteArray code_array = ByteArray::cast(regexp.Bytecode(is_one_byte));
+  int total_register_count = regexp.MaxRegisterCount();
 
-  return MatchInternal(isolate, code_array, subject_string, registers,
-                       registers_length, start_position, call_origin,
-                       regexp.BacktrackLimit());
+  return MatchInternal(isolate, code_array, subject_string, output_registers,
+                       output_register_count, total_register_count,
+                       start_position, call_origin, regexp.BacktrackLimit());
 }
 
 IrregexpInterpreter::Result IrregexpInterpreter::MatchInternal(
     Isolate* isolate, ByteArray code_array, String subject_string,
-    int* registers, int registers_length, int start_position,
-    RegExp::CallOrigin call_origin, uint32_t backtrack_limit) {
+    int* output_registers, int output_register_count, int total_register_count,
+    int start_position, RegExp::CallOrigin call_origin,
+    uint32_t backtrack_limit) {
   DCHECK(subject_string.IsFlat());
 
   // Note: Heap allocation *is* allowed in two situations if calling from
@@ -980,27 +1026,23 @@ IrregexpInterpreter::Result IrregexpInterpreter::MatchInternal(
   //    after interrupts have run.
   DisallowHeapAllocation no_gc;
 
-  // Reset registers to -1 (=undefined).
-  // This is necessary because registers are only written when a
-  // capture group matched.
-  // Resetting them ensures that previous matches are cleared.
-  memset(registers, -1, sizeof(registers[0]) * registers_length);
-
   uc16 previous_char = '\n';
   String::FlatContent subject_content = subject_string.GetFlatContent(no_gc);
   if (subject_content.IsOneByte()) {
     Vector<const uint8_t> subject_vector = subject_content.ToOneByteVector();
     if (start_position != 0) previous_char = subject_vector[start_position - 1];
     return RawMatch(isolate, code_array, subject_string, subject_vector,
-                    registers, start_position, previous_char, call_origin,
-                    backtrack_limit);
+                    output_registers, output_register_count,
+                    total_register_count, start_position, previous_char,
+                    call_origin, backtrack_limit);
   } else {
     DCHECK(subject_content.IsTwoByte());
     Vector<const uc16> subject_vector = subject_content.ToUC16Vector();
     if (start_position != 0) previous_char = subject_vector[start_position - 1];
     return RawMatch(isolate, code_array, subject_string, subject_vector,
-                    registers, start_position, previous_char, call_origin,
-                    backtrack_limit);
+                    output_registers, output_register_count,
+                    total_register_count, start_position, previous_char,
+                    call_origin, backtrack_limit);
   }
 }
 
@@ -1009,11 +1051,11 @@ IrregexpInterpreter::Result IrregexpInterpreter::MatchInternal(
 // This method is called through an external reference from RegExpExecInternal
 // builtin.
 IrregexpInterpreter::Result IrregexpInterpreter::MatchForCallFromJs(
-    Address subject, int32_t start_position, Address, Address, int* registers,
-    int32_t registers_length, Address, RegExp::CallOrigin call_origin,
-    Isolate* isolate, Address regexp) {
+    Address subject, int32_t start_position, Address, Address,
+    int* output_registers, int32_t output_register_count, Address,
+    RegExp::CallOrigin call_origin, Isolate* isolate, Address regexp) {
   DCHECK_NOT_NULL(isolate);
-  DCHECK_NOT_NULL(registers);
+  DCHECK_NOT_NULL(output_registers);
   DCHECK(call_origin == RegExp::CallOrigin::kFromJs);
 
   DisallowHeapAllocation no_gc;
@@ -1028,38 +1070,18 @@ IrregexpInterpreter::Result IrregexpInterpreter::MatchForCallFromJs(
     return IrregexpInterpreter::RETRY;
   }
 
-  // In generated code, registers are allocated on the stack. The given
-  // `registers` argument is only guaranteed to hold enough space for permanent
-  // registers (i.e. for captures), and not for temporary registers used only
-  // during matcher execution. We match that behavior in the interpreter by
-  // using a SmallVector as internal register storage.
-  static constexpr int kBaseRegisterArraySize = 64;  // Arbitrary.
-  const int internal_register_count =
-      Smi::ToInt(regexp_obj.DataAt(JSRegExp::kIrregexpMaxRegisterCountIndex));
-  base::SmallVector<int, kBaseRegisterArraySize> internal_registers(
-      internal_register_count);
-
-  Result result =
-      Match(isolate, regexp_obj, subject_string, internal_registers.data(),
-            internal_register_count, start_position, call_origin);
-
-  // Copy capture registers to the output array.
-  if (result == IrregexpInterpreter::SUCCESS) {
-    CHECK_GE(internal_registers.size(), registers_length);
-    MemCopy(registers, internal_registers.data(),
-            registers_length * sizeof(registers[0]));
-  }
-
-  return result;
+  return Match(isolate, regexp_obj, subject_string, output_registers,
+               output_register_count, start_position, call_origin);
 }
 
 #endif  // !COMPILING_IRREGEXP_FOR_EXTERNAL_EMBEDDER
 
 IrregexpInterpreter::Result IrregexpInterpreter::MatchForCallFromRuntime(
     Isolate* isolate, Handle<JSRegExp> regexp, Handle<String> subject_string,
-    int* registers, int registers_length, int start_position) {
-  return Match(isolate, *regexp, *subject_string, registers, registers_length,
-               start_position, RegExp::CallOrigin::kFromRuntime);
+    int* output_registers, int output_register_count, int start_position) {
+  return Match(isolate, *regexp, *subject_string, output_registers,
+               output_register_count, start_position,
+               RegExp::CallOrigin::kFromRuntime);
 }
 
 }  // namespace internal
diff --git a/src/regexp/regexp-interpreter.h b/src/regexp/regexp-interpreter.h
@@ -23,36 +23,42 @@ class V8_EXPORT_PRIVATE IrregexpInterpreter : public AllStatic {
 
   // In case a StackOverflow occurs, a StackOverflowException is created and
   // EXCEPTION is returned.
-  static Result MatchForCallFromRuntime(Isolate* isolate,
-                                        Handle<JSRegExp> regexp,
-                                        Handle<String> subject_string,
-                                        int* registers, int registers_length,
-                                        int start_position);
+  static Result MatchForCallFromRuntime(
+      Isolate* isolate, Handle<JSRegExp> regexp, Handle<String> subject_string,
+      int* output_registers, int output_register_count, int start_position);
 
   // In case a StackOverflow occurs, EXCEPTION is returned. The caller is
   // responsible for creating the exception.
+  //
   // RETRY is returned if a retry through the runtime is needed (e.g. when
   // interrupts have been scheduled or the regexp is marked for tier-up).
+  //
   // Arguments input_start, input_end and backtrack_stack are
   // unused. They are only passed to match the signature of the native irregex
   // code.
+  //
+  // Arguments output_registers and output_register_count describe the results
+  // array, which will contain register values of all captures if SUCCESS is
+  // returned. For all other return codes, the results array remains unmodified.
   static Result MatchForCallFromJs(Address subject, int32_t start_position,
                                    Address input_start, Address input_end,
-                                   int* registers, int32_t registers_length,
+                                   int* output_registers,
+                                   int32_t output_register_count,
                                    Address backtrack_stack,
                                    RegExp::CallOrigin call_origin,
                                    Isolate* isolate, Address regexp);
 
   static Result MatchInternal(Isolate* isolate, ByteArray code_array,
-                              String subject_string, int* registers,
-                              int registers_length, int start_position,
+                              String subject_string, int* output_registers,
+                              int output_register_count,
+                              int total_register_count, int start_position,
                               RegExp::CallOrigin call_origin,
                               uint32_t backtrack_limit);
 
  private:
   static Result Match(Isolate* isolate, JSRegExp regexp, String subject_string,
-                      int* registers, int registers_length, int start_position,
-                      RegExp::CallOrigin call_origin);
+                      int* output_registers, int output_register_count,
+                      int start_position, RegExp::CallOrigin call_origin);
 };
 
 }  // namespace internal
diff --git a/src/regexp/regexp-macro-assembler.h b/src/regexp/regexp-macro-assembler.h
@@ -28,13 +28,14 @@ struct DisjunctDecisionRow {
 class RegExpMacroAssembler {
  public:
   // The implementation must be able to handle at least:
-  static const int kMaxRegister = (1 << 16) - 1;
-  static const int kMaxCPOffset = (1 << 15) - 1;
-  static const int kMinCPOffset = -(1 << 15);
-
-  static const int kTableSizeBits = 7;
-  static const int kTableSize = 1 << kTableSizeBits;
-  static const int kTableMask = kTableSize - 1;
+  static constexpr int kMaxRegisterCount = (1 << 16);
+  static constexpr int kMaxRegister = kMaxRegisterCount - 1;
+  static constexpr int kMaxCPOffset = (1 << 15) - 1;
+  static constexpr int kMinCPOffset = -(1 << 15);
+
+  static constexpr int kTableSizeBits = 7;
+  static constexpr int kTableSize = 1 << kTableSizeBits;
+  static constexpr int kTableMask = kTableSize - 1;
 
   static constexpr int kUseCharactersValue = -1;
 
diff --git a/src/regexp/regexp.cc b/src/regexp/regexp.cc
diff --git a/src/runtime/runtime-regexp.cc b/src/runtime/runtime-regexp.cc
diff --git a/test/cctest/test-regexp.cc b/test/cctest/test-regexp.cc

Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ JSRegExp::Type JSRegExp::TypeTag() const {`
`34`	`34`	`return static_cast<JSRegExp::Type>(smi.value());`
`35`	`35`	`}`
`36`	`36`
`37`		`-int JSRegExp::CaptureCount() {`
	`37`	`+int JSRegExp::CaptureCount() const {`
`38`	`38`	`switch (TypeTag()) {`
`39`	`39`	`case ATOM:`
`40`	`40`	`return 0;`
`@@ -45,6 +45,11 @@ int JSRegExp::CaptureCount() {`
`45`	`45`	`}`
`46`	`46`	`}`
`47`	`47`
	`48`	`+int JSRegExp::MaxRegisterCount() const {`
	`49`	`+ CHECK_EQ(TypeTag(), IRREGEXP);`
	`50`	`+ return Smi::ToInt(DataAt(kIrregexpMaxRegisterCountIndex));`
	`51`	`+}`
	`52`	`+`
`48`	`53`	`JSRegExp::Flags JSRegExp::GetFlags() {`
`49`	`54`	`DCHECK(this->data().IsFixedArray());`
`50`	`55`	`Object data = this->data();`