Reviewers: ulan,
Message:
ulan, ptal.
I'm still pretty o.O about this finding. How come nobody ever noticed?
Description:
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 the symbol_cache_ is initially of size 0, so 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 symbol
stream was never really used, this mismatch went unnoticed.
This CL also fixes that bug.
BUG=
Please review this at https://codereview.chromium.org/172753002/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+53, -6 lines):
M src/parser.cc
M src/preparser.cc
M test/cctest/cctest.h
M test/cctest/test-parsing.cc
Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index
902fcac81b501fedc75fc3e0d4d9c474c4c5de7e..c7db12d87980a49e84fe8f7dec940fe7322ddd00
100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -207,12 +207,9 @@ void RegExpBuilder::AddQuantifierToAtom(
Handle<String> 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<unsigned>(symbol_id)
- >= static_cast<unsigned>(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.
+ if (symbol_id < 0) {
if (scanner()->is_literal_ascii()) {
return isolate()->factory()->InternalizeOneByteString(
Vector<const uint8_t>::cast(scanner()->literal_ascii_string()));
Index: src/preparser.cc
diff --git a/src/preparser.cc b/src/preparser.cc
index
e96c8cd892c028df8bba6d6a8168a28c5c8d0de2..a5de23ebeede10d2b59726f32d0294f6403555f1
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;
Index: test/cctest/cctest.h
diff --git a/test/cctest/cctest.h b/test/cctest/cctest.h
index
d9f76294e1a5d432db4b2fdf9127d39630a02e00..a4991a5a19711f8d71c2a38a151b9c953d815aac
100644
--- a/test/cctest/cctest.h
+++ b/test/cctest/cctest.h
@@ -315,6 +315,18 @@ static inline v8::Local<v8::Value> CompileRun(const
char* source) {
}
+static inline v8::Local<v8::Value> PreCompileCompileRun(const char*
source) {
+ v8::Local<v8::String> script_source =
+ v8::String::NewFromUtf8(v8::Isolate::GetCurrent(), source);
+ v8::ScriptData* preparse = v8::ScriptData::PreCompile(script_source);
+ v8::Local<v8::Script> script =
+ v8::Script::Compile(script_source, NULL, preparse);
+ v8::Local<v8::Value> result = script->Run();
+ delete preparse;
+ return result;
+}
+
+
// Helper function that compiles and runs the source with given origin.
static inline v8::Local<v8::Value> CompileRunWithOrigin(const char* source,
const char*
origin_url,
Index: test/cctest/test-parsing.cc
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index
30e97aabdcd5197867588979550531347a9bd799..8aa940dd936a7dc0f453f3d1738dcaea9515ebb3
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<v8::Context> context = v8::Context::New(isolate);
+ v8::Context::Scope context_scope(context);
+ int marker;
+ CcTest::i_isolate()->stack_guard()->SetStackLimit(
+ reinterpret_cast<uintptr_t>(&marker) - 128 * 1024);
+
+ {
+ const char* source = "var myo = {if: \"foo\"}; myo.if;";
+ v8::Local<v8::Value> 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<v8::Value> 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<v8::Value> 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();
--
--
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.
