Reviewers: ulan,
Message:
ulan, here's another entertaining CL for you, ptal.
Description:
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.
[email protected]
BUG=346221
LOG=y
Please review this at https://codereview.chromium.org/179433004/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+63, -5 lines):
M src/preparse-data.h
M src/preparse-data.cc
M test/cctest/test-parsing.cc
Index: src/preparse-data.cc
diff --git a/src/preparse-data.cc b/src/preparse-data.cc
index
8e088482850b5183675dbc34bdc4c3b90d099782..bba63230073e38482d5c995b5f0963078c6de0f9
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));
}
Index: src/preparse-data.h
diff --git a/src/preparse-data.h b/src/preparse-data.h
index
3a1e99d5d187518e90312702a703e3328cb8340a..15b0310d5169ce21460e0ada7bc9357720894058
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;
Index: test/cctest/test-parsing.cc
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index
8aa940dd936a7dc0f453f3d1738dcaea9515ebb3..dff11aa63d8b1092c6cb66688cb05b3c211a8b36
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();
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
