[parser] Skipping inner funcs: implement a bailout.

In some cases, PreParser cannot replicate the Scope structure created by
Parser. It happens esp. with arrow function parameters, since the relevant
information is already lost by the time we figure out it's an arrow function.

In these cases, PreParser should bail out of trying to create data for skipping
inner functions.

Implementation notes:

- The arrow function case is more fundamental; the non-arrow case could be
  hacked together somehow if we implemented tracking is_simple for each param
  separately; but now that it's possible to bail out consistently from both
  cases, I don't think the is_simple complication is worth it.

- The added mjsunit test cases are based on the test262 test cases which exposed
  the problem.

- cctest/preparser/PreParserScopeAnalysis was exercising similar cases, but the
  problem didn't show up because the function parameters didn't contain
  skippable functions. Those test cases have been repurposed for testing the
  bailout.

- Extra precaution: the bailout tests are in a separate file, to guard from the
  bug that a bailout case results in bailing out of *all* data creation, which
  would make all skipping tests in the same file useless.

BUG=v8:5516

Change-Id: I4324749a5ec602fa5d7dc27647ade0284a6842fe
Reviewed-on: https://chromium-review.googlesource.com/599849Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47170}

src/parsing/preparsed-scope-data.cc

@@ -108,29 +108,33 @@ ProducedPreParsedScopeData::DataGatheringScope::DataGatheringScope(
     DeclarationScope* function_scope, PreParser* preparser)
     : function_scope_(function_scope),
       preparser_(preparser),
