If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Status

Tamarin
Tracing Virtual Machine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
In TC host's called into the VM using a handful of entry points:

ScriptObject::call
MethodEnv::coerceEnter
Toplevel::constructObject

We want to eradicate this practice and have one mechanism for calling in.  Further more these entry points will be restriced to static method that only take primitives as arguments.  The idea is all the VM state be only stored in the VM.  This may be overly restrictive but we'd rather be overly closed and loosen up than start wide open and get stuck there (current situation).   

So a entry point is declared in AS and GlobalOptimizer generates the necessary glue code to call into the interpreter.
(Assignee)

Comment 1

10 years ago
Created attachment 306036 [details] [diff] [review]
prototype 


This shows the glue hand coded at the top of avmshell.cpp, this is what G.O. will generate.  Mostly looking an okay that everything interpMethodEnv is doing is okay, is okay to pull the MethodEnv out of the AbcEnv like that or should I go through the TraitsEnv?  This seemed the most straightforward approach.

Note that this doesn't work, still working out the kicks with how I'm passing the args to forth.
Assignee: nobody → treilly
Status: NEW → ASSIGNED
Attachment #306036 - Flags: review?(stejohns)

Comment 2

10 years ago
pulling  the ME our of AbcEnv is ok in this case I think.

You're restarting the sp at stack, won't it step on existing stack items? ditto for rp.

Are the Atom args to avmplus_System_private_callin just to allow for null and undefined? if so we should come up with another way so we don't have to expose Atom if possible.

nit: argc should be uint, not int

probably good to have Edwin look at this too
(Assignee)

Comment 3

10 years ago
I'm not concerning myself with re-entrancy at the moment, basically its assumed anything using this callin approach is a root entry point, I suspect we'll get a lot of mileage out of that simplification (ie I think we can get pretty far w/o re-entrancy support).

Hmm, yeah supporting Atom probably isn't necessary, I'll remove it.  Where I'm struggling is with the stack setup, I'm replacing w_callenv, with w_callin and that will be a stripped down version of callenv, basically I think I can avoid all the coercion and stack prep stuff by getting the stack right in interpMethodEnv.  Let me know what you think of that, I haven't gotten my head around how the forth stack setup needs to work.  So far I have

EXTERN: w_callin ( obj args argc env -- result )
	PUSHFRAME          ( obj args argc env )
	methodinfopc SETPC	( obj args argc )
	ENV getforthimpl	( obj args argc wimpl )
	EXECUTE				( result )
	POPFRAME ;

Comment 4

10 years ago
yeah, for a C->ABC call it should be fine to require the stack correct ahead of time, I think it should be possible to avoid the coercion. though the coercion probably won't make much difference.

re: forth stack setup, yeah, it's messy. you may need to wrap the execute in enterabc/exitabc (or is that encapsulated inside the called code? not sure)

re: re-entrancy, got it, that would make life much simpler. (probably be good to add some assertions checking for that.)
(Assignee)

Comment 5

10 years ago
Created attachment 306318 [details] [diff] [review]
working prototype


This is working, now I'm off to add the stub generation to G.O. everything seem kosher?
Attachment #306036 - Attachment is obsolete: true
Attachment #306318 - Flags: review?(edwsmith)
Attachment #306036 - Flags: review?(stejohns)
(Assignee)

Comment 6

10 years ago
Created attachment 306361 [details] [diff] [review]
GlobalOptimizer stub generation diff

This is the asc part.
Attachment #306361 - Flags: review?(edwsmith)
(Assignee)

Updated

10 years ago
Blocks: 420195
(Assignee)

Comment 7

10 years ago
Example:

class Foo
{
   static private function callfromC(arg:int):bool
   { 
      trace("abc, easy as " + arg);
      return true;
   }
}

From C:

bool b = avmplus_Foo_private_callfromC(123);

The compiler generates avmplus_Foo_private_callfromC in the builtin cpp file and a decl in the .h file.
(Assignee)

Comment 8

