534279 - nativegen.py should generate both direct and indirect thunks, switched by #ifdef

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Edwin Smith]

There already is a AVMFEATURE_INDIRECT_NATIVE_THUNKS that controls thunks, but it has to be in sync with the --directthunks flag passed to nativegen.py.  This leads to build script hair, and different copies of builtin.cpp/h (etc) depending on the commandline flag.

moreover, we keep finding bugs where C++ native method signatures don't match the AS3 declarations.  With --directthunks, these become C++ compile time errors.

idea: lets have nativegen.py generate both configurations at the same time, inside a giant #ifdef .. #else .. #endif.  Then, its easy for the feature to be dictated by a choice of platform/config, and also easy to enable/disable.

as a followup task, i think DEBUGGER builds should use direct thunks; the code size isn't an issue, they should be slightly faster, and when profiling in shark or vtune or oprofile, the function names make sense.  (but we can debate this decision).

Comment 1

[Steven Johnson]

(In reply to comment #0)
> I think DEBUGGER builds should use direct thunks; the code
> size isn't an issue, they should be slightly faster, and when profiling in
> shark or vtune or oprofile, the function names make sense.  (but we can debate
> this decision).

+1 -- cc'ing some AIR guys since they are all-Debugger, all-the-time, but I very much doubt the % code size difference will be notable there.

Comment 2

[Christopher A. Brichford]

(In reply to comment #1)
> (In reply to comment #0)
> > I think DEBUGGER builds should use direct thunks; the code
> > size isn't an issue, they should be slightly faster, and when profiling in
> > shark or vtune or oprofile, the function names make sense.  (but we can debate
> > this decision).
> 
> +1 -- cc'ing some AIR guys since they are all-Debugger, all-the-time, but I
> very much doubt the % code size difference will be notable there.

This could very well be a good idea, but should be tracked as a separate issue.

Comment 3

[Christopher A. Brichford]

What platforms or targets turn on direct thunks today?

Comment 4

[Christopher A. Brichford]

(In reply to comment #0)
> There already is a AVMFEATURE_INDIRECT_NATIVE_THUNKS that controls thunks, but
> it has to be in sync with the --directthunks flag passed to nativegen.py.  This
> leads to build script hair, and different copies of builtin.cpp/h (etc)
> depending on the commandline flag.
> 
> moreover, we keep finding bugs where C++ native method signatures don't match
> the AS3 declarations.  With --directthunks, these become C++ compile time
> errors.
> 
> idea: lets have nativegen.py generate both configurations at the same time,
> inside a giant #ifdef .. #else .. #endif.  Then, its easy for the feature to be
> dictated by a choice of platform/config, and also easy to enable/disable.
> 
This all sounds good. While we are in there we should look at other places command line flags are dependent on pre-processor settings.

Comment 5

[Edwin Smith]

(In reply to comment #3)
> What platforms or targets turn on direct thunks today?

None do, currently.  I use it for profiling because direct thunk names are readable, but I also keep finding signature mismatches that way.

Comment 6

[Christopher A. Brichford]

Could we always generate function that contains calls to all the methods with the correct signatures?  This function would never be called, but would fail to compile if a function signature was incorrect.  Since this function is never called, it should always be dead code stripped.  On platforms with broken dead code stripping, we can put this generated function inside of #ifdef DEBUG.

Comment 7

[Steven Johnson]

(In reply to comment #6)
> Could we always generate function that contains calls to all the methods with
> the correct signatures? 

This has been proposed; the gotcha is determining which compiler configs will complain about unreachable-code and/or expression-always-false, even in debug-only builds.

Comment 8

[Christopher A. Brichford]

(In reply to comment #7)
> (In reply to comment #6)
> > Could we always generate function that contains calls to all the methods with
> > the correct signatures? 
> 
> This has been proposed; the gotcha is determining which compiler configs will
> complain about unreachable-code and/or expression-always-false, even in
> debug-only builds.

Instead of generating calls to native methods, we can have the generated function fills in a giant struct that has a pointer to member function of the correct type.   The assignment of the struct's members should not require a cast.  Pointer to struct should be passed in as an argument, generated function should be marked as externally callable.

Comment 9

[Edwin Smith]

If all we need is the sanity check, then we don't need a cross-compiler portable solution.

On some mainstream platform (mac desktop probably), just run nativegen.py with direct thunks, and just compile the single output CPP file to check for compile errors.  dont even need to link, and it just has to happen when nativegen.py runs.

Comment 10

[Christopher A. Brichford]

(In reply to comment #9)
> If all we need is the sanity check, then we don't need a cross-compiler
> portable solution.
> 
> On some mainstream platform (mac desktop probably), just run nativegen.py with
> direct thunks, and just compile the single output CPP file to check for compile
> errors.  dont even need to link, and it just has to happen when nativegen.py
> runs.

We run nativegen.py as part of every clean build.  We don't check its output into version control.  If we can find these problems automatically at compile time we should do so.  These are real problems that could take a significant amount of time for a non-vm engineer to track down.

I agree that we don't need a portable solution.  It would be sufficient for now to just support gcc ( darwin and linux ) and msvc ( win32, win64, and arm ).  Most changes will run through a debug build on one of those configurations.

Comment 11

[Edwin Smith]

Attachment: nativegen.py generates thunks both ways, guarded by feature ifdef

Also in this patch:

* rename AVMPLUS_INDIRECT_NATIVE_THUNKS -> VMCFG_INDIRECT_NATIVE_THUNKS
* remove nativegen.py --directthunks switch

Comment 12

[Edwin Smith]

Attachment: in the shell, make DEBUGGER builds use direct thunks by default

Comment 13

[Edwin Smith]

Comment on attachment 421634 [details] [diff] [review]
nativegen.py generates thunks both ways, guarded by feature ifdef

pushed http://hg.mozilla.org/tamarin-redux/rev/fb976dc5c812

Comment 14

[Edwin Smith]

Comment on attachment 421639 [details] [diff] [review]
in the shell, make DEBUGGER builds use direct thunks by default

pushed pushed  http://hg.mozilla.org/tamarin-redux/rev/2bd0fde420aa

Comment 15

[Chris Peyer]

Engineering work item.  Marking as verified.