New issue
Advanced search Search tips

Issue 5685 link

Starred by 12 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
HW: All
NextAction: ----
OS: All
Priority: 2
Type: Bug

Blocking:
issue 1569



Sign in to add a comment

Interoperability between modules and compilation cache causes problems

Project Member Reported by mstarzinger@chromium.org, Nov 29 2016

Issue description

The compilation cache canonicalizes SharedFunctionInfo objects for scripts with the same source and origin. This also applies to modules having the same source text and module name. As a side effect the top-level code of such a SharedFunctionInfo might be executed more than once.

This has the potential to cause additional bugs further down the compilation pipeline. One particular case that comes to mind is that optimized code will also be shared (if the native context is the same). This would prevent specialization of optimized code for one given module contexts (i.e. Runtime_PushModuleContext is executed more than once for a single SharedFunctionInfo).

As a more immediate problem, trying to reproduce the case where such canonicalization via the compilation cache kicks in already triggers existing assertions. The following is a repro that executes a module (containing a simple import) twice. Further down the line the DebugPrint statements should reveal the sharing of SharedFunctionInfo objects even for nested functions ...

---

$ ./out/x64.debug/d8 --allow-natives-syntax --module mod-import.js --module mod-import.js
#
# Fatal error in ../src/d8.cc, line 712
# Check failed: d->specifier_to_module_map .insert(std::make_pair(file_name, Global<Module>(isolate, module))) .second.
#

---

$ cat mod-import.js
import {f as foo} from "mod.js";
%DebugPrint(foo);
foo();
foo();
%OptimizeFunctionOnNextCall(foo);
foo();
%DebugPrint(foo);

---

$ cat mod.js 
var x = Math.random();
export function f() { print(x); }

---
 

Comment 1 by neis@chromium.org, Nov 30 2016

Blocking: 1569

Comment 2 by neis@chromium.org, Dec 16 2016

Here's a d8 test case illustrating the problem of a script and a module receiving the same SFI:

$ d8 /absolute/path/to/current/directory/junk --module junk

#
# Fatal error in ../../src/ast/scopeinfo.cc, line 540
# Check failed: scope_type() == MODULE_SCOPE.
#

(junk can be an empty file for instance)

Comment 3 by adamk@chromium.org, Dec 16 2016

Yup, I think that's the problem this bug should be targeted at fixing now (adding an "is_module" bit to the cache key).

Comment 4 by neis@chromium.org, Jan 3 2017

Status: Started (was: Available)

Comment 5 by neis@chromium.org, Jan 3 2017

PTAL at https://codereview.chromium.org/2611643002 for one way of achieving this: I'm adding an IsModule flag to the source's origin (more precisely its ScriptOriginOptions). The origin is already part of the cache's key.
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b0f5abbea372b0cc4d27f267de271766f5c8a11a

commit b0f5abbea372b0cc4d27f267de271766f5c8a11a
Author: neis <neis@chromium.org>
Date: Thu Jan 19 06:59:20 2017

[modules] Add an IsModule flag to ScriptOriginOptions.

Since the script origin is part of the key used in the compilation
cache, this ensures that the cache never confuses a module with a
non-module script.

BUG= v8:1569 , v8:5685 

Review-Url: https://codereview.chromium.org/2611643002
Cr-Commit-Position: refs/heads/master@{#42490}

[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/include/v8.h
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/src/api.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/src/bootstrapper.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/src/compiler.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/src/compiler.h
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/src/d8.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/src/objects.h
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/test/cctest/compiler/test-linkage.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/test/cctest/interpreter/bytecode-expectations-printer.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/test/cctest/test-compiler.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/test/cctest/test-modules.cc
[modify] https://crrev.com/b0f5abbea372b0cc4d27f267de271766f5c8a11a/test/cctest/test-serialize.cc

Labels: Priority-2

Comment 8 by adamk@chromium.org, Dec 19 2017

Owner: neis@chromium.org
Status: Fixed (was: Started)

Sign in to add a comment