10 years ago
Oops there supposed to be a [native] metadata tag on the function

Comment 9

10 years ago
This sounds very useful.  However, it only supports a "static" model and some facility must be provided for a more dynamic model (a-la ScriptObject::call and the like) - for example, when the name of the function to call is provided at runtime.  Assuming we had catchalls implemented, I guess this would still work - we can pass the name of the method-to-call/property-to-set-get to script code, which implements the dynamic model.  But IIUC, catchalls aren't close.  I expect I'm reading too much into ScriptObject::call being listed above though...

Comment 10

10 years ago
Comment on attachment 306318 [details] [diff] [review]
working prototype

I would prefer to see this require a toplevel function as the entry point rather than a static class method.  for one thing, it avoids the overhead of a class when its not needed.  it also eliminates the need for class_id, just need method_id.

i will pull the patch and see what the changes would look like, before i push to hard on the idea.

Comment 11

10 years ago
(In reply to comment #9)
> This sounds very useful.  However, it only supports a "static" model and some
> facility must be provided for a more dynamic model (a-la ScriptObject::call and
> the like) - for example, when the name of the function to call is provided at
> runtime.  Assuming we had catchalls implemented, I guess this would still work
> - we can pass the name of the method-to-call/property-to-set-get to script
> code, which implements the dynamic model.  But IIUC, catchalls aren't close.  I
> expect I'm reading too much into ScriptObject::call being listed above
> though...
> 

The idea is to keep the C->ABC transition as minimal as possible, any other niceities such as a dynamic invocation point, a class, complex parameters, and/or exception handling, can be done in AS3.

I would even go so far as to say if we think we need a "standard" embedder-friendly api, with all the bells and whistles, that even that API should be done in AS3.

(Assignee)

Comment 12

10 years ago
(In reply to comment #10)
> (From update of attachment 306318 [details] [diff] [review])
> I would prefer to see this require a toplevel function as the entry point
> rather than a static class method.  for one thing, it avoids the overhead of a
> class when its not needed.  it also eliminates the need for class_id, just need
> method_id.
> 
> i will pull the patch and see what the changes would look like, before i push
> to hard on the idea.

I wasn't sure how to get the right global object to be the reciever for that case.  Also I thought the ability to declare them in a class was a better fit for the player, maybe we could support both?

Comment 13

10 years ago
The class objects are sitting in the global object, so whatever youre doing now is going through the desired global object, to get the class object.  and, initializing it as a side effect, which is an implicit callin.  so, one less load should do it.  Also youre using a method_id for the method to call, just as easy to get the method_id for the global function.  code would look like this:

package
{
  [native-entry-powerjam]
  function entrypoint() {}
}

I'm playing with this as we speak, let me just see what the code looks like.  I dont think this needs to be user friendly at all, all the user-friendly abstractions you want could be done in as3.

my desire is to only support global methods but supporting both, or just class-static methods, could be a compromise if there is some major pain point with global functions.

Comment 14

10 years ago
Comment on attachment 306318 [details] [diff] [review]
working prototype

for the hostClass array on toplevel to work right, all the classes that have entry points have to be in the same abc file.  otherwise class_id's will recycle and collide.

Comment 15

10 years ago
Comment on attachment 306318 [details] [diff] [review]
working prototype

scalpel in the patient?  

-		const bool t = allowTracing ? tracing() : true;
+		const bool t = true;//allowTracing ? tracing() : true;

You're stowing away the CC object in newclass(), which means someone else had to call some other entry point to initialize the global object first.  

the proof of concept would be to wrap the code in shell/main.as in a function (or class) -- make sure we can bootstrap the vm with an entry point callin

Comment 16

10 years ago
Comment on attachment 306361 [details] [diff] [review]
GlobalOptimizer stub generation diff

code looks fine, but if you can macro-ize any of the boilerplate that goes into a callin method, it'll save you a lot of headache later.  i.e. the output files (.cpp, .h) use macros where possible, for boilerplate code, so mostly if the code has to change, the macros are changed, instead of changing G.O.
Attachment #306361 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 17

10 years ago
yeah I was thinking at some point we may want to optimize it further so that the args are written to the real stack but as long as the mechanism is fixed the generated code isn't really important as its all hidden from the host.  But yeah, point taken, keeping the native knowledge in the asc to a minimum would be good, in particular I don't like how the G.O. knows so much about Boxes.

Comment 18

10 years ago
Hey Tommy, one thing we should probably work on while we're at it is providing a well-defined interface for native methods; right now they need to include internal headers and know about the guts of things like ScriptObject and Box, plus, they might need to be able to call getprop or getuintprop on an array argument passed to them... we should really create a "public" api that can insulate TT clients from the internals of TT.
(Assignee)

Comment 19

10 years ago
yeah Ed and I discussed this, the idea is that the vm will generate an init method and vm startup looks like (for the shell):

avm vm = shell_init(); // init pools, create DominEnv, Toplevel etc
avmplus_main(vm);

both of these methods are generated by G.O.  "vm" is an opaque pointer to an AbcEnv.  ScriptObjectp would need to be exposed for callout methods self and classself args but it can probably just be declared and not defined (ie another opaque pointer).

Once I get all this baked I'll have a go at creating a "embedding" API.  I'm thinking avm.h (I love short names!)

Current list:

space.h
avmplusTypes.h
String.h
NativeFunction.h
Debugger stuff

There's a couple things in the way of making this work:

- ConsoleOutputStream crap, just goes away , avmshell wires System._write to stdout
- interupt handling
- stack overflow handling

Another idea that came up was to merge the builtin abcs so there's only one (probably a couple hundred bytes of savings there)

Comment 20

10 years ago
I would love to see Console/PrintStream gone from the vm code, if anything maybe its embedder can provide a FILE*.  reeks of commandline, but even for fancy debug output routing how hard could it be?

what really needs to be done for stack overflow handling?  the vm throws a stack overflow exception, which can be caught in a userdefined callin.  
(Assignee)

Comment 21

10 years ago
okay, so far so good, what about CodeContext, shouldn't DomainEnv* suffice for its purposes?  The player subclasses it to create a ScriptPlayer*/SecurityContext*/DomainEnv* tuple.  It appears to need to get from the executing code to the swf (ScriptPlayer*).  

Comment 22

10 years ago
I have a few concerns that may or may not be valid:

1) can we avoid having the global function pollute the global namespace? Or, the namespace for the global function avoid polluting the set of namespaces? Or, are there namespaces which are reserved for ES4 compilers?

2) Is declaring the private function "private" enough to prevent script callers from calling it? I know we haven't gotten into discussions about security models yet, but I don't want to design in a system which is hard to secure later.

