[JSC] modules can be visited more than once when resolving bindings t…

…hrough "star" exports as long as the exportName is different each time

https://bugs.webkit.org/show_bug.cgi?id=178308

Reviewed by Mark Lam.

JSTests:

* test262.yaml:

Source/JavaScriptCore:

With the change of the spec[1], we now do not need to remember star resolution modules.
We reflect this change to our implementation. Since this change is covered by test262,
this patch improves the score of test262.

We also add logging to ResolveExport to debug it easily.

[1]: tc39/ecma262@a865e77

* runtime/AbstractModuleRecord.cpp:
(JSC::AbstractModuleRecord::ResolveQuery::dump const):
(JSC::AbstractModuleRecord::resolveExportImpl):

Canonical link: https://commits.webkit.org/194890@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223894 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog

@@ -1,3 +1,12 @@
+2017-10-15  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] modules can be visited more than once when resolving bindings through "star" exports as long as the exportName is different each time
+        https://bugs.webkit.org/show_bug.cgi?id=178308
+
+        Reviewed by Mark Lam.
+
+        * test262.yaml:
+
 2017-10-23  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Use fastJoin in Array#toString

JSTests/test262.yaml

@@ -85790,7 +85790,7 @@
 - path: test262/test/language/module-code/instn-iee-star-cycle-indirect-x_FIXTURE.js
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-iee-star-cycle.js
-  cmd: runTest262 :fail, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
+  cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-iee-trlng-comma.js
   cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-iee-trlng-comma_FIXTURE.js
@@ -85892,7 +85892,7 @@
 - path: test262/test/language/module-code/instn-named-star-cycle-indirect-x_FIXTURE.js
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-named-star-cycle.js
-  cmd: runTest262 :fail, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
+  cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-once.js
   cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-resolve-empty-export.js
@@ -85998,7 +85998,7 @@
 - path: test262/test/language/module-code/instn-star-star-cycle-indirect-x_FIXTURE.js
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-star-star-cycle.js
-  cmd: runTest262 :fail, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
+  cmd: runTest262 :normal, "NoException", ["../../../harness/assert.js", "../../../harness/sta.js"], [:module]
 - path: test262/test/language/module-code/instn-uniq-env-rec-other_FIXTURE.js
   cmd: prepareTest262Fixture
 - path: test262/test/language/module-code/instn-uniq-env-rec.js

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,22 @@
+2017-10-15  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] modules can be visited more than once when resolving bindings through "star" exports as long as the exportName is different each time
+        https://bugs.webkit.org/show_bug.cgi?id=178308
+
+        Reviewed by Mark Lam.
+
+        With the change of the spec[1], we now do not need to remember star resolution modules.
+        We reflect this change to our implementation. Since this change is covered by test262,
+        this patch improves the score of test262.
+
+        We also add logging to ResolveExport to debug it easily.
+
+        [1]: https://github.com/tc39/ecma262/commit/a865e778ff0fc60e26e3e1c589635103710766a1
+
+        * runtime/AbstractModuleRecord.cpp:
+        (JSC::AbstractModuleRecord::ResolveQuery::dump const):
+        (JSC::AbstractModuleRecord::resolveExportImpl):
+
 2017-10-24  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Use emitDumbVirtualCall in 32bit JIT

Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp

