From 94af17a845213b2574ae00e9260cb7f4827999d4 Mon Sep 17 00:00:00 2001
From: "marja@chromium.org"
 <marja@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Tue, 25 Feb 2014 11:51:02 +0000
Subject: [PATCH] Fix the bit massaging code in
 CompleteParserRecorder::WriteNumber.

The original code, added by
https://codereview.chromium.org/3384003/diff/7001/src/parser.cc 3.5 years ago,
failed to write numbers which contain a chunk of 7 zeroes in the middle. The
smallest such number is 2^14, so this is a problem if the source file to
preparse contains 16384 or more symbols (which happens in the wild).

This bug went unnoticed because the symbol data was not used by Parser (see
https://codereview.chromium.org/172753002/ for starting to use it again) and
there were no tests.

R=ulan@chromium.org
BUG=346221
LOG=y

Review URL: https://codereview.chromium.org/179433004

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19538 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 src/preparse-data.cc        | 20 +++++++++++++++-----
 src/preparse-data.h         |  4 ++++
 test/cctest/test-parsing.cc | 44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/src/preparse-data.cc b/src/preparse-data.cc
index 8e08848..bba6323 100644
--- a/src/preparse-data.cc
+++ b/src/preparse-data.cc
@@ -167,16 +167,26 @@ Vector<unsigned> CompleteParserRecorder::ExtractData() {
 
 
 void CompleteParserRecorder::WriteNumber(int number) {
+  // Split the number into chunks of 7 bits. Write them one after another (the
+  // most significant first). Use the MSB of each byte for signalling that the
+  // number continues. See ScriptDataImpl::ReadNumber for the reading side.
   ASSERT(number >= 0);
 
   int mask = (1 << 28) - 1;
-  for (int i = 28; i > 0; i -= 7) {
-    if (number > mask) {
-      symbol_store_.Add(static_cast<byte>(number >> i) | 0x80u);
-      number &= mask;
-    }
+  int i = 28;
+  // 26 million symbols ought to be enough for anybody.
+  ASSERT(number <= mask);
+  while (number < mask) {
     mask >>= 7;
+    i -= 7;
   }
+  while (i > 0) {
+    symbol_store_.Add(static_cast<byte>(number >> i) | 0x80u);
+    number &= mask;
+    mask >>= 7;
+    i -= 7;
+  }
+  ASSERT(number < (1 << 7));
   symbol_store_.Add(static_cast<byte>(number));
 }
 
diff --git a/src/preparse-data.h b/src/preparse-data.h
index 3a1e99d..15b0310 100644
--- a/src/preparse-data.h
+++ b/src/preparse-data.h
@@ -183,6 +183,10 @@ class CompleteParserRecorder: public FunctionLoggingParserRecorder {
   virtual int symbol_ids() { return symbol_id_; }
 
  private:
+  // For testing. Defined in test-parsing.cc.
+  friend void FakeWritingSymbolIdInPreParseData(CompleteParserRecorder* log,
+                                                int number);
+
   struct Key {
     bool is_ascii;
     Vector<const byte> literal_bytes;
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index 8aa940d..dff11aa 100644
--- a/test/cctest/test-parsing.cc
+++ b/test/cctest/test-parsing.cc
@@ -360,6 +360,50 @@ TEST(PreparsingObjectLiterals) {
   }
 }
 
+namespace v8 {
+namespace internal {
+
+void FakeWritingSymbolIdInPreParseData(i::CompleteParserRecorder* log,
+                                       int number) {
+  log->WriteNumber(number);
+  if (log->symbol_id_ < number + 1) {
+    log->symbol_id_ = number + 1;
+  }
+}
+
+}
+}
+
+
+TEST(StoringNumbersInPreParseData) {
+  // Symbol IDs are split into chunks of 7 bits for storing. This is a
+  // regression test for a bug where a symbol id was incorrectly stored if some
+  // of the chunks in the middle were all zeros.
+  i::CompleteParserRecorder log;
+  for (int i = 0; i < 18; ++i) {
+    FakeWritingSymbolIdInPreParseData(&log, 1 << i);
+  }
+  for (int i = 1; i < 18; ++i) {
+    FakeWritingSymbolIdInPreParseData(&log, (1 << i) + 1);
+  }
+  for (int i = 6; i < 18; ++i) {
+    FakeWritingSymbolIdInPreParseData(&log, (3 << i) + (5 << (i - 6)));
+  }
+  i::Vector<unsigned> store = log.ExtractData();
+  i::ScriptDataImpl script_data(store);
+  script_data.Initialize();
+  // Check that we get the same symbols back.
+  for (int i = 0; i < 18; ++i) {
+    CHECK_EQ(1 << i, script_data.GetSymbolIdentifier());
+  }
+  for (int i = 1; i < 18; ++i) {
+    CHECK_EQ((1 << i) + 1, script_data.GetSymbolIdentifier());
+  }
+  for (int i = 6; i < 18; ++i) {
+    CHECK_EQ((3 << i) + (5 << (i - 6)), script_data.GetSymbolIdentifier());
+  }
+}
+
 
 TEST(RegressChromium62639) {
   v8::V8::Initialize();
-- 
2.7.4