561366 - Only capture one ScopeTypeChain for every traits

Status: RESOLVED FIXED

(Tamarin Graveyard :: Verifier, defect, P3)

Description

[Edwin Smith]

It is possible for OP_newclass to reference the same class twice, so we need to only capture the scope chain once and verify its compatible, like we do for OP_newfunction.

Comment 1

[Edwin Smith]

Attachment: Avoid capturing duplicate scope chains, fail if any are different.

Comment 2

[Steven Johnson]

Comment on attachment 441045 [details] [diff] [review]
Avoid capturing duplicate scope chains, fail if any are different.

This looks fine, but where are we seeing this and what is the symptom being fixed?

Comment 3

[Edwin Smith]

Ran into it last year in the jit cache work (bug 486699) and last night working on OSR (539094), both of which run the verifier more than once on the same function.  But, its also a potential problem if OP_newclass<N> shows up more than once in a single ABC, just like we've dealt with for OP_newfunction.

We need a test case that does precisely that (tests what happens with OP_newclass<N> being encountered more than once).  testing repeated verifier runs isn't possible yet.

Comment 4

[Edwin Smith]

Widening the scope of the bug; a similar problem exists for OP_newactivation.  Encountering this bytecode more than once causes more than one identical ScopeTypeChain to be captured.

The problem is more benign in this case; the opcode does not specify a method, so the scopes will always match.  The extra ones are just garbage, but still create problems if the verifier's side effects are executed twice as a result of lazy jit invocation.

Also note that for OP_newactivation, creating and discarding extra scope chains creates unnecessary GC pressure.  This happens in any method with an activation scope since every instruction is visited at least twice during verify/jit.

Comment 5

[Edwin Smith]

Attachment: Only capture activation traits the first time we see OP_newactivation

only requesting feedback, because:

* patch needs rebasing to tip
* scope-capture work should move to ScopeWriter

This patch adds a hasScope() convenience function to ScopeOrTraits, and removes resolveActivationScope since it will be refactored within the verifier.

Comment 6

[Steven Johnson]

Comment on attachment 441673 [details] [diff] [review]
Only capture activation traits the first time we see OP_newactivation

No red flags here.

Comment 7

[Edwin Smith]

Comment on attachment 441045 [details] [diff] [review]
Avoid capturing duplicate scope chains, fail if any are different.

OP_newclass fix pushed
http://hg.mozilla.org/tamarin-redux/rev/9dc6bf9db472

Comment 8

[Edwin Smith]

Attachment: (v2) Only capture activation traits once, the first time a method is verified

Changes since last patch:

* rebased
* factored code into ScopeWriter
* leave activationTraits->resolveSignatures(toplevel) call in the main verify block, where it will be invoked unconditionally when we see OP_newactivation in phase 1.  This matches existing behavior; I don't intend to change the point where resolveSignatures is called.

Comment 9

[Edwin Smith]

Comment on attachment 451055 [details] [diff] [review]
(v2) Only capture activation traits once, the first time a method is verified

TR: http://hg.mozilla.org/tamarin-redux/rev/bfcef5ed94df