@@ -35,6 +35,9 @@
 #include "UnlinkedModuleProgramCodeBlock.h"
 
 namespace JSC {
+namespace AbstractModuleRecordInternal {
+static const bool verbose = false;
+} // namespace AbstractModuleRecordInternal
 
 const ClassInfo AbstractModuleRecord::s_info = { "AbstractModuleRecord", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(AbstractModuleRecord) };
 
@@ -177,6 +180,7 @@ struct AbstractModuleRecord::ResolveQuery {
         static bool equal(const ResolveQuery&, const ResolveQuery&);
         static const bool safeToCompareToEmptyOrDeleted = true;
     };
+    using HashTraits = WTF::CustomHashTraits<ResolveQuery>;
 
     ResolveQuery(AbstractModuleRecord* moduleRecord, UniquedStringImpl* exportName)
         : moduleRecord(moduleRecord)
@@ -211,6 +215,15 @@ struct AbstractModuleRecord::ResolveQuery {
         return exportName.isHashTableDeletedValue();
     }
 
+    void dump(PrintStream& out) const
+    {
+        if (!moduleRecord) {
+            out.print("<empty>");
+            return;
+        }
+        out.print(moduleRecord->moduleKey(), " \"", exportName.get(), "\"");
+    }
+
     // The module record is not marked from the GC. But these records are reachable from the JSGlobalObject.
     // So we don't care the reachability to this record.
     AbstractModuleRecord* moduleRecord;
@@ -245,7 +258,10 @@ auto AbstractModuleRecord::resolveExportImpl(ExecState* exec, const ResolveQuery
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    // http://www.ecma-international.org/ecma-262/6.0/#sec-resolveexport
+    if (AbstractModuleRecordInternal::verbose)
+        dataLog("Resolving ", root, "\n");
+
+    // https://tc39.github.io/ecma262/#sec-resolveexport
 
     // How to avoid C++ recursion in this function:
     // This function avoids C++ recursion of the naive ResolveExport implementation.
@@ -480,16 +496,28 @@ auto AbstractModuleRecord::resolveExportImpl(ExecState* exec, const ResolveQuery
     //  4. Once we follow star links, we should not retrieve the result from the cache and should not cache the result.
     //  5. Once we see star links, even if we have not yet traversed that star link path, we should disable caching.
 
-    typedef WTF::HashSet<ResolveQuery, ResolveQuery::Hash, WTF::CustomHashTraits<ResolveQuery>> ResolveSet;
+    using ResolveSet = WTF::HashSet<ResolveQuery, ResolveQuery::Hash, ResolveQuery::HashTraits>;
     enum class Type { Query, IndirectFallback, GatherStars };
     struct Task {
         ResolveQuery query;
         Type type;
     };
 
+    auto typeString = [] (Type type) -> const char* {
+        switch (type) {
+        case Type::Query:
+            return "Query";
+        case Type::IndirectFallback:
+            return "IndirectFallback";
+        case Type::GatherStars:
+            return "GatherStars";
+        }
+        RELEASE_ASSERT_NOT_REACHED();
+        return nullptr;
+    };
+
     Vector<Task, 8> pendingTasks;
     ResolveSet resolveSet;
-    HashSet<AbstractModuleRecord*> starSet;
 
     Vector<Resolution, 8> frames;
 
@@ -500,7 +528,7 @@ auto AbstractModuleRecord::resolveExportImpl(ExecState* exec, const ResolveQuery
     // Call when the query is not resolved in the current module.
     // It will enqueue the star resolution requests. Return "false" if the error occurs.
     auto resolveNonLocal = [&](const ResolveQuery& query) -> bool {
-        // http://www.ecma-international.org/ecma-262/6.0/#sec-resolveexport
+        // https://tc39.github.io/ecma262/#sec-resolveexport
         // section 15.2.1.16.3, step 6
         // If the "default" name is not resolved in the current module, we need to throw an error and stop resolution immediately,
         // Rationale to this error: A default export cannot be provided by an export *.
@@ -509,17 +537,12 @@ auto AbstractModuleRecord::resolveExportImpl(ExecState* exec, const ResolveQuery
         if (query.exportName == vm.propertyNames->defaultKeyword.impl())
             return false;
 
-        // step 7, If exportStarSet contains module, then return null.
-        if (!starSet.add(query.moduleRecord).isNewEntry)
-            return true;
-
         // Enqueue the task to gather the results of the stars.
         // And append the new Resolution frame to gather the local result of the stars.
         pendingTasks.append(Task { query, Type::GatherStars });
         foundStarLinks = true;
         frames.append(Resolution::notFound());
 
-
         // Enqueue the tasks in reverse order.
         for (auto iterator = query.moduleRecord->starExportEntries().rbegin(), end = query.moduleRecord->starExportEntries().rend(); iterator != end; ++iterator) {
             const RefPtr<UniquedStringImpl>& starModuleName = *iterator;
@@ -563,6 +586,9 @@ auto AbstractModuleRecord::resolveExportImpl(ExecState* exec, const ResolveQuery
         const Task task = pendingTasks.takeLast();
         const ResolveQuery& query = task.query;
 
+        if (AbstractModuleRecordInternal::verbose)
+            dataLog("    ", typeString(task.type), " ", task.query, "\n");
+
         switch (task.type) {
         case Type::Query: {
             AbstractModuleRecord* moduleRecord = query.moduleRecord;