634655 - nativegen.py should pass the current code context to native methods that need the current code context

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

ChrisB says:

"When the VM calls from C++ into AS3, one of the arguments passed in
should be code context. Maybe methods that need the code context should declare they need it in some way."

Comment 1

[Christopher A. Brichford]

The C++ code has to get a CC from somewhere.  Right now there a three
basic ways to do it:

1. Get the current one off of AvmCore
2. Make a new one
3. Look up one in the player by URL.

I think we should get rid of case #1.  The C++ code would never be
allowed to get the CC from the AvmCore.  Instead the C++ code would get
the CC from avmplus when a native method is called via the argument list
of the native method.

This will make it harder for code buried twenty call frame deep in the
player to decide to get the CC from AvmCore and make some decision on
it.  Instead the CC will have to be passed down which should make the
much easier to understand, since the existence of a CC in a C++ method
signature will imply one of two things:
a.  The C++ code might do a security check
b.  The C++ code might call back into AS3 on the same call stack or a
future call stack.

Comment 2

[Steven Johnson]

Ah, so the idea is that if a method needs to CC, it must explicitly request that it be passed to it?

Comment 3

[Jeff Mott]

Why not always?  Is there a cost associated?  Just want to understand the benefit of forcing people to set a flag.

Comment 4

[Christopher A. Brichford]

(In reply to comment #2)
> Ah, so the idea is that if a method needs to CC, it must explicitly request
> that it be passed to it?

Yes.

Fixing this bug will avmplus to remove the "CodeContext* AvmCore::codeContext()
const;" from the public avmplus API.

Comment 5

[Christopher A. Brichford]

(In reply to comment #3)
> Why not always?  Is there a cost associated?  Just want to understand the
> benefit of forcing people to set a flag.

Always would be fine.  It is an extra push on the stack for evey native method call, but maybe the simplicity is worth the cost.

Comment 6

[Edwin Smith]

It would be good to check with Kpalacz about the current state of TLS and ExecEnv; CodeContext is another way of saying "call stack" which is another way of saying "thread local information".  Provided the api is tight, only one such "context" parameter should be passed to a native method.

out of scope for this bug, maybe, but I seem to recall various native methods jumping through hoops because they aren't passed the MethodEnv* for the native method.  CC (or EE), and ME each provide interesting context to the called method.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> It would be good to check with Kpalacz about the current state of TLS and
> ExecEnv; CodeContext is another way of saying "call stack" which is another way
> of saying "thread local information".  Provided the api is tight, only one such
> "context" parameter should be passed to a native method.

FYI, that work is tracked in Bug 619858

Comment 8

[Krzysztof Palacz]

I ran into a very similar problem in bug 619858: ExecEnv* is always available as an incoming argument to the the generated C++ caller of a VM method, but there is no way to specify that this ExecEnv* should be passed into  the VM method, so it has to fumble around to find ExecEnv* if it needs it (or resort to platform TLS, which is not for free).
If ExecEnv* were to land, codeContext() would belong in it (it wouldn't store a CodeContext, but neither does AvmCore).

Comment 9

[Rick Reitmaier]

+1 would be good to a clean 'API' for the pieces that we expose outside the VM; whether we use TLS or not; e.g: get[Function,Class]CodeContext().

Comment 10

[Dan Smith]

Tentatively marking as 'Future' item and to be reviewed as part of upcoming ANI work.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.