494013 - Interpreter and jit use invalid method for determining code context

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Jason Williams]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10
Build Identifier: 

In the interpreter.cpp and CodeGenLIR.cpp test the pool's domain's base to determine if code is builtin (e.g. pool->domain->base).  These should instead check pool->isBuiltin. In addition the namespace should be emitted when we are not using a default namespace regardless of a change in the code context change.

Reproducible: Always

Comment 1

[Jason Williams]

Attachment: Patch from flashruntime mainline

This is a patch generated from CL#525765 on flashruntime mainline

Comment 2

[Steven Johnson]

Comment on attachment 378662 [details] [diff] [review]
Patch from flashruntime mainline

rerouting to edwin.

what's up with the change in nativegen.py?

Comment 3

[Jason Williams]

The current nativegen.py cleverly restricts access of method and class entries for the native init code to the file that it generates. A command line option was added so that in certain circumstances those definitions can be reached from outside the generated file.

Comment 4

[Steven Johnson]

I'm going to take the liberty of pushing this (since it's been approved) since some current Flash/AIR work is much happier with it in place.

Comment 5

[Steven Johnson]

pushed to redux as changeset:   1978:df1583f8776c

Comment 6

[Steven Johnson]

This bug needs to be re-opened: the patch has uncovered a latent bug, where a ScopeChain that is pointed to by dxnsAddr can be collected, leading to a crash.

Comment 7

[Steven Johnson]

Attachment: Patch: pin ScopeChain with dxnsAddr

Not sure if this is this optimal/correct fix... this adds dxnsAddrOwner to AvmCore so that dxnsAddr values that belong to a ScopeChain can rely on the ScopeChain being pinned. This fixes the crash, but it's not clear to me whether or not this indicates an underlying flaw elsewhere in our dxns save/restore code: I thought functions were responsible for saving/restoring dxnsAddr, but CodegenLIR::emitSetCodeContext will generate emitSetDxns() without any thought to restoring it later. Is this the correct behavior?

Comment 8

[Edwin Smith]

it's correct as designed, but the design has flaws.  the goal is for core->dxnsAddr to be pointing to the active one at the point any builtin code looks at it.  CodegenLIR does this by setting it before calls that could go into such builtin code.  Interpreter does it by saving/restoring in the method prolog boundaries.

It has come to light that these dueling strategies need to be fixed; cleanest least fragile fix is for jit'd code to save/restore dxnsAddr in its prolog/epilog, just like the interpreter.

(same story for codeContext).

The new pinning code seems very heavy.  to pin something, it should be sufficient to write its address to the stack; then it will be seen by the GC.  I dont think we need the new field in core, or the new save/restore logic, just for pinning.

Comment 9

[Edwin Smith]

for pinning, here's a sketch:

in interpreter, add a scope field in aux_memory, then in the prolog just:

   aux_memory.scope = scope; // pin the scope object, dxnsAddr points into it

and in the jit:

   prolog:
      pinnedScope = insAlloc(sizeof(Scope*));
      lirout->insStore( /* pinnedScope = scope */ );
   epilog:
      Ins(LIR_live, pinnedScope);

Comment 10

[Steven Johnson]

(In reply to comment #8)
> It has come to light that these dueling strategies need to be fixed; cleanest
> least fragile fix is for jit'd code to save/restore dxnsAddr in its
> prolog/epilog, just like the interpreter.
> (same story for codeContext).

indeed, that explains a lot. think this constitutes an outright bug or merely a backlog item?

> The new pinning code seems very heavy.  to pin something, it should be
> sufficient to write its address to the stack; then it will be seen by the GC. 

Ah yes, a better/simpler idea. I'll try that and offer a new patch.

Comment 11

[Steven Johnson]

It appears there is a different underlying bug here -- Interpreter and LIR take different approaches to maintaining dxns (and codeContext), which can cause a "stale" dxns to remain active longer than it should. Opening new bug https://bugzilla.mozilla.org/show_bug.cgi?id=496473 to track this.

Comment 12

[Steven Johnson]

fyi, this patch definitely isn't ready for Flash/AIR, as they assume EnterCodeContext can be used when there is no currentMethodFrame. I'll work on a revised patch.

Comment 13

[Edwin Smith]

Lets add some docs for EnterCodeContext, specifically what the expected contract is.  sounds like it intends to mimic a full MethodFrame?  If that's the case then the dxns logic might need adjusting (the other fields won't be valid)

Comment 14

[Steven Johnson]

(In reply to comment #13)
> Lets add some docs for EnterCodeContext, specifically what the expected
> contract is.  sounds like it intends to mimic a full MethodFrame?  If that's
> the case then the dxns logic might need adjusting (the other fields won't be
> valid)

this comment (and the one prior) really apply to bug 496473, moving comment there

Comment 15

[Steven Johnson]

I think this bug is now defunct and should be closed as obsolete

Comment 16

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.