[parser] Move some PPSD sanity checks to debug (reland)

Move some of PreParsedScopeData's santity checks, such as the magic
value separating skippable function data from scope data, to be debug
only, to save memory.

Start position of inner skippable functions is still kept, because it's
too good at catching bugs, but we may want to remove it in the future
as well.

Relanding unchanged after the (unrelated) flake it exposed is fixed in:
https://chromium-review.googlesource.com/1131503

Bug: chromium:818642
Change-Id: Id1d9fe757875cd05ea9a92b41e7256c3ee86fc8e
Reviewed-on: https://chromium-review.googlesource.com/1131505
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54380}

src/parsing/preparsed-scope-data.cc

@@ -24,19 +24,21 @@ class VariableMaybeAssignedField : public BitField8<bool, 0, 1> {};
 class VariableContextAllocatedField
     : public BitField8<bool, VariableMaybeAssignedField::kNext, 1> {};
 
-const int kMagicValue = 0xC0DE0DE;
 
 #ifdef DEBUG
+const int kMagicValue = 0xC0DE0DE;
+
 const size_t kUint32Size = 5;
 const size_t kUint8Size = 2;
 const size_t kQuarterMarker = 0;
+const size_t kPlaceholderSize = kUint32Size;
 #else
 const size_t kUint32Size = 4;
 const size_t kUint8Size = 1;
+const size_t kPlaceholderSize = 0;
 #endif
 
-const int kPlaceholderSize = kUint32Size;
-const int kSkippableFunctionDataSize = 4 * kUint32Size + 1 * kUint8Size;
+const size_t kSkippableFunctionDataSize = 4 * kUint32Size + 1 * kUint8Size;
 
 class LanguageField : public BitField8<LanguageMode, 0, 1> {};
 class UsesSuperField : public BitField8<bool, LanguageField::kNext, 1> {};
@@ -51,7 +53,7 @@ STATIC_ASSERT(LanguageModeSize <= LanguageField::kNumValues);
 
   (Skippable function data:)
   ------------------------------------
-  | scope_data_start                 |
+  | scope_data_start (debug only)    |
   ------------------------------------
   | data for inner function 1        |
   | ...                              |
@@ -59,11 +61,11 @@ STATIC_ASSERT(LanguageModeSize <= LanguageField::kNumValues);
   | data for inner function n        |
   | ...                              |
   ------------------------------------
-  (Scope allocation data:)             << scope_data_start points here
+  (Scope allocation data:)             << scope_data_start points here in debug
   ------------------------------------
-  magic value
+  magic value (debug only)
   ------------------------------------
-  scope positions
+  scope positions (debug only)
   ------------------------------------
   | scope type << only in debug      |
   | eval                             |
@@ -101,19 +103,19 @@ void ProducedPreParsedScopeData::ByteData::WriteUint32(uint32_t data) {
   free_quarters_in_last_byte_ = 0;
 }
 
+#ifdef DEBUG
 void ProducedPreParsedScopeData::ByteData::OverwriteFirstUint32(uint32_t data) {
   auto it = backing_store_.begin();
-#ifdef DEBUG
   // Check that that position already holds an item of the expected size.
   DCHECK_GE(backing_store_.size(), kUint32Size);
   DCHECK_EQ(*it, kUint32Size);
   ++it;
-#endif
   const uint8_t* d = reinterpret_cast<uint8_t*>(&data);
   for (size_t i = 0; i < 4; ++i) {
     *it++ = *d++;
   }
 }
+#endif
 
 void ProducedPreParsedScopeData::ByteData::WriteUint8(uint8_t data) {
 #ifdef DEBUG
@@ -166,8 +168,10 @@ ProducedPreParsedScopeData::ProducedPreParsedScopeData(
   if (parent != nullptr) {
     parent->data_for_inner_functions_.push_back(this);
   }
+#ifdef DEBUG
   // Reserve space for scope_data_start, written later:
   byte_data_->WriteUint32(0);
+#endif
 }
 
 // Create a ProducedPreParsedScopeData which is just a proxy for a previous
@@ -227,6 +231,9 @@ void ProducedPreParsedScopeData::AddSkippableFunction(
     return;
   }
 
+  // Start position is used for a sanity check when consuming the data, we could
+  // remove it in the future if we're very pressed for space but it's been good
+  // at catching bugs in the wild so far.
   byte_data_->WriteUint32(start_position);
   byte_data_->WriteUint32(end_position);
   byte_data_->WriteUint32(num_parameters);
@@ -259,6 +266,7 @@ void ProducedPreParsedScopeData::SaveScopeAllocationData(
     return;
   }
 
+#ifdef DEBUG
   byte_data_->OverwriteFirstUint32(scope_data_start);
 
   // For a data integrity check, write a value between data about skipped inner
@@ -266,6 +274,7 @@ void ProducedPreParsedScopeData::SaveScopeAllocationData(
   byte_data_->WriteUint32(kMagicValue);
   byte_data_->WriteUint32(scope->start_position());
   byte_data_->WriteUint32(scope->end_position());
+#endif
 
   SaveDataForScope(scope);
 }
@@ -488,9 +497,9 @@ void ConsumedPreParsedScopeData::SetData(Isolate* isolate,
   int scope_data_start = scope_data_->ReadUint32();
   scope_data_->SetPosition(scope_data_start);
   DCHECK_EQ(scope_data_->ReadUint32(), kMagicValue);
-#endif
   // The first data item is scope_data_start. Skip over it.
   scope_data_->SetPosition(kPlaceholderSize);
+#endif
 }
 
 ProducedPreParsedScopeData*
@@ -535,14 +544,16 @@ void ConsumedPreParsedScopeData::RestoreScopeAllocationData(
 
   ByteData::ReadingScope reading_scope(this);
 
+#ifdef DEBUG
   int magic_value_from_data = scope_data_->ReadUint32();
   // Check that we've consumed all inner function data.
-  CHECK_EQ(magic_value_from_data, kMagicValue);
+  DCHECK_EQ(magic_value_from_data, kMagicValue);
 
   int start_position_from_data = scope_data_->ReadUint32();
   int end_position_from_data = scope_data_->ReadUint32();
-  CHECK_EQ(start_position_from_data, scope->start_position());
-  CHECK_EQ(end_position_from_data, scope->end_position());
+  DCHECK_EQ(start_position_from_data, scope->start_position());
+  DCHECK_EQ(end_position_from_data, scope->end_position());
+#endif
 
   RestoreData(scope);
 

src/parsing/preparsed-scope-data.h

@@ -76,8 +76,10 @@ class ProducedPreParsedScopeData : public ZoneObject {
     void WriteUint8(uint8_t data);
     void WriteQuarter(uint8_t data);
 
+#ifdef DEBUG
     // For overwriting previously written data at position 0.
     void OverwriteFirstUint32(uint32_t data);
+#endif
 
     Handle<PodArray<uint8_t>> Serialize(Isolate* isolate);
 

test/cctest/parsing/test-preparser.cc

@@ -822,7 +822,9 @@ TEST(ProducingAndConsumingByteData) {
   bytes.WriteUint8(255);
   bytes.WriteUint32(0);
   bytes.WriteUint8(0);
+#ifdef DEBUG
   bytes.OverwriteFirstUint32(2017);
+#endif
   bytes.WriteUint8(100);
   // Write quarter bytes between uint8s and uint32s to verify they're stored
   // correctly.
@@ -845,7 +847,11 @@ TEST(ProducingAndConsumingByteData) {
       &bytes_for_reading, *data_on_heap);
 
   // Read the data back.
+#ifdef DEBUG
   CHECK_EQ(bytes_for_reading.ReadUint32(), 2017);
+#else
+  CHECK_EQ(bytes_for_reading.ReadUint32(), 1983);
+#endif
   CHECK_EQ(bytes_for_reading.ReadUint32(), 2147483647);
   CHECK_EQ(bytes_for_reading.ReadUint8(), 4);
   CHECK_EQ(bytes_for_reading.ReadUint8(), 255);