-      parent_data_(preparser->produced_preparsed_scope_data()) {
+      produced_preparsed_scope_data_(nullptr) {
   if (FLAG_experimental_preparser_scope_analysis) {
+    ProducedPreParsedScopeData* parent =
+        preparser->produced_preparsed_scope_data();
     Zone* main_zone = preparser->main_zone();
-    auto* new_data = new (main_zone) ProducedPreParsedScopeData(main_zone);
-    if (parent_data_ != nullptr) {
-      parent_data_->data_for_inner_functions_.push_back(new_data);
-    }
-    preparser->set_produced_preparsed_scope_data(new_data);
-    function_scope->set_produced_preparsed_scope_data(new_data);
+    produced_preparsed_scope_data_ =
+        new (main_zone) ProducedPreParsedScopeData(main_zone, parent);
+    preparser->set_produced_preparsed_scope_data(
+        produced_preparsed_scope_data_);
+    function_scope->set_produced_preparsed_scope_data(
+        produced_preparsed_scope_data_);
   }
 }
 
 ProducedPreParsedScopeData::DataGatheringScope::~DataGatheringScope() {
   if (FLAG_experimental_preparser_scope_analysis) {
-    preparser_->set_produced_preparsed_scope_data(parent_data_);
+    preparser_->set_produced_preparsed_scope_data(
+        produced_preparsed_scope_data_->parent_);
   }
 }
 
 void ProducedPreParsedScopeData::DataGatheringScope::MarkFunctionAsSkippable(
     int end_position, int num_inner_functions) {
   DCHECK(FLAG_experimental_preparser_scope_analysis);
-  DCHECK_NOT_NULL(parent_data_);
-  parent_data_->AddSkippableFunction(
+  DCHECK_NOT_NULL(produced_preparsed_scope_data_);
+  DCHECK_NOT_NULL(produced_preparsed_scope_data_->parent_);
+  produced_preparsed_scope_data_->parent_->AddSkippableFunction(
       function_scope_->start_position(), end_position,
       function_scope_->num_parameters(), num_inner_functions,
       function_scope_->language_mode(), function_scope_->uses_super_property());
@@ -144,6 +148,10 @@ void ProducedPreParsedScopeData::AddSkippableFunction(
   DCHECK_EQ(scope_data_start_, -1);
   DCHECK(previously_produced_preparsed_scope_data_.is_null());
 
+  if (bailed_out_) {
+    return;
+  }
+
   size_t current_size = backing_store_.size();
   backing_store_.resize(current_size + SkippableFunctionDataOffsets::kSize);
   backing_store_[current_size + SkippableFunctionDataOffsets::kStartPosition] =
@@ -171,6 +179,10 @@ void ProducedPreParsedScopeData::SaveScopeAllocationData(
   DCHECK_EQ(scope_data_start_, -1);
   DCHECK_EQ(backing_store_.size() % SkippableFunctionDataOffsets::kSize, 0);
 
+  if (bailed_out_) {
+    return;
+  }
+
   scope_data_start_ = static_cast<int>(backing_store_.size());
 
   // If there are no skippable inner functions, we don't need to save anything.
@@ -191,10 +203,17 @@ void ProducedPreParsedScopeData::SaveScopeAllocationData(
 MaybeHandle<PreParsedScopeData> ProducedPreParsedScopeData::Serialize(
     Isolate* isolate) const {
   if (!previously_produced_preparsed_scope_data_.is_null()) {
+    DCHECK(!bailed_out_);
     DCHECK_EQ(backing_store_.size(), 0);
     DCHECK_EQ(data_for_inner_functions_.size(), 0);
     return previously_produced_preparsed_scope_data_;
   }
+  if (bailed_out_) {
+    return MaybeHandle<PreParsedScopeData>();
+  }
+
+  DCHECK(!ThisOrParentBailedOut());
+
   // FIXME(marja): save space by using a byte array and converting
   // function data to bytes.
   size_t length = backing_store_.size();

src/parsing/preparsed-scope-data.h

@@ -69,19 +69,30 @@ class ProducedPreParsedScopeData : public ZoneObject {
  public:
   // Create a ProducedPreParsedScopeData object which will collect data as we
   // parse.
-  explicit ProducedPreParsedScopeData(Zone* zone)
-      : backing_store_(zone),
+  explicit ProducedPreParsedScopeData(Zone* zone,
+                                      ProducedPreParsedScopeData* parent)
+      : parent_(parent),
+        backing_store_(zone),
         data_for_inner_functions_(zone),
-        scope_data_start_(-1) {}
+        scope_data_start_(-1),
+        bailed_out_(false) {
+    if (parent != nullptr) {
+      parent->data_for_inner_functions_.push_back(this);
+    }
+  }
 
   // Create a ProducedPreParsedScopeData which is just a proxy for a previous
   // produced PreParsedScopeData.
   ProducedPreParsedScopeData(Handle<PreParsedScopeData> data, Zone* zone)
-      : backing_store_(zone),
+      : parent_(nullptr),
+        backing_store_(zone),
         data_for_inner_functions_(zone),
         scope_data_start_(-1),
+        bailed_out_(false),
         previously_produced_preparsed_scope_data_(data) {}
 
+  ProducedPreParsedScopeData* parent() const { return parent_; }
+
   // For gathering the inner function data and splitting it up according to the
   // laziness boundaries. Each lazy function gets its own
   // ProducedPreParsedScopeData, and so do all lazy functions inside it.
@@ -95,7 +106,7 @@ class ProducedPreParsedScopeData : public ZoneObject {
    private:
     DeclarationScope* function_scope_;
     PreParser* preparser_;
-    ProducedPreParsedScopeData* parent_data_;
+    ProducedPreParsedScopeData* produced_preparsed_scope_data_;
 
     DISALLOW_COPY_AND_ASSIGN(DataGatheringScope);
   };
@@ -104,6 +115,31 @@ class ProducedPreParsedScopeData : public ZoneObject {
   // subscopes') variables.
   void SaveScopeAllocationData(DeclarationScope* scope);
 
+  // In some cases, PreParser cannot produce the same Scope structure as
+  // Parser. If it happens, we're unable to produce the data that would enable
+  // skipping the inner functions of that function.
+  void Bailout() {
+    bailed_out_ = true;
+
+    // We don't need to call Bailout on existing / future children: the only way
+    // to try to retrieve their data is through calling Serialize on the parent,
+    // and if the parent is bailed out, it won't call Serialize on its children.
+  }
+
+  bool bailed_out() const { return bailed_out_; }
+
+#ifdef DEBUG
+  bool ThisOrParentBailedOut() const {
+    if (bailed_out_) {
+      return true;
+    }
+    if (parent_ == nullptr) {
+      return false;
+    }
+    return parent_->ThisOrParentBailedOut();
+  }
+#endif  // DEBUG
+
   // If there is data (if the Scope contains skippable inner functions), move
   // the data into the heap and return a Handle to it; otherwise return a null
   // MaybeHandle.
@@ -122,6 +158,8 @@ class ProducedPreParsedScopeData : public ZoneObject {
   void SaveDataForVariable(Variable* var);
   void SaveDataForInnerScopes(Scope* scope);
 
+  ProducedPreParsedScopeData* parent_;
+
   // TODO(marja): Make the backing store more efficient once we know exactly
   // what data is needed.
   ZoneDeque<uint32_t> backing_store_;
@@ -131,6 +169,9 @@ class ProducedPreParsedScopeData : public ZoneObject {
   // the latter starts.
   int scope_data_start_;
 
+  // Whether we've given up producing the data for this function.
+  bool bailed_out_;
+
   // ProducedPreParsedScopeData can also mask a Handle<PreParsedScopeData>
   // which was produced already earlier. This happens for deeper lazy functions.
   Handle<PreParsedScopeData> previously_produced_preparsed_scope_data_;

src/parsing/preparser.cc

@@ -383,6 +383,30 @@ PreParser::LazyParsingResult PreParser::ParseStatementListAndLogFunction(
   return kLazyParsingComplete;
 }
 
+PreParserStatement PreParser::BuildParameterInitializationBlock(
+    const PreParserFormalParameters& parameters, bool* ok) {
+  DCHECK(!parameters.is_simple);
+  if (FLAG_experimental_preparser_scope_analysis &&
+      scope()->calls_sloppy_eval()) {
+    DCHECK_NOT_NULL(produced_preparsed_scope_data_);
+    // We cannot replicate the Scope structure constructed by the Parser,
+    // because we've lost information whether each individual parameter was
+    // simple or not. Give up trying to produce data to skip inner functions.
+    if (produced_preparsed_scope_data_->parent() != nullptr) {
+      // Lazy parsing started before the current function; the function which
+      // cannot contain skippable functions is the parent function. (Its inner
+      // functions cannot either; they are implicitly bailed out.)
+      produced_preparsed_scope_data_->parent()->Bailout();
+    } else {
+      // Lazy parsing started at the current function; it cannot contain
+      // skippable functions.
+      produced_preparsed_scope_data_->Bailout();
+    }
+  }
+
+  return PreParserStatement::Default();
+}
+
 PreParserExpression PreParser::ExpressionFromIdentifier(
     PreParserIdentifier name, int start_position, InferName infer) {
   VariableProxy* proxy = nullptr;

src/parsing/preparser.h

@@ -1411,10 +1411,8 @@ class PreParser : public ParserBase<PreParser> {
     return loop;
   }
 
-  V8_INLINE PreParserStatement BuildParameterInitializationBlock(
-      const PreParserFormalParameters& parameters, bool* ok) {
-    return PreParserStatement::Default();
-  }
+  PreParserStatement BuildParameterInitializationBlock(
+      const PreParserFormalParameters& parameters, bool* ok);
 
   V8_INLINE PreParserStatement
   BuildRejectPromiseOnException(PreParserStatement init_block) {
@@ -1640,7 +1638,11 @@ class PreParser : public ParserBase<PreParser> {
 
   V8_INLINE void AddParameterInitializationBlock(
       const PreParserFormalParameters& parameters, PreParserStatementList body,
-      bool is_async, bool* ok) {}
+      bool is_async, bool* ok) {
+    if (!parameters.is_simple) {
+      BuildParameterInitializationBlock(parameters, ok);
+    }
+  }
 
   V8_INLINE void AddFormalParameter(PreParserFormalParameters* parameters,
                                     PreParserExpression pattern,

test/cctest/parsing/test-preparser.cc

@@ -25,6 +25,10 @@ enum SkipTests {
   SKIP_STRICT = SKIP_STRICT_FUNCTION | SKIP_STRICT_OUTER
 };
 
+enum class PreciseMaybeAssigned { YES, NO };
+
+enum class Bailout { BAILOUT_IF_OUTER_SLOPPY, NO };
+
 }  // namespace
 
 TEST(PreParserScopeAnalysis) {
@@ -119,19 +123,23 @@ TEST(PreParserScopeAnalysis) {
   struct Inner {
     Inner(const char* s) : source(s) {}  // NOLINT
     Inner(const char* s, SkipTests skip) : source(s), skip(skip) {}
-    Inner(const char* s, SkipTests skip, bool precise)
+    Inner(const char* s, SkipTests skip, PreciseMaybeAssigned precise)
         : source(s), skip(skip), precise_maybe_assigned(precise) {}
 
     Inner(const char* p, const char* s) : params(p), source(s) {}
     Inner(const char* p, const char* s, SkipTests skip)
         : params(p), source(s), skip(skip) {}
-    Inner(const char* p, const char* s, SkipTests skip, bool precise)
+    Inner(const char* p, const char* s, SkipTests skip,
+          PreciseMaybeAssigned precise)
         : params(p), source(s), skip(skip), precise_maybe_assigned(precise) {}
+    Inner(const char* p, const char* s, SkipTests skip, Bailout bailout)
+        : params(p), source(s), skip(skip), bailout(bailout) {}
 
     const char* params = "";
     const char* source;
     SkipTests skip = DONT_SKIP;
-    bool precise_maybe_assigned = true;
+    PreciseMaybeAssigned precise_maybe_assigned = PreciseMaybeAssigned::YES;
+    Bailout bailout = Bailout::NO;
   } inners[] = {
       // Simple cases
       {"var1;"},
@@ -144,7 +152,7 @@ TEST(PreParserScopeAnalysis) {
       // Var declarations and assignments.
       {"var var1;"},
       {"var var1; var1 = 5;"},
-      {"if (true) { var var1; }", DONT_SKIP, false},
+      {"if (true) { var var1; }", DONT_SKIP, PreciseMaybeAssigned::NO},
       {"if (true) { var var1; var1 = 5; }"},
       {"var var1; function f() { var1; }"},
       {"var var1; var1 = 5; function f() { var1; }"},
@@ -213,7 +221,7 @@ TEST(PreParserScopeAnalysis) {
       // Variable called "arguments"
       {"var arguments;", SKIP_STRICT},
       {"var arguments; arguments = 5;", SKIP_STRICT},
-      {"if (true) { var arguments; }", SKIP_STRICT, false},
+      {"if (true) { var arguments; }", SKIP_STRICT, PreciseMaybeAssigned::NO},
       {"if (true) { var arguments; arguments = 5; }", SKIP_STRICT},
       {"var arguments; function f() { arguments; }", SKIP_STRICT},
       {"var arguments; arguments = 5; function f() { arguments; }",
@@ -451,9 +459,10 @@ TEST(PreParserScopeAnalysis) {
       {"var1, ...var2", "function f1() { var2 = 9; }", SKIP_STRICT_FUNCTION},
 
       // Default parameters.
-      {"var1 = 3", "", SKIP_STRICT_FUNCTION, false},
-      {"var1, var2 = var1", "", SKIP_STRICT_FUNCTION, false},
-      {"var1, var2 = 4, ...var3", "", SKIP_STRICT_FUNCTION, false},
+      {"var1 = 3", "", SKIP_STRICT_FUNCTION, PreciseMaybeAssigned::NO},
+      {"var1, var2 = var1", "", SKIP_STRICT_FUNCTION, PreciseMaybeAssigned::NO},
+      {"var1, var2 = 4, ...var3", "", SKIP_STRICT_FUNCTION,
+       PreciseMaybeAssigned::NO},
 
       // Destructuring parameters. Because of the search space explosion, we
       // cannot test all interesting cases. Let's try to test a relevant subset.
@@ -497,10 +506,10 @@ TEST(PreParserScopeAnalysis) {
       // Complicated params.
       {"var1, [var2], var3 = 24, [var4, var5] = [2, 4], var6, {var7}, var8, "
        "{name9: var9, name10: var10}, ...var11",
-       "", SKIP_STRICT_FUNCTION, false},
+       "", SKIP_STRICT_FUNCTION, PreciseMaybeAssigned::NO},
 
       // Complicated cases from bugs.
-      {"var1 = {} = {}", "", SKIP_STRICT_FUNCTION, false},
+      {"var1 = {} = {}", "", SKIP_STRICT_FUNCTION, PreciseMaybeAssigned::NO},
 
       // Destructuring rest. Because we can.
       {"var1, ...[var2]", "", SKIP_STRICT_FUNCTION},
@@ -513,12 +522,16 @@ TEST(PreParserScopeAnalysis) {
       {"var1, ...{0: var2, 1: var3}", "", SKIP_STRICT_FUNCTION},
 
       // Default parameters for destruring parameters.
-      {"[var1, var2] = [2, 4]", "", SKIP_STRICT_FUNCTION, false},
-      {"{var1, var2} = {var1: 3, var2: 3}", "", SKIP_STRICT_FUNCTION, false},
+      {"[var1, var2] = [2, 4]", "", SKIP_STRICT_FUNCTION,
+       PreciseMaybeAssigned::NO},
+      {"{var1, var2} = {var1: 3, var2: 3}", "", SKIP_STRICT_FUNCTION,
+       PreciseMaybeAssigned::NO},
 
       // Default parameters inside destruring parameters.
-      {"[var1 = 4, var2 = var1]", "", SKIP_STRICT_FUNCTION, false},
-      {"{var1 = 4, var2 = var1}", "", SKIP_STRICT_FUNCTION, false},
+      {"[var1 = 4, var2 = var1]", "", SKIP_STRICT_FUNCTION,
+       PreciseMaybeAssigned::NO},
+      {"{var1 = 4, var2 = var1}", "", SKIP_STRICT_FUNCTION,
+       PreciseMaybeAssigned::NO},
 
       // Locals shadowing parameters.
       {"var1, var2", "var var1 = 16; () => { var1 = 17; }"},
@@ -531,22 +544,26 @@ TEST(PreParserScopeAnalysis) {
       {"var1, var2, ...var3", "var var3 = 16; () => { var3 = 17; }",
        SKIP_STRICT_FUNCTION},
       {"var1, var2 = var1", "var var1 = 16; () => { var1 = 17; }",
-       SKIP_STRICT_FUNCTION, false},
+       SKIP_STRICT_FUNCTION, PreciseMaybeAssigned::NO},
 
       // Hoisted sloppy block function shadowing a parameter.
       // FIXME(marja): why is maybe_assigned inaccurate?
-      {"var1, var2", "for (;;) { function var1() { } }", DONT_SKIP, false},
+      {"var1, var2", "for (;;) { function var1() { } }", DONT_SKIP,
+       PreciseMaybeAssigned::NO},
 
-      // Eval in default parameter.
+      // Sloppy eval in default parameter.
       {"var1, var2 = eval(''), var3", "let var4 = 0;", SKIP_STRICT_FUNCTION,
-       false},
+       Bailout::BAILOUT_IF_OUTER_SLOPPY},
       {"var1, var2 = eval(''), var3 = eval('')", "let var4 = 0;",
-       SKIP_STRICT_FUNCTION, false},
+       SKIP_STRICT_FUNCTION, Bailout::BAILOUT_IF_OUTER_SLOPPY},
 
-      // Eval in arrow function parameter list which is inside another arrow
-      // function parameter list.
+      // Sloppy eval in arrow function parameter list which is inside another
+      // arrow function parameter list.
       {"var1, var2 = (var3, var4 = eval(''), var5) => { let var6; }, var7",
-       "let var8 = 0;", SKIP_STRICT_FUNCTION},
+       "let var8 = 0;", SKIP_STRICT_FUNCTION, Bailout::BAILOUT_IF_OUTER_SLOPPY},
+
+      // Sloppy eval in a function body with non-simple parameters.
+      {"var1 = 1, var2 = 2", "eval('');", SKIP_STRICT_FUNCTION},
 
       // Catch variable
       {"try { } catch(var1) { }"},
@@ -660,6 +677,11 @@ TEST(PreParserScopeAnalysis) {
               ->produced_preparsed_scope_data();
       i::MaybeHandle<i::PreParsedScopeData> maybe_produced_data_on_heap =
           produced_data->Serialize(isolate);
+      if (inners[inner_ix].bailout == Bailout::BAILOUT_IF_OUTER_SLOPPY &&
+          !outers[outer_ix].strict_outer) {
+        DCHECK(maybe_produced_data_on_heap.is_null());
+        continue;
+      }
       DCHECK(!maybe_produced_data_on_heap.is_null());
       i::Handle<i::PreParsedScopeData> produced_data_on_heap =
           maybe_produced_data_on_heap.ToHandleChecked();
@@ -707,7 +729,7 @@ TEST(PreParserScopeAnalysis) {
 
       i::ScopeTestHelper::CompareScopes(
           normal_scope, unallocated_scope,
-          inners[inner_ix].precise_maybe_assigned);
+          inners[inner_ix].precise_maybe_assigned == PreciseMaybeAssigned::YES);
     }
   }
 }

test/mjsunit/skipping-inner-functions-bailout.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --experimental-preparser-scope-analysis
+
+// Tests for cases where PreParser must bail out of creating data for skipping
+// inner functions, since it cannot replicate the Scope structure created by
+// Parser.
+
+function TestBailoutBecauseOfSloppyEvalInArrowParams() {
+  let bailout = (a = function() {}, b = eval('')) => 0
+  bailout();
+
+  function not_skippable() {}
+}
+TestBailoutBecauseOfSloppyEvalInArrowParams();
+
+function TestBailoutBecauseOfSloppyEvalInArrowParams2() {
+  let bailout = (a = function() {}, b = eval('')) => {}
+  bailout();
+
+  function not_skippable() {}
+}
+TestBailoutBecauseOfSloppyEvalInArrowParams2();
+
+function TestBailoutBecauseOfSloppyEvalInParams() {
+  function bailout(a = function() {}, b = eval('')) {
+    function not_skippable() {}
+  }
+  bailout();
+
+  function not_skippable_either() {}
+}
+TestBailoutBecauseOfSloppyEvalInParams();
+
+ // Test bailing out from 2 places.
+function TestMultiBailout1() {
+  function bailout(a = function() {}, b = eval('')) {
+    function not_skippable() {}
+  }
+  bailout();
+
+  function bailout_too(a = function() {}, b = eval('')) {
+    function not_skippable_either() {}
+  }
+  bailout_too();
+}
+TestMultiBailout1();
+
+function TestMultiBailout2() {
+  function f(a = function() {}, b = eval('')) {
+    function not_skippable() {}
+  }
+  f();
+
+  function not_skippable_either() {
+    function bailout_too(a = function() {}, b = eval('')) {
+      function inner_not_skippable() {}
+    }
+    bailout_too();
+  }
+  not_skippable_either();
+}
+TestMultiBailout2();
+
+function TestMultiBailout3() {
+  function bailout(a = function() {}, b = eval('')) {
+    function bailout_too(a = function() {}, b = eval('')) {
+      function not_skippable() {}
+    }
+    bailout_too();
+  }
+  bailout();
+
+  function not_skippable_either() {}
+}
+TestMultiBailout3();

test/mjsunit/skipping-inner-functions.js

@@ -203,3 +203,48 @@ let f1 = (ctxt_alloc_param) => {
 }
 f1(9)();
 assertEquals(19, result);
+
+function TestStrictEvalInParams() {
+  "use strict";
+  var result = 0;
+
+  function lazy(a = function() { return 2; }, b = eval('3')) {
+    function skip_me() {
+      result = a() + b;
+    }
+    return skip_me;
+  }
+  lazy()();
+  assertEquals(5, result);
+
+  function not_skippable_either() {}
+}
+
+TestStrictEvalInParams();
+
+function TestSloppyEvalInFunctionWithComplexParams() {
+  var result = 0;
+
+  function lazy1(ctxt_alloc_param = 2) {
+    var ctxt_alloc_var = 3;
+    function skip_me() {
+      result = ctxt_alloc_param + ctxt_alloc_var;
+    }
+    eval('');
+    return skip_me;
+  }
+  lazy1()();
+  assertEquals(5, result);
+
+  function lazy2(ctxt_alloc_param = 4) {
+    var ctxt_alloc_var = 5;
+    function skip_me() {
+      eval('result = ctxt_alloc_param + ctxt_alloc_var;');
+    }
+    return skip_me;
+  }
+  lazy2()();
+  assertEquals(9, result);
+}
+
+TestSloppyEvalInFunctionWithComplexParams();