Refactor parser mode configuration for correctness

This patch refactors the parser and preparser interface to be more
readable and type-safe.  It has no behavior changes.

Previously, parsers and preparsers were configured via bitfield called
parser_flags in the Parser constructor, and flags in
PreParser::PreParseProgram, ParserApi::Parse, and ParserApi::PreParse.
This was error-prone in practice: six call sites passed incorrectly
typed values to this interface (a boolean FLAG value, a boolean false
and a boolean true value).  None of these errors were caught by the
compiler because it's just an "int".

The parser flags interface was also awkward because it encoded a
language mode, but the language mode was only used to turn on harmony
scoping or not -- it wasn't used to actually set the parser's language
mode.

Fundamentally these errors came in because of the desire for a
procedural parser interface, in ParserApi.  Because we need to be able
to configure the parser in various ways, the flags argument got added;
but no one understood how to use the flags properly.  Also they were
only used by constructors: callers packed bits, and the constructors
unpacked them into booleans on the parser or preparser.

The solution is to allow parser construction, configuration, and
invocation to be separated.  This patch does that.

It passes the existing tests.

BUG=

Review URL: https://codereview.chromium.org/13450007
Patch from Andy Wingo <wingo@igalia.com>.

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14151 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
parent c02bf451
......@@ -1655,7 +1655,7 @@ void ObjectTemplate::SetInternalFieldCount(int value) {
ScriptData* ScriptData::PreCompile(const char* input, int length) {
i::Utf8ToUtf16CharacterStream stream(
reinterpret_cast<const unsigned char*>(input), length);
return i::ParserApi::PreParse(&stream);
return i::PreParserApi::PreParse(&stream);
}
......@@ -1664,10 +1664,10 @@ ScriptData* ScriptData::PreCompile(v8::Handle<String> source) {
if (str->IsExternalTwoByteString()) {
i::ExternalTwoByteStringUtf16CharacterStream stream(
i::Handle<i::ExternalTwoByteString>::cast(str), 0, str->length());
return i::ParserApi::PreParse(&stream);
return i::PreParserApi::PreParse(&stream);
} else {
i::GenericStringUtf16CharacterStream stream(str, 0, str->length());
return i::ParserApi::PreParse(&stream);
return i::PreParserApi::PreParse(&stream);
}
}
......
......@@ -520,14 +520,15 @@ static Handle<SharedFunctionInfo> MakeFunctionInfo(CompilationInfo* info) {
// Only allow non-global compiles for eval.
ASSERT(info->is_eval() || info->is_global());
ParsingFlags flags = kNoParsingFlags;
if ((info->pre_parse_data() != NULL ||
String::cast(script->source())->length() > FLAG_min_preparse_length) &&
!DebuggerWantsEagerCompilation(info)) {
flags = kAllowLazy;
}
if (!ParserApi::Parse(info, flags)) {
return Handle<SharedFunctionInfo>::null();
{
Parser parser(info);
if ((info->pre_parse_data() != NULL ||
String::cast(script->source())->length() > FLAG_min_preparse_length) &&
!DebuggerWantsEagerCompilation(info))
parser.set_allow_lazy(true);
if (!parser.Parse()) {
return Handle<SharedFunctionInfo>::null();
}
}
// Measure how long it takes to do the compilation; only take the
......@@ -864,7 +865,7 @@ bool Compiler::CompileLazy(CompilationInfo* info) {
if (InstallCodeFromOptimizedCodeMap(info)) return true;
// Generate the AST for the lazily compiled function.
if (ParserApi::Parse(info, kNoParsingFlags)) {
if (Parser::Parse(info)) {
// Measure how long it takes to do the lazy compilation; only take the
// rest of the function into account to avoid overlap with the lazy
// parsing statistics.
......@@ -932,7 +933,7 @@ void Compiler::RecompileParallel(Handle<JSFunction> closure) {
return;
}
if (ParserApi::Parse(*info, kNoParsingFlags)) {
if (Parser::Parse(*info)) {
LanguageMode language_mode = info->function()->language_mode();
info->SetLanguageMode(language_mode);
shared->set_language_mode(language_mode);
......
......@@ -8124,8 +8124,7 @@ bool HOptimizedGraphBuilder::TryInline(CallKind call_kind,
// Parse and allocate variables.
CompilationInfo target_info(target, zone());
Handle<SharedFunctionInfo> target_shared(target->shared());
if (!ParserApi::Parse(&target_info, kNoParsingFlags) ||
!Scope::Analyze(&target_info)) {
if (!Parser::Parse(&target_info) || !Scope::Analyze(&target_info)) {
if (target_info.isolate()->has_pending_exception()) {
// Parse or scope error, never optimize this function.
SetStackOverflow();
......
......@@ -609,7 +609,7 @@ static void CompileScriptForTracker(Isolate* isolate, Handle<Script> script) {
CompilationInfoWithZone info(script);
info.MarkAsGlobal();
// Parse and don't allow skipping lazy functions.
if (ParserApi::Parse(&info, kNoParsingFlags)) {
if (Parser::Parse(&info)) {
// Compile the code.
LiveEditFunctionTracker tracker(info.isolate(), info.function());
if (Compiler::MakeCodeForLiveEdit(&info)) {
......
This diff is collapsed.
......@@ -163,17 +163,17 @@ class ScriptDataImpl : public ScriptData {
};
class ParserApi {
class PreParserApi {
public:
// Parses the source code represented by the compilation info and sets its
// function literal. Returns false (and deallocates any allocated AST
// nodes) if parsing failed.
static bool Parse(CompilationInfo* info, int flags);
// Generic preparser generating full preparse data.
// Pre-parse a character stream and return full preparse data.
//
// This interface is here instead of in preparser.h because it instantiates a
// preparser recorder object that is suited to the parser's purposes. Also,
// the preparser doesn't know about ScriptDataImpl.
static ScriptDataImpl* PreParse(Utf16CharacterStream* source);
};
// ----------------------------------------------------------------------------
// REGEXP PARSING
......@@ -425,18 +425,34 @@ class SingletonLogger;
class Parser {
public:
Parser(CompilationInfo* info,
int parsing_flags, // Combination of ParsingFlags
v8::Extension* extension,
ScriptDataImpl* pre_data);
explicit Parser(CompilationInfo* info);
virtual ~Parser() {
delete reusable_preparser_;
reusable_preparser_ = NULL;
}
bool allow_natives_syntax() const { return allow_natives_syntax_; }
bool allow_lazy() const { return allow_lazy_; }
bool allow_modules() { return scanner().HarmonyModules(); }
bool allow_harmony_scoping() { return scanner().HarmonyScoping(); }
bool allow_generators() const { return allow_generators_; }
void set_allow_natives_syntax(bool allow) { allow_natives_syntax_ = allow; }
void set_allow_lazy(bool allow) { allow_lazy_ = allow; }
void set_allow_modules(bool allow) { scanner().SetHarmonyModules(allow); }
void set_allow_harmony_scoping(bool allow) {
scanner().SetHarmonyScoping(allow);
}
void set_allow_generators(bool allow) { allow_generators_ = allow; }
// Parses the source code represented by the compilation info and sets its
// function literal. Returns false (and deallocates any allocated AST
// nodes) if parsing failed.
static bool Parse(CompilationInfo* info) { return Parser(info).Parse(); }
bool Parse();
// Returns NULL if parsing failed.
FunctionLiteral* ParseProgram();
FunctionLiteral* ParseLazy();
void ReportMessageAt(Scanner::Location loc,
const char* message,
......@@ -550,6 +566,7 @@ class Parser {
Mode old_mode_;
};
FunctionLiteral* ParseLazy();
FunctionLiteral* ParseLazy(Utf16CharacterStream* source,
ZoneScope* zone_scope);
......@@ -568,10 +585,15 @@ class Parser {
void ReportMessage(const char* message, Vector<const char*> args);
void ReportMessage(const char* message, Vector<Handle<String> > args);
void set_pre_parse_data(ScriptDataImpl *data) {
pre_parse_data_ = data;
symbol_cache_.Initialize(data ? data->symbol_count() : 0, zone());
}
bool inside_with() const { return top_scope_->inside_with(); }
Scanner& scanner() { return scanner_; }
Mode mode() const { return mode_; }
ScriptDataImpl* pre_data() const { return pre_data_; }
ScriptDataImpl* pre_parse_data() const { return pre_parse_data_; }
bool is_extended_mode() {
ASSERT(top_scope_ != NULL);
return top_scope_->is_extended_mode();
......@@ -833,13 +855,13 @@ class Parser {
FunctionState* current_function_state_;
Target* target_stack_; // for break, continue statements
v8::Extension* extension_;
ScriptDataImpl* pre_data_;
ScriptDataImpl* pre_parse_data_;
FuncNameInferrer* fni_;
Mode mode_;
bool allow_natives_syntax_;
bool allow_lazy_;
bool allow_modules_;
bool allow_generators_;
bool stack_overflow_;
// If true, the next (and immediately following) function literal is
// preceded by a parenthesis.
......
......@@ -191,11 +191,9 @@ PreParserData Preparse(UnicodeInputStream* input, size_t max_stack) {
internal::Scanner scanner(&unicode_cache);
scanner.Initialize(&buffer);
internal::CompleteParserRecorder recorder;
preparser::PreParser::PreParseResult result =
preparser::PreParser::PreParseProgram(&scanner,
&recorder,
internal::kAllowLazy,
stack_limit);
preparser::PreParser preparser(&scanner, &recorder, stack_limit);
preparser.set_allow_lazy(true);
preparser::PreParser::PreParseResult result = preparser.PreParseProgram();
if (result == preparser::PreParser::kPreParseStackOverflow) {
return PreParserData::StackOverflow();
}
......
......@@ -179,7 +179,7 @@ PreParser::SourceElements PreParser::ParseSourceElements(int end_token,
Statement statement = ParseSourceElement(CHECK_OK);
if (allow_directive_prologue) {
if (statement.IsUseStrictLiteral()) {
set_language_mode(harmony_scoping_ ?
set_language_mode(allow_harmony_scoping() ?
i::EXTENDED_MODE : i::STRICT_MODE);
} else if (!statement.IsStringLiteral()) {
allow_directive_prologue = false;
......
......@@ -119,11 +119,7 @@ class PreParser {
PreParser(i::Scanner* scanner,
i::ParserRecorder* log,
uintptr_t stack_limit,
bool allow_lazy,
bool allow_natives_syntax,
bool allow_modules,
bool allow_generators)
uintptr_t stack_limit)
: scanner_(scanner),
log_(log),
scope_(NULL),
......@@ -131,30 +127,43 @@ class PreParser {
strict_mode_violation_location_(i::Scanner::Location::invalid()),
strict_mode_violation_type_(NULL),
stack_overflow_(false),
allow_lazy_(allow_lazy),
allow_modules_(allow_modules),
allow_natives_syntax_(allow_natives_syntax),
allow_generators_(allow_generators),
parenthesized_function_(false),
harmony_scoping_(scanner->HarmonyScoping()) { }
allow_lazy_(false),
allow_natives_syntax_(false),
allow_generators_(false),
parenthesized_function_(false) { }
~PreParser() {}
bool allow_natives_syntax() const { return allow_natives_syntax_; }
bool allow_lazy() const { return allow_lazy_; }
bool allow_modules() const { return scanner_->HarmonyModules(); }
bool allow_harmony_scoping() const { return scanner_->HarmonyScoping(); }
bool allow_generators() const { return allow_generators_; }
void set_allow_natives_syntax(bool allow) { allow_natives_syntax_ = allow; }
void set_allow_lazy(bool allow) { allow_lazy_ = allow; }
void set_allow_modules(bool allow) { scanner_->SetHarmonyModules(allow); }
void set_allow_harmony_scoping(bool allow) {
scanner_->SetHarmonyScoping(allow);
}
void set_allow_generators(bool allow) { allow_generators_ = allow; }
// Pre-parse the program from the character stream; returns true on
// success (even if parsing failed, the pre-parse data successfully
// captured the syntax error), and false if a stack-overflow happened
// during parsing.
static PreParseResult PreParseProgram(i::Scanner* scanner,
i::ParserRecorder* log,
int flags,
uintptr_t stack_limit) {
bool allow_lazy = (flags & i::kAllowLazy) != 0;
bool allow_natives_syntax = (flags & i::kAllowNativesSyntax) != 0;
bool allow_modules = (flags & i::kAllowModules) != 0;
bool allow_generators = (flags & i::kAllowGenerators) != 0;
return PreParser(scanner, log, stack_limit, allow_lazy,
allow_natives_syntax, allow_modules,
allow_generators).PreParse();
PreParseResult PreParseProgram() {
Scope top_scope(&scope_, kTopLevelScope);
bool ok = true;
int start_position = scanner_->peek_location().beg_pos;
ParseSourceElements(i::Token::EOS, &ok);
if (stack_overflow_) return kPreParseStackOverflow;
if (!ok) {
ReportUnexpectedToken(scanner_->current_token());
} else if (!scope_->is_classic_mode()) {
CheckOctalLiteral(start_position, scanner_->location().end_pos, &ok);
}
return kPreParseSuccess;
}
// Parses a single function literal, from the opening parentheses before
......@@ -514,22 +523,6 @@ class PreParser {
bool is_generator_;
};
// Preparse the program. Only called in PreParseProgram after creating
// the instance.
PreParseResult PreParse() {
Scope top_scope(&scope_, kTopLevelScope);
bool ok = true;
int start_position = scanner_->peek_location().beg_pos;
ParseSourceElements(i::Token::EOS, &ok);
if (stack_overflow_) return kPreParseStackOverflow;
if (!ok) {
ReportUnexpectedToken(scanner_->current_token());
} else if (!scope_->is_classic_mode()) {
CheckOctalLiteral(start_position, scanner_->location().end_pos, &ok);
}
return kPreParseSuccess;
}
// Report syntax error
void ReportUnexpectedToken(i::Token::Value token);
void ReportMessageAt(i::Scanner::Location location,
......@@ -683,11 +676,9 @@ class PreParser {
const char* strict_mode_violation_type_;
bool stack_overflow_;
bool allow_lazy_;
bool allow_modules_;
bool allow_natives_syntax_;
bool allow_generators_;
bool parenthesized_function_;
bool harmony_scoping_;
};
} } // v8::preparser
......
......@@ -10774,14 +10774,14 @@ class ScopeIterator {
info.MarkAsEval();
info.SetContext(Handle<Context>(function_->context()));
}
if (ParserApi::Parse(&info, kNoParsingFlags) && Scope::Analyze(&info)) {
if (Parser::Parse(&info) && Scope::Analyze(&info)) {
scope = info.function()->scope();
}
RetrieveScopeChain(scope, shared_info);
} else {
// Function code
CompilationInfoWithZone info(shared_info);
if (ParserApi::Parse(&info, kNoParsingFlags) && Scope::Analyze(&info)) {
if (Parser::Parse(&info) && Scope::Analyze(&info)) {
scope = info.function()->scope();
}
RetrieveScopeChain(scope, shared_info);
......
......@@ -42,26 +42,6 @@ namespace v8 {
namespace internal {
// General collection of (multi-)bit-flags that can be passed to scanners and
// parsers to signify their (initial) mode of operation.
enum ParsingFlags {
kNoParsingFlags = 0,
// Embed LanguageMode values in parsing flags, i.e., equivalent to:
// CLASSIC_MODE = 0,
// STRICT_MODE,
// EXTENDED_MODE,
kLanguageModeMask = 0x03,
kAllowLazy = 0x04,
kAllowNativesSyntax = 0x08,
kAllowModules = 0x10,
kAllowGenerators = 0x20
};
STATIC_ASSERT((kLanguageModeMask & CLASSIC_MODE) == CLASSIC_MODE);
STATIC_ASSERT((kLanguageModeMask & STRICT_MODE) == STRICT_MODE);
STATIC_ASSERT((kLanguageModeMask & EXTENDED_MODE) == EXTENDED_MODE);
// Returns the value (0 .. 15) of a hexadecimal character c.
// If c is not a legal hexadecimal character, returns a value < 0.
inline int HexValue(uc32 c) {
......
......@@ -262,12 +262,11 @@ TEST(StandAlonePreParser) {
i::Scanner scanner(i::Isolate::Current()->unicode_cache());
scanner.Initialize(&stream);
int flags = i::kAllowLazy | i::kAllowNativesSyntax;
v8::preparser::PreParser preparser(&scanner, &log, stack_limit);
preparser.set_allow_lazy(true);
preparser.set_allow_natives_syntax(true);
v8::preparser::PreParser::PreParseResult result =
v8::preparser::PreParser::PreParseProgram(&scanner,
&log,
flags,
stack_limit);
preparser.PreParseProgram();
CHECK_EQ(v8::preparser::PreParser::kPreParseSuccess, result);
i::ScriptDataImpl data(log.ExtractData());
CHECK(!data.has_error());
......@@ -298,12 +297,11 @@ TEST(StandAlonePreParserNoNatives) {
i::Scanner scanner(i::Isolate::Current()->unicode_cache());
scanner.Initialize(&stream);
// Flags don't allow natives syntax.
// Preparser defaults to disallowing natives syntax.
v8::preparser::PreParser preparser(&scanner, &log, stack_limit);
preparser.set_allow_lazy(true);
v8::preparser::PreParser::PreParseResult result =
v8::preparser::PreParser::PreParseProgram(&scanner,
&log,
i::kAllowLazy,
stack_limit);
preparser.PreParseProgram();
CHECK_EQ(v8::preparser::PreParser::kPreParseSuccess, result);
i::ScriptDataImpl data(log.ExtractData());
// Data contains syntax error.
......@@ -329,7 +327,7 @@ TEST(RegressChromium62639) {
i::Utf8ToUtf16CharacterStream stream(
reinterpret_cast<const i::byte*>(program),
static_cast<unsigned>(strlen(program)));
i::ScriptDataImpl* data = i::ParserApi::PreParse(&stream);
i::ScriptDataImpl* data = i::PreParserApi::PreParse(&stream);
CHECK(data->HasError());
delete data;
}
......@@ -354,7 +352,7 @@ TEST(Regress928) {
i::Handle<i::String> source(
FACTORY->NewStringFromAscii(i::CStrVector(program)));
i::GenericStringUtf16CharacterStream stream(source, 0, source->length());
i::ScriptDataImpl* data = i::ParserApi::PreParse(&stream);
i::ScriptDataImpl* data = i::PreParserApi::PreParse(&stream);
CHECK(!data->HasError());
data->Initialize();
......@@ -400,12 +398,10 @@ TEST(PreParseOverflow) {
i::Scanner scanner(i::Isolate::Current()->unicode_cache());
scanner.Initialize(&stream);
v8::preparser::PreParser preparser(&scanner, &log, stack_limit);
preparser.set_allow_lazy(true);
v8::preparser::PreParser::PreParseResult result =
v8::preparser::PreParser::PreParseProgram(&scanner,
&log,
true,
stack_limit);
preparser.PreParseProgram();
CHECK_EQ(v8::preparser::PreParser::kPreParseStackOverflow, result);
}
......@@ -994,7 +990,6 @@ TEST(ScopePositions) {
int marker;
i::Isolate::Current()->stack_guard()->SetStackLimit(
reinterpret_cast<uintptr_t>(&marker) - 128 * 1024);
i::FLAG_harmony_scoping = true;
for (int i = 0; source_data[i].outer_prefix; i++) {
int kPrefixLen = Utf8LengthHelper(source_data[i].outer_prefix);
......@@ -1017,7 +1012,9 @@ TEST(ScopePositions) {
CHECK_EQ(source->length(), kProgramSize);
i::Handle<i::Script> script = FACTORY->NewScript(source);
i::CompilationInfoWithZone info(script);
i::Parser parser(&info, i::kAllowLazy | i::EXTENDED_MODE, NULL, NULL);
i::Parser parser(&info);
parser.set_allow_lazy(true);
parser.set_allow_harmony_scoping(true);
info.MarkAsGlobal();
info.SetLanguageMode(source_data[i].language_mode);
i::FunctionLiteral* function = parser.ParseProgram();
......@@ -1065,31 +1062,57 @@ i::Handle<i::String> FormatMessage(i::ScriptDataImpl* data) {
}
void TestParserSync(i::Handle<i::String> source, int flags) {
enum ParserFlag {
kAllowLazy,
kAllowNativesSyntax,
kAllowHarmonyScoping,
kAllowModules,
kAllowGenerators,
kParserFlagCount
};
static bool checkParserFlag(unsigned flags, ParserFlag flag) {
return flags & (1 << flag);
}
#define SET_PARSER_FLAGS(parser, flags) \
parser.set_allow_lazy(checkParserFlag(flags, kAllowLazy)); \
parser.set_allow_natives_syntax(checkParserFlag(flags, \
kAllowNativesSyntax)); \
parser.set_allow_harmony_scoping(checkParserFlag(flags, \
kAllowHarmonyScoping)); \
parser.set_allow_modules(checkParserFlag(flags, kAllowModules)); \
parser.set_allow_generators(checkParserFlag(flags, kAllowGenerators));
void TestParserSyncWithFlags(i::Handle<i::String> source, unsigned flags) {
uintptr_t stack_limit = i::Isolate::Current()->stack_guard()->real_climit();
bool harmony_scoping = ((i::kLanguageModeMask & flags) == i::EXTENDED_MODE);
// Preparse the data.
i::CompleteParserRecorder log;
i::Scanner scanner(i::Isolate::Current()->unicode_cache());
i::GenericStringUtf16CharacterStream stream(source, 0, source->length());
scanner.SetHarmonyScoping(harmony_scoping);
scanner.Initialize(&stream);
v8::preparser::PreParser::PreParseResult result =
v8::preparser::PreParser::PreParseProgram(
&scanner, &log, flags, stack_limit);
CHECK_EQ(v8::preparser::PreParser::kPreParseSuccess, result);
{
i::Scanner scanner(i::Isolate::Current()->unicode_cache());
i::GenericStringUtf16CharacterStream stream(source, 0, source->length());
v8::preparser::PreParser preparser(&scanner, &log, stack_limit);
SET_PARSER_FLAGS(preparser, flags);
scanner.Initialize(&stream);
v8::preparser::PreParser::PreParseResult result =
preparser.PreParseProgram();
CHECK_EQ(v8::preparser::PreParser::kPreParseSuccess, result);
}
i::ScriptDataImpl data(log.ExtractData());
// Parse the data
i::Handle<i::Script> script = FACTORY->NewScript(source);
bool save_harmony_scoping = i::FLAG_harmony_scoping;
i::FLAG_harmony_scoping = harmony_scoping;
i::CompilationInfoWithZone info(script);
i::Parser parser(&info, flags, NULL, NULL);
info.MarkAsGlobal();
i::FunctionLiteral* function = parser.ParseProgram();
i::FLAG_harmony_scoping = save_harmony_scoping;
i::FunctionLiteral* function;
{
i::Handle<i::Script> script = FACTORY->NewScript(source);
i::CompilationInfoWithZone info(script);
i::Parser parser(&info);
SET_PARSER_FLAGS(parser, flags);
info.MarkAsGlobal();
function = parser.ParseProgram();
}
// Check that preparsing fails iff parsing fails.
if (function == NULL) {
......@@ -1139,19 +1162,9 @@ void TestParserSync(i::Handle<i::String> source, int flags) {
}
void TestParserSyncWithFlags(i::Handle<i::String> source) {
static const int kFlagsCount = 6;
const int flags[kFlagsCount] = {
i::kNoParsingFlags | i::CLASSIC_MODE,
i::kNoParsingFlags | i::STRICT_MODE,
i::kNoParsingFlags | i::EXTENDED_MODE,
i::kAllowLazy | i::CLASSIC_MODE,
i::kAllowLazy | i::STRICT_MODE,
i::kAllowLazy | i::EXTENDED_MODE
};
for (int k = 0; k < kFlagsCount; ++k) {
TestParserSync(source, flags[k]);
void TestParserSync(i::Handle<i::String> source) {
for (unsigned flags = 0; flags < (1 << kParserFlagCount); ++flags) {
TestParserSyncWithFlags(source, flags);
}
}
......@@ -1254,7 +1267,7 @@ TEST(ParserSync) {
CHECK(length == kProgramSize);
i::Handle<i::String> source =
FACTORY->NewStringFromAscii(i::CStrVector(program.start()));
TestParserSyncWithFlags(source);
TestParserSync(source);
}
}
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment