eliminate the loose dependence of avm and avmglue on api-versions.xml

RESOLVED FIXED

Status

Tamarin
Tools
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Jeff Dyer, Assigned: Steven Johnson)

Tracking

unspecified
x86
Mac OS X
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
apivergen.as generates api-versions.as and api-versions.h from api-versions.xml.

.as files in the builtins and avmglue depend on the api-versions.as. .cpp/.h files in avm and avmglue depend on api-versions.h.

after running apivergen.as over api-versions.xml, the generated .as file must be copied to a location that is in the .as include path of the avm and avmglue, and the .h file must be copied into the .cpp include path.

additionally, there is code in Flash and AIR cores with version information that need to stay synchronized with this version information.

when we build builtin.abc/.cpp/.h generate a single file that provides all versioning information needed throughout the system.

optionally, use metadata in builtin.abc to inform ASC of the mapping between API names (e.g. FP_10_2) and api version numbers.
(Reporter)

Comment 1

8 years ago
we have discussed the value of eliminating apivergen.as in favor of doing similar processing with nativegen.py. i think this is a bit of a red herring since it is trivial to run apivergen.as at the same time as nativegen.py. what is really at issue here is what needs to be generated. if that is a single .h file, then the workflow issues that we are currently suffering will go away regardless of which utility generate it.

Comment 2

8 years ago
(On that note, I'm on an anti-python pro-dogfood campaign: moving code from AS3 into Python is the wrong direction.)
(Assignee)

Comment 3

8 years ago
Created attachment 492015 [details] [diff] [review]
Patch

(NOTE: THIS PATCH REQUIRES a special build of asc.jar from Jeff Dyer.)

Basic concept here is to avoid having to build a global whitelist of version urls ahead of time; asc.jar no longer needs (or generates) such a a list of blessed uris, but is expected to simply mark all "[API]" annotated namespaces regardless of uri.

Then, nativegen.py sniffs for those namespaces and emits them in the glue; when we initialize natives we add them to a hashtable of versioned uris. (Which is probably slightly faster than the old binary search lookup we did, though I suspect it doesn't much matter.)

The one gotcha is that a versioned namespace can't appear in "unversioned" form before it appears in "versioned" form, but this should be trivial to guarantee (and patch includes debug verification of this.)

With Jeff's special asc.jar, this patch will applies and function correctly in avmshell and Flash; AIR is having build difficulties that I haven't deciphered; perhaps stale asc.jar, or perhaps other compilers (mxml? compc?) that need similar munging?

Anyway, please review and/or comment.
Assignee: nobody → stejohns
Attachment #492015 - Flags: review?(jodyer)
Attachment #492015 - Flags: feedback?(cthilgen)
(Assignee)

Comment 4

8 years ago
In case Jeff or Chris want to investigate, the AIR failure I get is:


Building airglobal.swc
Loading configuration file /Users/stejohns/FlashFarm/sb3_FlashRuntimeCode/flash/avmglue/flex-config.xml
Error: id
java.lang.NoSuchFieldError: id
	at flex2.compiler.as3.SyntaxTreeEvaluator.evaluate(SyntaxTreeEvaluator.java:191)
	at macromedia.asc.parser.MetaDataNode.evaluate(MetaDataNode.java:39)
	at flash.swf.tools.as3.EvaluatorAdapter.evaluate(EvaluatorAdapter.java:349)
	at macromedia.asc.parser.StatementListNode.evaluate(StatementListNode.java:60)
	at flash.swf.tools.as3.EvaluatorAdapter.evaluate(EvaluatorAdapter.java:934)
	at macromedia.asc.parser.ProgramNode.evaluate(ProgramNode.java:80)
	at flex2.compiler.as3.As3Compiler.parse1(As3Compiler.java:378)
	at flex2.compiler.CompilerAPI.parse1(CompilerAPI.java:2851)
	at flex2.compiler.CompilerAPI.parse1(CompilerAPI.java:2804)
	at flex2.compiler.CompilerAPI.batch1(CompilerAPI.java:253)
	at flex2.compiler.CompilerAPI.batch(CompilerAPI.java:1270)
	at flex2.compiler.CompilerAPI.compile(CompilerAPI.java:1488)
	at flex2.compiler.CompilerAPI.compile(CompilerAPI.java:1375)
	at flex2.tools.Compc.compc(Compc.java:340)
	at flex2.tools.Compc.compc(Compc.java:83)
	at flex2.tools.Compc.main(Compc.java:49)

So perhaps a new rev of compc (that also ignores uris or something) is necessary?
(Reporter)

Comment 5

8 years ago
Comment on attachment 492015 [details] [diff] [review]
Patch

The approach taken here is sound, but I found a couple issues that should be corrected before landing.

1/When I apply this patch, rebuild apivergen.as and rebuild the builtin.abc and shell_toplevel.abc, and re-make, I get 44 test failures in ./test/acceptance/versioning. They all have to do with toplevel get/set functions. (disclaimer, could be user error on my part.)

2/In nativegen.py, I'd look in the constant pool for namespaces whose URI has a version marker rather than for traits with a metadata. This is because, a compiler might version namespaces for which there is no use as a trait namespace, or might erase the metadata altogether.
Attachment #492015 - Flags: review?(jodyer) → review-
(Reporter)

Comment 6

8 years ago
(In reply to comment #4)
> In case Jeff or Chris want to investigate, the AIR failure I get is:
> 
> 
> Building airglobal.swc
> Loading configuration file
> /Users/stejohns/FlashFarm/sb3_FlashRuntimeCode/flash/avmglue/flex-config.xml
> Error: id
> java.lang.NoSuchFieldError: id
>     at
> flex2.compiler.as3.SyntaxTreeEvaluator.evaluate(SyntaxTreeEvaluator.java:191)
>     at macromedia.asc.parser.MetaDataNode.evaluate(MetaDataNode.java:39)
>     at flash.swf.tools.as3.EvaluatorAdapter.evaluate(EvaluatorAdapter.java:349)
>     at
> macromedia.asc.parser.StatementListNode.evaluate(StatementListNode.java:60)
>     at flash.swf.tools.as3.EvaluatorAdapter.evaluate(EvaluatorAdapter.java:934)
>     at macromedia.asc.parser.ProgramNode.evaluate(ProgramNode.java:80)
>     at flex2.compiler.as3.As3Compiler.parse1(As3Compiler.java:378)
>     at flex2.compiler.CompilerAPI.parse1(CompilerAPI.java:2851)
>     at flex2.compiler.CompilerAPI.parse1(CompilerAPI.java:2804)
>     at flex2.compiler.CompilerAPI.batch1(CompilerAPI.java:253)
>     at flex2.compiler.CompilerAPI.batch(CompilerAPI.java:1270)
>     at flex2.compiler.CompilerAPI.compile(CompilerAPI.java:1488)
>     at flex2.compiler.CompilerAPI.compile(CompilerAPI.java:1375)
>     at flex2.tools.Compc.compc(Compc.java:340)
>     at flex2.tools.Compc.compc(Compc.java:83)
>     at flex2.tools.Compc.main(Compc.java:49)
> 
> So perhaps a new rev of compc (that also ignores uris or something) is
> necessary?

This is most certainly due to asc.jar being out of sync with the Flex SDK used to build AIR. I suspect that Chris knows where to get the latest Flex SDK
(Assignee)

Comment 7

8 years ago
(In reply to comment #5)
> 1/When I apply this patch, rebuild apivergen.as and rebuild the builtin.abc and
> shell_toplevel.abc, and re-make, I get 44 test failures in
> ./test/acceptance/versioning. They all have to do with toplevel get/set
> functions. (disclaimer, could be user error on my part.)

I'm not seeing that -- (I skipped the rebuild of apivergen.as) -- on a Mac Release build I get no failures. What build(s) are you trying?
(Assignee)

Comment 8

8 years ago
(In reply to comment #6)
> This is most certainly due to asc.jar being out of sync with the Flex SDK used
> to build AIR. I suspect that Chris knows where to get the latest Flex SDK

Um, ok, but I don't know how that's gonna solve my problem... does compc embed its own asc, or refer to ours, or...?
(Assignee)

Comment 9

8 years ago
Created attachment 493866 [details] [diff] [review]
Patch v2

Revise to emit the URIs based on the magic mark in the namespace, rather than parsing metadata. (I still can't repeat the failures you are seeing -- are you sure you're building with the right asc.jar?)
Attachment #492015 - Attachment is obsolete: true
Attachment #493866 - Flags: review?(jodyer)
Attachment #493866 - Flags: feedback?(cthilgen)
Attachment #492015 - Flags: feedback?(cthilgen)
(Assignee)

Comment 10

8 years ago
Upon further reflection, I wonder if the test


                    if api = 0 and not strippeduri in self.versioned_uris:

should be

                    if api > 660 and not strippeduri in self.versioned_uris:

(i.e., only add to the list of versioned apis if it has a version *and* it's a version other than the lowest possible one, which will always match everything...)
(Reporter)

Comment 11

8 years ago
Comment on attachment 493866 [details] [diff] [review]
Patch v2

The logic looks good, and the code passes the tests for me know (as suspected, previous failures were mine, not the code's.)
Attachment #493866 - Flags: review?(jodyer) → review+
(Assignee)

Comment 12

8 years ago
What are your thoughts on Comment 10?
(Assignee)

Comment 13

8 years ago
Created attachment 494206 [details] [diff] [review]
Patch v3

One final tweak: we now keep track of what version(s) are seen for each uri, and if a uri has only a single version, which is the lowest value seen, don't emit it. (I decided to avoid hardcoding "660" into the script since those numbers might conceivably change in the future, and I'm assuming that api.jar -apiversioning stamps the low value onto any namespace that is otherwise unversioned...)
Attachment #493866 - Attachment is obsolete: true
Attachment #494206 - Flags: review?(jodyer)
Attachment #493866 - Flags: feedback?(cthilgen)

Updated

8 years ago
Flags: flashplayer-qrb?
(Assignee)

Comment 14

8 years ago
Created attachment 494440 [details] [diff] [review]
Patch v3 addendum

Two issues turned up in AIR testing:

-- minor: the "lowest version" tracking needs to track across multiple ABCs, as some of the AIR "mobile profile" build code can run nativegen on multiple ABCs. Trivial to fix.

-- major: this design requires that any versioned URI can't previously be seen as non-versioned; it turns out that avmplus uses "flash.errors" in a nonversioned way, but AIR uses it in a versioned way. I see two options to deal with this...
   -- the hack approach (in this patch) simply ensures that the there is versioning for flash.errors in the VM. Simple to detect and fix, but conceptually ugly.
   -- alternately, we could just emit *all* builtin uris as being "versioned", even if they are version 660 (effectively unversioned); downside here is that the uri lookup map gets larger, which may or may not matter. 

Opinions?
Attachment #494440 - Flags: review?(jodyer)
(Reporter)

Comment 15

8 years ago
I choose the second option. This is what the previous, interim, solution did: version all builtin public package namespaces. If the uri lookup gets noticeably slower, there should be a better hashing function to fix that.
(Reporter)

Updated

8 years ago
Attachment #494206 - Flags: review?(jodyer) → review+
(Reporter)

Comment 16

8 years ago
Comment on attachment 494440 [details] [diff] [review]
Patch v3 addendum

This is harmless, but doesn't solve the general problem, which occurs when two builtin components don't agree on whether an URI is versioned. I suggest implementing the more general solution suggested in a comment above.
Attachment #494440 - Flags: review?(jodyer) → review+
(Assignee)

Comment 17

8 years ago
Created attachment 494446 [details] [diff] [review]
Patch v4

OK then, here's a patch that does just that. (Relevant changes mainly in nativegen.py)
Attachment #494206 - Attachment is obsolete: true
Attachment #494440 - Attachment is obsolete: true
Attachment #494446 - Flags: review?(jodyer)
(Assignee)

Comment 18

8 years ago
(In reply to comment #15)
> I choose the second option. This is what the previous, interim, solution did:
> version all builtin public package namespaces. If the uri lookup gets
> noticeably slower, there should be a better hashing function to fix that.

Yeah, good point; I would be very surprised if the quadratic-probe-hashtable approach in this patch is meaningfully slower than the binary-search approach we previously used (in fact, I'd wager it's trivially faster).
(Reporter)

Updated

8 years ago
Attachment #494446 - Flags: review?(jodyer) → review+

Comment 19

8 years ago
changeset: 5607:9abeebd7725c
user:      Steven Johnson <stejohns@adobe.com>
summary:   Bug 602614 - eliminate the loose dependence of avm and avmglue on api-versions.xml (r=jodyer) NOTE: THIS PATCH REQUIRES NEW ASC.JAR

http://hg.mozilla.org/tamarin-redux/rev/9abeebd7725c
(Assignee)

Comment 20

8 years ago
pushed to TR: 5607:9abeebd7725c
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: flashplayer-qrb? → flashplayer-qrb+
You need to log in before you can comment on or make changes to this bug.