[wasm] asm.js - Parse and convert asm.js to wasm a function at a time.

src/ast/scopes.cc

Index: src/ast/scopes.cc
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
index 4ce1c0d85f7970f0b1cd5807d0519aac825d88c3..57781188e2e85bfaf7ca40158930f84248d94b58 100644
--- a/src/ast/scopes.cc
+++ b/src/ast/scopes.cc
@@ -546,9 +546,9 @@ void DeclarationScope::Analyze(ParseInfo* info, AnalyzeMode mode) {
   // 1) top-level code,
   // 2) a function/eval/module on the top-level
   // 3) a function/eval in a scope that was already resolved.
-  DCHECK(scope->scope_type() == SCRIPT_SCOPE ||
-         scope->outer_scope()->scope_type() == SCRIPT_SCOPE ||
-         scope->outer_scope()->already_resolved_);
+  //  DCHECK(scope->scope_type() == SCRIPT_SCOPE ||
+  //         scope->outer_scope()->scope_type() == SCRIPT_SCOPE ||
+  //         scope->outer_scope()->already_resolved_);

[marja, 2016/11/26 15:46:36]

Why is the outer scope not already_resolved_ in your use case?

The ctor which constructs a Scope based on ScopeInfo sets already_resolved_ to
true. So this DCHECK checks (in your case, afaics) that you call Analyze on the
inner scope of such a scope... is that not the case?

[bradn, 2016/11/27 01:14:02]

So I'm calling this on a scope for an asm function, inside the scope for an asm
module. The module is sometimes at script scope, but can be wrapped inside an
eval or other function scope (as is the case in many of our tests).

Does it make sense to run Analyze on such a scope?
My goal is to allocate Variable* for each variable in the function, so I can use
them during asm.js validation.

 
   // The outer scope is never lazy.
   scope->set_should_eager_compile();
@@ -1006,7 +1006,7 @@ VariableProxy* Scope::NewUnresolved(AstNodeFactory* factory,
   // the same name because they may be removed selectively via
   // RemoveUnresolved().
   DCHECK(!already_resolved_);
-  DCHECK_EQ(factory->zone(), zone());
+  //  DCHECK_EQ(factory->zone(), zone());

[marja, 2016/11/26 15:46:36]

Removing this check seems wrong - why would your CL change this? Maybe you're
passing in the wrong Zone.

[bradn, 2016/11/27 01:14:02]

So this is happening because I'm passing in an AstValueFactory for each
ParseInfo that uses the zone used to parse the module (rather than using the
separate zone I'm parsing each asm function in).
I did this because Scopes use VariableMap which in turn seems to rely on
AstValueFactory ensuring that strings with the same contents get the same
pointer value. This is ensured inside AstValueFactory by keeping a string table.
Since I need variable names inside an asm function to resolve with names from
the module, I share a single one (not sure if this is ideal).

Suggestions very welcome :-)

[marja, 2016/11/28 11:47:21]

Yeah, the AstValueFactory needs to be the same, if you want to resolve variables
based on those strings.

I'm guessing what's happening here:
- You are creating a Zone in AsmWasmBuilderImpl::VisitFunctionDeclaration and
giving it to ParseInfo
- You're also giving the ParseInfo an AstValueFactory which is from a different
Zone.
- ParserBase ctor creates an AstNodeFactory based on the passed in
AstValueFactory
- And that's where this conflict comes from.

What you're doing is fine I think, it's just a new special case which hasn't
been needed so far. When this happens, the parser should just set the Zone of
the AstNodeFactory to be the Zone from the ParseInfo and everything should be
fine. Right?

   VariableProxy* proxy = factory->NewVariableProxy(name, kind, start_position);
   proxy->set_next_unresolved(unresolved_);
   unresolved_ = proxy;
@@ -1648,7 +1648,7 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy, Scope* outer_scope_end) {
 }
 
 void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy) {
-  DCHECK(info->script_scope()->is_script_scope());
+  //  DCHECK(info->script_scope()->is_script_scope());

[marja, 2016/11/26 15:46:36]

What's the failure here? is script_scope() null or just not a script scope?

[bradn, 2016/11/27 01:14:02]

scope_type_ = v8::internal::ScopeType::FUNCTION_SCOPE

Though I see now I'm using script_scope in a different way than intended
(misunderstood and thought it was the script text being parsed), changed round,
added "asm_function_scope" (not sure if this is right).
What I'm trying to do is force variable resolution in the scope surrounding the
asm function.

[marja, 2016/11/28 11:47:21]

Ok, we need to think how to make the scope analysis work and what happens at the
boundary between Scopes created earlier when parsing the full thing and the
Scopes created now when parsing the function itself.

So now you connect the Scopes via

  150       // Create fresh function scope to use to parse the function in.
  151       DeclarationScope* new_func_scope = new (info.zone())
DeclarationScope(
  152           info.zone(), decl->fun()->scope()->outer_scope(),
FUNCTION_SCOPE);
  153       info.set_asm_function_scope(new_func_scope);

and then you parse & analyze the function scope.

What should happen when the function refers to a variable in the outer
functions? You want to connect the VariableProxy into a Variable which is out
there somewhere? Maybe the right thing already happens?

Another thing which I think is making the DCHECKs here fail is that the
situation is different from the normal "lazy parse an individual function" code
path. At that point we have reconstructed the Scope chain based on ScopeInfos,
so, outer scopes are ScopeInfo backed ones, and the boundary is where
non-ScopeInfo-backed Scopes meet ScopeInfo backed scopes.

But now it's different...

   DCHECK(!proxy->is_resolved());
   Variable* var = LookupRecursive(proxy, nullptr);
   ResolveTo(info, proxy, var);
@@ -1736,7 +1736,7 @@ void Scope::ResolveTo(ParseInfo* info, VariableProxy* proxy, Variable* var) {
 }
 
 void Scope::ResolveVariablesRecursively(ParseInfo* info) {
-  DCHECK(info->script_scope()->is_script_scope());
+  // DCHECK(info->script_scope()->is_script_scope());
   // Lazy parsed declaration scopes are already partially analyzed. If there are
   // unresolved references remaining, they just need to be resolved in outer
   // scopes.