From 0a01afda76699a9bfff4d9626bf6092ac815cf3b Mon Sep 17 00:00:00 2001 From: "marja@chromium.org" Date: Thu, 20 Feb 2014 11:35:37 +0000 Subject: [PATCH] Re-enable Parser::symbol_cache_ (after a long time!) The Parser never used the symbol stream produced by the PreParser for anything useful, due to a bug introduced 3.5 years ago by https://codereview.chromium.org/3356010/diff/7001/src/parser.cc. The bug is that calling Initialize on symbol_cache_ doesn't change its length. So the length remains 0, and the "if" in Parser::LookupSymbol is always true, and Parser::LookupCachedSymbol is never called and symbol_cache_ never filled. This bug also masked a bug that the symbol stream produced by PreParser doesn't match what Parser wants to consume. The repro case is the following: var myo = {if: 4}; print(myo.if); PreParser doesn't log a symbol for the first "if", but in the corresponding place, Parser consumes one symbol from the symbol stream. Since the consumed symbols were never really used, this mismatch went unnoticed. This CL also fixes that bug. BUG= R=ulan@chromium.org Review URL: https://codereview.chromium.org/172753002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19505 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/parser.cc | 12 ++++++------ src/preparser.cc | 1 + test/cctest/cctest.h | 12 ++++++++++++ test/cctest/test-parsing.cc | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/parser.cc b/src/parser.cc index 902fcac81..68823e6c1 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -207,12 +207,12 @@ void RegExpBuilder::AddQuantifierToAtom( Handle Parser::LookupSymbol(int symbol_id) { - // Length of symbol cache is the number of identified symbols. - // If we are larger than that, or negative, it's not a cached symbol. - // This might also happen if there is no preparser symbol data, even - // if there is some preparser data. - if (static_cast(symbol_id) - >= static_cast(symbol_cache_.length())) { + // If there is no preparser symbol data, a negative number will be passed. In + // that case, we'll just read the literal from Scanner. This also guards + // against corrupt preparse data where the symbol id is larger than the symbol + // count. + if (symbol_id < 0 || + (pre_parse_data_ && symbol_id >= pre_parse_data_->symbol_count())) { if (scanner()->is_literal_ascii()) { return isolate()->factory()->InternalizeOneByteString( Vector::cast(scanner()->literal_ascii_string())); diff --git a/src/preparser.cc b/src/preparser.cc index e96c8cd89..a5de23ebe 100644 --- a/src/preparser.cc +++ b/src/preparser.cc @@ -1184,6 +1184,7 @@ PreParser::Expression PreParser::ParseObjectLiteral(bool* ok) { if (Token::IsKeyword(next)) { Consume(next); checker.CheckProperty(next, kValueProperty, CHECK_OK); + LogSymbol(); } else { // Unexpected token. *ok = false; diff --git a/test/cctest/cctest.h b/test/cctest/cctest.h index d9f76294e..a4991a5a1 100644 --- a/test/cctest/cctest.h +++ b/test/cctest/cctest.h @@ -315,6 +315,18 @@ static inline v8::Local CompileRun(const char* source) { } +static inline v8::Local PreCompileCompileRun(const char* source) { + v8::Local script_source = + v8::String::NewFromUtf8(v8::Isolate::GetCurrent(), source); + v8::ScriptData* preparse = v8::ScriptData::PreCompile(script_source); + v8::Local script = + v8::Script::Compile(script_source, NULL, preparse); + v8::Local result = script->Run(); + delete preparse; + return result; +} + + // Helper function that compiles and runs the source with given origin. static inline v8::Local CompileRunWithOrigin(const char* source, const char* origin_url, diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc index 30e97aabd..8aa940dd9 100644 --- a/test/cctest/test-parsing.cc +++ b/test/cctest/test-parsing.cc @@ -324,6 +324,43 @@ TEST(StandAlonePreParserNoNatives) { } +TEST(PreparsingObjectLiterals) { + // Regression test for a bug where the symbol stream produced by PreParser + // didn't match what Parser wanted to consume. + v8::Isolate* isolate = CcTest::isolate(); + v8::HandleScope handles(isolate); + v8::Local context = v8::Context::New(isolate); + v8::Context::Scope context_scope(context); + int marker; + CcTest::i_isolate()->stack_guard()->SetStackLimit( + reinterpret_cast(&marker) - 128 * 1024); + + { + const char* source = "var myo = {if: \"foo\"}; myo.if;"; + v8::Local result = PreCompileCompileRun(source); + CHECK(result->IsString()); + v8::String::Utf8Value utf8(result); + CHECK_EQ("foo", *utf8); + } + + { + const char* source = "var myo = {\"bar\": \"foo\"}; myo[\"bar\"];"; + v8::Local result = PreCompileCompileRun(source); + CHECK(result->IsString()); + v8::String::Utf8Value utf8(result); + CHECK_EQ("foo", *utf8); + } + + { + const char* source = "var myo = {1: \"foo\"}; myo[1];"; + v8::Local result = PreCompileCompileRun(source); + CHECK(result->IsString()); + v8::String::Utf8Value utf8(result); + CHECK_EQ("foo", *utf8); + } +} + + TEST(RegressChromium62639) { v8::V8::Initialize(); i::Isolate* isolate = CcTest::i_isolate(); -- 2.34.1