3) I don't understand the comments above about re-entrancy. We're definitely going to need re-entrancy for even simple operations such as a DOM modification firing event listener notifications.

4) assuming that we'll have a few standard callback functions: what would an efficient way to call them with variable number of args? Creating an array for a varags function sounds like a lot of overhead.
(Assignee)

Comment 23

10 years ago
Responding to (In reply to comment #22)

> 1) can we avoid having the global function pollute the global namespace? Or,
> the namespace for the global function avoid polluting the set of namespaces?
> Or, are there namespaces which are reserved for ES4 compilers?

Each .as file gets its own "global" and only public symbols are exported to the rest of the domain, so if you use private then the symbol isn't visible outside the .as file.
 
> 2) Is declaring the private function "private" enough to prevent script callers
> from calling it? I know we haven't gotten into discussions about security
> models yet, but I don't want to design in a system which is hard to secure
> later.

Only script in the same .as file could call it.

> 3) I don't understand the comments above about re-entrancy. We're definitely
> going to need re-entrancy for even simple operations such as a DOM modification
> firing event listener notifications.

Re-entrancy is on the table, no worries.  

> 4) assuming that we'll have a few standard callback functions: what would an
> efficient way to call them with variable number of args? Creating an array for
> a varags function sounds like a lot of overhead.

