599277 - Atom representation should move from AvmPlus into base layer so that MMgc can access it

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

For the exact tracing work I would like MMgc to have knowledge of the atom representation of the AVM.  (I could slice it differently by delegating the problem from MMgc back into the AVM, but the result is the same - a dependency.  Exposing the atom representation seems like the smallest injection of dependency.  I'm open to arguments against that.)  MMgc would only use this information when tracing AVM objects that are specially marked as such, so it would not get in the way of using MMgc for other kinds of objects.  Indeed, MMgc could support exact tracing of other kinds of atoms as well.

When we compile for the avmshell the atom information is available through the magic of files already included in the environment; for the Flash Player some additional including is necessary, and this is why I'd like to make changes: If we don't segregate the atom representation from the rest of the AVM we risk having to pull in a varying amount of AVM header files into MMgc when MMgc is separately compiled by the Flash Player.  I want to avoid that by clearly separating the atom representation into a base layer whose files are guaranteed to be independent of the AVM.

Judging from preliminary work this is not going to be a large change.  Right now the atoms are in various namespaces: avmplus::Atom, avmplus::AtomConstants::whatever.  There are a couple of dependencies from atom-inlines.h into AvmCore, but I think those can be broken without too much trouble.

Comment 1

[Lars T Hansen]

Attachment: Preliminary - for safe keeping

Comment 2

[Lars T Hansen]

This will probably mean (see bug #591492):

- Move atom.h, atom-inlines.h, atom.cpp into vmbase/

- #include "vmbase.h" from avmplus.h and MMgc.h

- Break the dependency from ScriptObject* atomObj() by allowing it to
  reference an extern function provided by AvmCore in DEBUG builds,
  where the function is declared in atom.h.

Comment 3

[Lars T Hansen]

Note: to test this with Xcode (ie test that the atom definitions aren't accidentally included through precompiled headers), it is necessary to apply this patch:

diff --git a/build/avmshell-common.xcconfig b/build/avmshell-common.xcconfig
--- a/build/avmshell-common.xcconfig
+++ b/build/avmshell-common.xcconfig
@@ -38,8 +38,8 @@
 GCC_STRICT_ALIASING = YES
 GCC_ENABLE_CPP_EXCEPTIONS=NO
 GCC_ENABLE_CPP_RTTI=NO
-GCC_PREFIX_HEADER = ../../../core/avmplus.h
-GCC_PRECOMPILE_PREFIX_HEADER = YES
+//GCC_PREFIX_HEADER = ../../../core/avmplus.h
+GCC_PRECOMPILE_PREFIX_HEADER = NO
 
 // shell is included in this search path only for avmshell-features.h, included from platform/VMPI.h
 HEADER_SEARCH_PATHS=../../../core ../../../MMgc ../../../pcre ../../../extensions .. ../.. ../../../shell

Comment 4

[Lars T Hansen]

Attachment: Preliminary #2

This is probably roughly right.  Ignore the changes to the build settings.

Comment 5

[Lars T Hansen]

Attachment: Add vmbase-inlines.h and include it in avmplus.h and MMgc.h

Comment 6

[Lars T Hansen]

Attachment: Move the atom code from core to vmbase

Here I've opted to keep atomObj with the atom code and create a dependency back to the core code by means of a forward declaration of class ScriptObject and a DEBUG-mode function for checking that the argument is valid.  The alternative would be to keep atomObj in core, since its return value is a core type.

(This is a large patch because of project file changes (Xcode, MSVC); the functional changes are very modest.)

Comment 7

[Steven Johnson]

Comment on attachment 479008 [details] [diff] [review]
Move the atom code from core to vmbase

R+ on condition you run fixtabs before landing...

Comment 8

[Lars T Hansen]

tamarin-redux changeset:   5290:e5f0cb5885bf (merge)
tamarin-redux changeset:   5286:31cb27e929bf (move atoms)
tamarin-redux changeset:   5285:2511d3c10de7 (vmbase-inlines.h)

Comment 9

[Lars T Hansen]

Backed out 31cb27e929bf because I've no time to fix flashruntime-redux.

Comment 10

[Lars T Hansen]

Attachment: The revert patch

Comment 11

[Lars T Hansen]

Tommy fixed the issues:

changeset:   5303:587a4476e0ea
changeset:   5301:bf24595d5b5d
changeset:   5300:1f3798783da2
changeset:   5299:79323113e543