523823 - shell needs a flag to set version to regression test bug fixes that have been versioned

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P1)

Description

[Dan Schaffer]

currently we have 1 versioned bug where the fix is for flash 10, so flash 9 still exhibits the older behavior.  A shell flag like -version=9 or -version=10 would set the bit mask in PoolObject.cpp to change the behavior.  With the fix we could add regression tests for the versioned bug for both the old behavior and the new behavior.

the versioned bug fix is: https://bugzilla.mozilla.org/show_bug.cgi?id=444630

Comment 1

[Jeff Dyer]

if VMCFG_TEST_API_VERSIONING is defined, then 'avmshell -api N' treats user code as if it was version N, where N is one of the player version ids (660-670). Will that do the trick?

Comment 2

[Dan Schaffer]

Attachment: enable -api parameter in shell

patch to add -api parameter to shell.  when building shell do:
$ ../configure.py --enable-shell --enable-api-versioning
...
$ shell/avmshell --usage
...
[-api N] execute ABCs as version N (see api-versions.h)

Comment 3

[Chris Peyer]

Also need a way to easily enable/disable the bugzilla bitfield bugs from the shell (preferable over having to compile the value in).

At the moment, then bit-flag for bug 444630 is set to 0 in the shell, with no easy way to turn it on / off.

From Jim Corbetts checkin notes:

Summary:Implemented the avmplus bug fix versioning. (Different than the API versioning)
This is so we can fix bugs in the avmplus without any references to the Flash Player (since it's open source and all) Basically, we set a bitField on the pool object (which Erik T. added) with contants that represent the bugzilla bugs that we want to fix. This is done when we're initializing the built-ins and any laoded files pools.
The code then checks the bitfield to decide if it wants to execute the old way or the fixed way.
** If you have an avmplus bug that needs version checking, add an entry to the enum in PoolObject.h (look for kbug) then add it to the bugFlags initialization in PlayerAvmCore::PlayerAvmCore and PlayerAvmCore::QueueAbcBuffer

Comment 4

[Lars T Hansen]

(In reply to comment #3)
> Also need a way to easily enable/disable the bugzilla bitfield bugs from the
> shell (preferable over having to compile the value in).
> 
> At the moment, then bit-flag for bug 444630 is set to 0 in the shell, with no
> easy way to turn it on / off.

Actually the current solution with bit flags in an int will collapse almost immediately once we start landing fixes.  It needs to be replaced immediately with standard C++ bit fields or a similar scalable mechanism.  (There is no reason that I can see that we should not be using standard C++ bit fields here.)

If there's any nervousness about placing these variables in the pool's top-level namespace - eg debuggers that insist on displaying them all, thus hiding too many other members - then put them in a struct.

Names of the bitfield could usefully name the bug tracking system, ie "bug444630" is better rendered as "bugzilla444630", if we anticipate landing fixes for other bug trackers.

Comment 5

[Chris Peyer]

Pushed --enable-api-versioning patch to redux - changeset: 2878:3d195cbcc865

Comment 6

[Lars T Hansen]

More thoughts about versioning: it seems to me that -api N should set the flags for specific bugs in a way consistent with how version N sets flags in the player.  In particular, the player needs an API to set those flags in the VM as well and it will probably (should probably) use the same mechanism it uses to communicate the version of the swf.  There is no utility, I think, in setting flags in configurations other than those we would use in the player, because it may not make sense for individual bugs to be tested - the fix for one bug may rely on the fix for another, and that would be correct if the former bug is only "fixed" in the same or later versions as the latter.

Comment 7

[Lars T Hansen]

(and I meant to add:)

That being so, is there any reason at all - other than custom, and /maybe/ documentation - why we should have flags for individual bugs and not for API versions in the C++ code?  The utility of hundreds of flags is sort of lost on me.

Comment 8

[Brent Baker]

Comment on attachment 407809 [details] [diff] [review]
enable -api parameter in shell 

Shouldn't the avmfeatures.py only set AVMFEATURE_xxx? This patch adds a VMCFG_xxx option.

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> (From update of attachment 407809 [details] [diff] [review])
> Shouldn't the avmfeatures.py only set AVMFEATURE_xxx? This patch adds a
> VMCFG_xxx option.

Yeah, kind of strange.  Could still be valid if we don't think we should expose that feature, but that's a slippery slope we probably should not be going down without careful thinking.

Comment 10

[Steven Johnson]

(In reply to comment #7)
> (and I meant to add:)
> 
> That being so, is there any reason at all - other than custom, and /maybe/
> documentation - why we should have flags for individual bugs and not for API
> versions in the C++ code?  The utility of hundreds of flags is sort of lost on
> me.

+1, I think the original design of the "bugflags" feature didn't consider the possible size of changes. Switching to a "bug-compatible-version" set of flags (with a few possible exceptions for special cases) makes more sense IMHO.

Comment 11

[Lars T Hansen]

Bug cleanup note:

This bug is only about providing a switch to the avmshell that makes the avmshell request a particular, named, coherent version of C++ and ABC code for testing.  For other aspects of the problem, go up to bug #535770 and look at sibling bugs of this bug.

Comment 12

[Lars T Hansen]

A more-or-less complete solution is attached to bug #535770 but it's premature to land anything until the problem has been researched in more depth and the core solutions have been implemented.

Comment 13

[Trevor Baker]

Is this really not yet resolved?

Comment 14

[Dan Smith]

Dan, determine whether this bug can be closed or the action required to close it.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.