We haven't thought about variadic callins, how important are they?  We haven't come across a need for them yet although we probably will...

(Assignee)

Comment 24

10 years ago
If we do add variadic call in the interface will probably be (int argc, Box* args) so I wouldn't worry about Array overhead.

Comment 25

10 years ago
The Box* solution is perfect. But yeah, we are going to be doing lots of variadic calls, because we don't know the signatures of IDL methods at compile-time, so we'll be constructing the correct call signatures at runtime.
(Assignee)

Comment 26

10 years ago
Created attachment 307503 [details] [diff] [review]
Take2


Getting closer, highlights:

1) AvmCore subclassing gone, "Shell" class gone and Configuration object introduced
2) vm bootstrap all encompassed in generated shell_init function. 
3) CodeContext/ShellCodeContext gone, thinking is AbcEnv will suffice
4) ConsoleOutputStream moved into core, this probably needs more thinking, like can we have ConsoleOutputStream route output back through System.write so the host can handle it?
5) DomainClass.cpp obviously needs to move into core, not sure how that should look, will take a stab at that next.
6) I cleaned up avmshell.cpp pretty heavy-handidly, I suspect most of this was scapels in the patient.
7) added shell_destroy method because the AvmCore needs to be destroyed.  Thinking about make AvmCore a GC object to make this unnecessary.
Attachment #306318 - Attachment is obsolete: true
Attachment #307503 - Flags: review?(edwsmith)
Attachment #306318 - Flags: review?(edwsmith)

Comment 27

10 years ago
(In reply to comment #22)

> 1) can we avoid having the global function pollute the global namespace? Or,
> the namespace for the global function avoid polluting the set of namespaces?
> Or, are there namespaces which are reserved for ES4 compilers?
> 
> 2) Is declaring the private function "private" enough to prevent script callers
> from calling it? I know we haven't gotten into discussions about security
> models yet, but I don't want to design in a system which is hard to secure
> later.

Adding to Tommy's reply.  With the latest drop, entry points are toplevel functions instead of static methods on classes.  They are in a user defined package, but aren't public.  This makes them "internal" (like java package-private), but not completely private.  AS3 currently lacks syntax to define fully private toplevel names.

all is not lost.  the VM supports fully private toplevel names.  The packager (GlobalOptimizer) that creates the builtin abc's and .cpp and .h files, turns these internal namespaces into private namespaces.   the net effect is like:

package mystuff {
   [native]
    private function main()
}

I would be interested to see if we can go further, and allow this:

package private {
}

but either way you get no name pollution, and referential security.  private names can only be referenced in the same abc where they are defined.  the VM compares them by pointer, whereas non-private namespaces are compared by value (uri).  

Comment 28

10 years ago
echoing Ben Garney, we should rename [native] to [callin] or [entrypoint] or something, since native is already an attribute that means something else.

nevermind about package private {}, its more useful to put entrypoints into packages along with classes they use, and have the packager (GlobalOptimizer) turn |internal| into |private| where necessary.

(Assignee)

Comment 29

10 years ago
Created attachment 307626 [details] [diff] [review]
take 2 for asc changes


Corresponds to Take2 patch, adds scriptId stuff, _init generation etc.  Needs cleaning up.
Attachment #306361 - Attachment is obsolete: true

Comment 30

