Get rid of PropagateScopeInfo, update asm_function in set_asm_module instead.

Get rid of PropagateScopeInfo, update asm_function in set_asm_module instead.

The last user was propagating asm_module_ to inner function scopes as asm_function_. asm_function_ is already set upon scope creation when the outer scope IsAsmModule(). With default parameter it's possible that inner scopes are created before set_asm_module() is called. To keep current behavior we'll eagerly mark inner scopes as asm_function_ upon set_asm_module().

There's only one special case that used to be marked asm_function_ which is now no longer marked as such: asm functions in block scopes that are 'Finalized'. PropagateScopeInfo used to mark them as asm_function_ as well, whereas the new version would not mark them upon construction of the inner scope.

I presume both above cornercases aren't actually intended valid asm.js use-cases anyway. The second we can now easily identify in the verifier and mark as invalid asm, since we'll have an asm module with non-asm-function inner functions. If we want to disallow the first, we can also not mark them as asm_functions_ (by removing the loop I added in set_asm_module), which will reveal this structure to the validator.

BUG=v8:5209
Committed: https://crrev.com/ce9e773828fdd946eee243c1229f914b548df82c
Cr-Commit-Position: refs/heads/master@{#39039}

[Toon Verwaest, 2016-08-23 12:43:41]

verwaest@chromium.org changed reviewers:
+ titzer@chromium.org

[Toon Verwaest, 2016-08-23 12:43:42]

ptal

[titzer, 2016-08-23 13:08:59]

https://codereview.chromium.org/2270743003/diff/1/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2270743003/diff/1/src/ast/scopes.h#newcode684
src/ast/scopes.h:684: void set_asm_module() {
Move to cc file? Not performance sensitive, I guess.

https://codereview.chromium.org/2270743003/diff/1/src/ast/scopes.h#newcode687
src/ast/scopes.h:687: for (Scope* inner = inner_scope_; inner != nullptr;
You could, e.g. recurse through block scopes as well, if you wanted to catch the
special case of functions within block scopes, but it is probably better that
the validator checks that there are no such functions.

[Toon Verwaest, 2016-08-23 13:17:51]

Addressed comment.

https://codereview.chromium.org/2270743003/diff/1/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2270743003/diff/1/src/ast/scopes.h#newcode687
src/ast/scopes.h:687: for (Scope* inner = inner_scope_; inner != nullptr;
On 2016/08/23 13:08:59, titzer wrote:
> You could, e.g. recurse through block scopes as well, if you wanted to catch
the
> special case of functions within block scopes, but it is probably better that
> the validator checks that there are no such functions.

The block scopes I'm talking about aren't in this list. They are the cases where
in DeclarationScope::DeclarationScope we set asm_function_ to true. It could be
that they currently have an outer block scope, but that we Finalize that block
scope at some point. We could extend FinalizeBlockScope to set asm_function_ to
true when it's removing a block scope in between an asm module and a function
scope ... but that's slower and probably not what we want anyway?

[Toon Verwaest, 2016-08-31 08:51:03]

ping

[titzer, 2016-08-31 08:51:54]

lgtm

[Toon Verwaest, 2016-08-31 09:01:27]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-31 09:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2270743003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-31 09:01:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 09:35:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-31 09:36:24]

Description was changed from

==========
Get rid of PropagateScopeInfo, update asm_function in set_asm_module instead.

The last user was propagating asm_module_ to inner function scopes as
asm_function_. asm_function_ is already set upon scope creation when the outer
scope IsAsmModule(). With default parameter it's possible that inner scopes are
created before set_asm_module() is called. To keep current behavior we'll
eagerly mark inner scopes as asm_function_ upon set_asm_module().

There's only one special case that used to be marked asm_function_ which is now
no longer marked as such: asm functions in block scopes that are 'Finalized'.
PropagateScopeInfo used to mark them as asm_function_ as well, whereas the new
version would not mark them upon construction of the inner scope.

I presume both above cornercases aren't actually intended valid asm.js use-cases
anyway. The second we can now easily identify in the verifier and mark as
invalid asm, since we'll have an asm module with non-asm-function inner
functions. If we want to disallow the first, we can also not mark them as
asm_functions_ (by removing the loop I added in set_asm_module), which will
reveal this structure to the validator.

BUG=v8:5209
==========

to

==========
Get rid of PropagateScopeInfo, update asm_function in set_asm_module instead.

The last user was propagating asm_module_ to inner function scopes as
asm_function_. asm_function_ is already set upon scope creation when the outer
scope IsAsmModule(). With default parameter it's possible that inner scopes are
created before set_asm_module() is called. To keep current behavior we'll
eagerly mark inner scopes as asm_function_ upon set_asm_module().

There's only one special case that used to be marked asm_function_ which is now
no longer marked as such: asm functions in block scopes that are 'Finalized'.
PropagateScopeInfo used to mark them as asm_function_ as well, whereas the new
version would not mark them upon construction of the inner scope.

I presume both above cornercases aren't actually intended valid asm.js use-cases
anyway. The second we can now easily identify in the verifier and mark as
invalid asm, since we'll have an asm module with non-asm-function inner
functions. If we want to disallow the first, we can also not mark them as
asm_functions_ (by removing the loop I added in set_asm_module), which will
reveal this structure to the validator.

BUG=v8:5209

Committed: https://crrev.com/ce9e773828fdd946eee243c1229f914b548df82c
Cr-Commit-Position: refs/heads/master@{#39039}
==========

[commit-bot: I haz the power, 2016-08-31 09:36:25]

Patchset 3 (id:??) landed as
https://crrev.com/ce9e773828fdd946eee243c1229f914b548df82c
Cr-Commit-Position: refs/heads/master@{#39039}