10 years ago
* (nit) would be nice for the loop: while((bindingCursor =  to be in a helper
method.  is it just to catch compiler bugs?

* the if (! ... everCalled()) check roughly duplicates the |initglobal| forth
word  (which it calls -- so we're basically doing if (!everCalled()) { if
(!everCalled()) { /* init */ }}, if you look at the code flow.

could we refactor w_callin so it takes care of that?   wouldn't even have to
change w_callin's signature, since obj is the global object that everCalled is
checking.    something like:

: w_callin ( obj args argc env -- result )
    OVER 2+ PICK initglobal DROP 
    // current body of w_callin

this means you don't have to call into the interpreter, exit, then call back
in.  you just call w_callin and it takes care of the rest.

* in interpMethodEnv(), it looks like youre pushing the global obj an extra
time, after the args but before the argc. and, pushing the args in reverse
order. i dont see why

* making AvmCore a gc object sounds fine, but is there still usefulness in a vm
shutdown hook, eg for printing stats?  if were going to make it optional and
leave it up to the embedder, maybe it belongs in AS3? i'm ambivalent for the
moment.  
(Assignee)

Comment 31

10 years ago
Created attachment 308716 [details] [diff] [review]
Latest asc patch, abc merging!
Attachment #307626 - Attachment is obsolete: true
Attachment #308716 - Flags: review?(edwsmith)
(Assignee)

Comment 32

10 years ago
Comment on attachment 307503 [details] [diff] [review]
Take2

Cancelling review of this, will ask for review of final patch once the trace tree stuff lands and I rebase to tip).
Attachment #307503 - Flags: review?(edwsmith) → review-
(Assignee)

Updated

10 years ago
Attachment #308716 - Flags: review?(edwsmith)
(Assignee)

Comment 33

10 years ago
Created attachment 310683 [details] [diff] [review]
ASC callin patch, abc merging, callin generation
Attachment #308716 - Attachment is obsolete: true
Attachment #310683 - Flags: review?(edwsmith)
(Assignee)

Comment 34

10 years ago
Created attachment 310684 [details] [diff] [review]
Callin/hosting stuff, should be ready for push (win/linux/mac tested)

Its noisy due to the uint8_t changes
Attachment #307503 - Attachment is obsolete: true
Attachment #310684 - Flags: review?(edwsmith)
(Assignee)

Comment 35

10 years ago
This patch needs more work.  It runs the regression tests and the sunspider benchmarks fine but fails on esc compiling debug.es which TT tip handles fine.   Hmm, esc might be the next frontier in regression tests...
(Assignee)

Updated

10 years ago
Summary: Callin infrastructure → Hosting infrastructure
(Assignee)

Updated

10 years ago
Attachment #310684 - Flags: review?(edwsmith)
(Assignee)

Comment 36

10 years ago
Created attachment 313107 [details] [diff] [review]
Hosting changes and more
Attachment #310684 - Attachment is obsolete: true
Attachment #313107 - Flags: review?(stejohns)

Comment 37

10 years ago
Comment on attachment 313107 [details] [diff] [review]
Hosting changes and more

Looks ok -- can we get documentation (even a rough wiki page) on the new callin/hosting setup soon, though?

Also, in the future maybe we should do the giant type substitutions (byte->uint8_t) as separate patches :-)
Attachment #313107 - Flags: review?(stejohns) → review+

Comment 38

10 years ago
Comment on attachment 310683 [details] [diff] [review]
ASC callin patch, abc merging, callin generation

looks good, but what does this review really mean?  i would assume there is a corresponding review process for asc changes using a JIRA bug and patch before pushing to svn?
Attachment #310683 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 39

10 years ago
I talked to Jono and he said there is no such process so I was gonna just piggy back on our bugzilla process here.  I plan on quoting this bug in my svn submission.  Agreed though for core asc changes a JIRA bug would be better, but since the community of people who even know what GO is is ~3 people I thought it overkill.   

I thought you might have reservations about some of the tweaks I did to try to get GO to do all the verifier poking optimizations that I eventually bailed on, they all look kosher?

Comment 40

10 years ago
(In reply to comment #39)

> I thought you might have reservations about some of the tweaks I did to try to
> get GO to do all the verifier poking optimizations that I eventually bailed on,
> they all look kosher?

They looked pretty benign, as long as all the regressions & sundry pass okay then I don't have any reservations.
(Assignee)

Comment 41

10 years ago
asc changes pushed to svn as revision 1089
(Assignee)

Comment 42

10 years ago
tt changes pushed as revision 331

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.