523192 - Atom should be an opaque type

Status: VERIFIED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Currently it's just an intptr, that code manipulates directly in many places. This is error prone in various ways (eg 64-bit vs 32-bit int atoms are tricky to get right) and also makes overloading hard, since Atom isn't a distinct type. It also prevents us from experimenting with different Atom representations.

Fixing this will be painful since misuse is pervasive in code. This bug will track multiple patches that are intended to get us there in baby steps.

Comment 1

[Steven Johnson]

Attachment: Cleanup: remove bogus "atom" usage in cpool_mn

For historical reasons, cpool_mn stored offsets as "atoms". No longer necessary or desirable.

Comment 2

[Edwin Smith]

Comment on attachment 407084 [details] [diff] [review]
Cleanup: remove bogus "atom" usage in cpool_mn

No visible bugs, but since we're touching all the code anyway, a constantMnCount() getter would be good.  (there's no good justification for cpool_mn even being a list in the first place, really).

Comment 3

[Steven Johnson]

(In reply to comment #2)
> No visible bugs, but since we're touching all the code anyway, a
> constantMnCount() getter would be good.  (there's no good justification for
> cpool_mn even being a list in the first place, really).

urr? what else would it be? re: the getter, I can add it, but since all use of it is related to the size-of-the-list, using the list's size() makes more sense IMHO

Comment 4

[Edwin Smith]

well, it could be a uint32_t* fixed-alloc'd array, for one thing.  the getter would just make a change like that involve less churn.  not a big deal.

Comment 5

[Steven Johnson]

Comment on attachment 407084 [details] [diff] [review]
Cleanup: remove bogus "atom" usage in cpool_mn

pushed to redux as changeset:   2820:317dc971f187

Comment 6

[Steven Johnson]

Attachment: first cut at Atom api

First cut at an opaque Atom-access API. This api is modeled directly off the API from the earlier Box work done a few months ago. This initial patch provides the basic API functionality that is necessary to abstract Atom access, along with AVMFEATURE_OPAQUE_ATOM to enable or disable it. 

Note that the code won't compile at all with the feature enabled; that's expected in this patch. 

My goal here is to review this patch for general API suitability, and if/when approved, continue adding subsequent patches that modify files (in small reviewable chunks) to use the Atom apis. When we get to a state that allows us to build with OPAQUE_API on, we change the default in avmshell-features.h. (Embedders will almost certainly need more massaging, so the feature will probably live for a while after that.)

I expect that the functions defined in atom.h will probably have to be expanded a bit, but based on the Box work, this is probably at least 80% of the functionality we need.

The only real uncertainty about this API I have is how exactly we should expose the int/uint/number issue, and in particular, how to deal with lingering 64-bit Atom integer issues (see bugs 522803 and 521353).

Comment 7

[Steven Johnson]

Upon further reflection (see also bug 522803), I'm going to revise the int/double parts of this API... I think the way to resolve lingering 64-bit issues is not to add "isInt32", etc, but rather focus the api on the fact that there are two numeric atom types: double and intptr. the key is to recognize that the intptr atom types can be larger than 32-bits and ensure the API reflects that.

Comment 9

[Steven Johnson]

Attachment: Atom api #2

Revisions to atom api after further reflection and experimenting. Input on int-related methods particularly welcome.

Comment 10

[Edwin Smith]

AvmCore.h:1001 - this assert should fire if given one of the large (>53 bit int) atoms generated by the test case for bug 521353.  thats probably okay since its an open bug, but beware.

Interpreter.cpp: integer_u() changed to integer_i(), but index is still uint32_t.  intentional?

atom-inlines.h: whitespace:  no tabs, please.  if you can enable your editor to read modelines, please do.  

atom.cpp: (nit) atomFromDouble and atomFromInt32/UInt32 may only need a GC*, not core*, if all they do is use core->GetGC() to allocate.

atom.h:  there is a rogue typedef OpaqueAtom in jit-calls.h, we could move it to atom.h and conditionally typedef OpaqueAtom Atom;  (OpaqueAtom is used to disambiguate Atom from int32_t for some overloaded functions).

(lowpri) maybe the Binding definitions should move to Traits.h

avmplus.h: lets split AvmCore.h into AvmCore-inlines.h instead of rearranging where atom-inlines.h is in avmplus.h

style question:  since all the accessors are prefixed with "atom" anyway, should we just bite it and make an Atom class with static methods?  (Atom::getObj(), etc)?  hurts the eyes a more, but a class def lets the compiler enforce that the interface & implementations match up, not to mention lets helper code be private.  I'd be fine if the answer is "hell no" but its worth asking since our helpers functions have gotten longer more verbose names.

Comment 11

[Steven Johnson]

(In reply to comment #10)
> AvmCore.h:1001 - this assert should fire if given one of the large (>53 bit
> int) atoms generated by the test case for bug 521353.  thats probably okay
> since its an open bug, but beware.

agreed. my approach here is not try to fix those bugs in the first pass; rather, get all access encapsulated first, then fix those in a later pass.
 
> Interpreter.cpp: integer_u() changed to integer_i(), but index is still
> uint32_t.  intentional?

Yes! Perhaps I should file a second bug. Verifier actually verifies this as int, not uint, and we do in fact get negative values.

> atom-inlines.h: whitespace:  no tabs, please.  if you can enable your editor to
> read modelines, please do.  

afaik XCode (my primary editor) doesn't do that, alas. I'll try to detab future patches.

> atom.cpp: (nit) atomFromDouble and atomFromInt32/UInt32 may only need a GC*,
> not core*, if all they do is use core->GetGC() to allocate.

true. theory here was that all accessors that needed "extra" arg would get an AvmCore, for consistency, but I could be persuaded to switch to gc.

> atom.h:  there is a rogue typedef OpaqueAtom in jit-calls.h, we could move it
> to atom.h and conditionally typedef OpaqueAtom Atom;  (OpaqueAtom is used to
> disambiguate Atom from int32_t for some overloaded functions).

good call, will do

> (lowpri) maybe the Binding definitions should move to Traits.h

agreed, but probably a later patch

> avmplus.h: lets split AvmCore.h into AvmCore-inlines.h instead of rearranging
> where atom-inlines.h is in avmplus.h

good call, will do in a separate patch that can be landed independently to reduce merge churn

> style question:  since all the accessors are prefixed with "atom" anyway,
> should we just bite it and make an Atom class with static methods? 
> (Atom::getObj(), etc)?  hurts the eyes a more, but a class def lets the
> compiler enforce that the interface & implementations match up, not to mention
> lets helper code be private.  I'd be fine if the answer is "hell no" but its
> worth asking since our helpers functions have gotten longer more verbose names.

hmm... good question. I'm inclined to say 'no' and keep as-is; these methods are pervasive enough that size matters. On the other hand, existing usage is even wordier (eg AvmCore::atomToScriptObject) so maybe this isn't an issue.

Lars, your thoughts?

Related style questions worth asking...

atomGetObj vs atomGetObject
atomGetStr vs atomGetString
atomGetNs vs atomGetNamespace
UInt32 vs Uint32
etc

Comment 12

[Steven Johnson]

Attachment: AvmCore-inlines

Added AvmCore-inlines.h and moved all relevant functions there. Other than drive-by int-type fixes, no other changes.

Exception: readU30 and skipU30 probably need to be moved there, but seem too large to mark REALLY_INLINE without investigating what the current compiler behavior really is. I marked 'em with @todo for now.

Comment 13

[Steven Johnson]

Scope-creep question: in the original Box work this was based on, all the coercion methods currently in AvmCore were moved as well (eg AvmCore::booleanAtom -> atomToBool, etc). I avoided that here on the theory that it wasn't essential, as almost all of those can be rewritten to use the primitives presented here. Opinions otherwise?

Comment 14

[Edwin Smith]

its fine.

language operator implementations are migrating to instr.cpp/h, so the code in atom.cpp/h should try to stay focused on atom encoding issues and not langue operator implementations, including coersion.

a minor goal for atom.cpp/h is to keep it free of AvmCore dependencies so someday it can be used from MMgc without dragging in all the VM definitions.  (not sure of feasability, but its nice to have as we think about Atom-aware GC code).

Comment 15

[Lars T Hansen]

(In reply to comment #14)
> a minor goal for atom.cpp/h is to keep it free of AvmCore dependencies so
> someday it can be used from MMgc without dragging in all the VM definitions. 
> (not sure of feasability, but its nice to have as we think about Atom-aware GC
> code).

I suspect the most we can hope for is to expose representation and selected tags downward to MMgc, so that it can make intelligent use of them for eg exact tracing.  The methods can be tied to AvmCore, generally - if there's a subset that doesn't require AvmCore ties then we can factor out later.

Comment 16

[Edwin Smith]

Comment on attachment 407579 [details] [diff] [review]
AvmCore-inlines

Don't forget project file updates to add AvmCore-inlines.h

Comment 17

[Steven Johnson]

(In reply to comment #14)
> language operator implementations are migrating to instr.cpp/h, so the code in
> atom.cpp/h should try to stay focused on atom encoding issues and not langue
> operator implementations, including coersion.

sounds like a good goal. I'll focus on ensuring that atom.x avoids dealing with anything that isn't strictly about encapsulating the atom representation.
 
> a minor goal for atom.cpp/h is to keep it free of AvmCore dependencies so
> someday it can be used from MMgc without dragging in all the VM definitions. 

agreed. I actually have a revised version of the initial patch with more of that done -- I want to fiddle with it a bit more before offering it for review, but have atom.x have no dependencies on avmcore.x is a goal that should be achievable.

> Don't forget project file updates to add AvmCore-inlines.h

will do.

Comment 18

[Steven Johnson]

Comment on attachment 407579 [details] [diff] [review]
AvmCore-inlines

pushed to redux as changeset:   2848:9a12580b9a73

Comment 19

[Steven Johnson]

Attachment: Rework Sampler.typeOrVTable

More preflight cleanup prior to atom api stuff... the Sampler code uses "Atom" (and uintptr) to mean an atomlike structure for "typeOrVTable", but it doesn't conform to a real Atom's constraints and won't compile with upcoming opaque-atom code. This patch uniformly replaces it with a new "SampleObjectType", which is (effectively) the same bit layout, but with a distinct type to avoid the collision and IMHO make the code more type-safe.

Comment 20

[Steven Johnson]

Comment on attachment 407857 [details] [diff] [review]
Rework Sampler.typeOrVTable

pushed (in advance of sr, with edwin's blessing) as changeset:   2875:b99c8865e80b

Comment 21

[Michael Daumling]

My $.02:

1) Make Atom a class containing the "old" atom. Visualizers will love that, and it is cleaner. Good compilers should be able to pass Atoms by value if they stay 4 bytes. This idea is not mandatory, however.

2) For 64-bit systems, use compressed pointers. An atomized pointer is a 32 bit offset from a 64 bit base address. MMgc should be able to support that.

3) Attach conversion routines as class members (we should check if compilers are able to work with the single member, or if they prefer to to two loads via "this", though). It is nice to have myAtom.toInt32() instead of Atom::toInt32(myAtom).

4) Move all Atom constants away from AvmCore to either a singleton, or, better to a static area.

5) If having a singleton is preferred, check if interned strings tables could be cross-AvmCores, and be part of that instance.

6) You cannot exclude AvmCore from method arguments if you want toString/valueOf functionality. You, however, derive from Atom (call it ASAtom) and collect functionality requiring AvmCore* there. AvmCore* should be an opaque value.

Why would it, BTW, be desirable to separate methods with AvmCore* parameter from methods with MMgc* parameter? Do you anticipate that Tamarin will support languages that are not AS-related?

Comment 22

[Steven Johnson]

(In reply to comment #21)
> My $.02:
> 
> 1) Make Atom a class containing the "old" atom. Visualizers will love that, and
> it is cleaner. Good compilers should be able to pass Atoms by value if they
> stay 4 bytes. This idea is not mandatory, however.

During the Box experiment a few months back, we found that some compilers were cranky about registerizing classes, regardless of size. However, we compromised and wrapped it in a class in Debug builds (handy for making it incompatible with "0"). I plan to do that here too, but first want to get to a point where I can flip "opaque" on.

> 2) For 64-bit systems, use compressed pointers. An atomized pointer is a 32 bit
> offset from a 64 bit base address. MMgc should be able to support that.

Worth experimenting on, but outside the scope of this bug... this bug is just to encapsulate access (which would be required for such a change)

> 3) Attach conversion routines as class members (we should check if compilers
> are able to work with the single member, or if they prefer to to two loads via
> "this", though). It is nice to have myAtom.toInt32() instead of
> Atom::toInt32(myAtom).

No, very much opposed to this, as it requires that an Atom be a class, which puts us at the mercy of stupid compilers. Existing C-style api gives us maximum flexibility as to Atom representation.
 
> 4) Move all Atom constants away from AvmCore to either a singleton, or, better
> to a static area.

Sounds good.
 
> 5) If having a singleton is preferred, check if interned strings tables could
> be cross-AvmCores, and be part of that instance.

Nice idea, but again outside the scope of this bug.
 
> Why would it, BTW, be desirable to separate methods with AvmCore* parameter
> from methods with MMgc* parameter? Do you anticipate that Tamarin will support
> languages that are not AS-related?

No, but minimizing dependencies is always a good thing.

Comment 23

[Lars T Hansen]

Comment on attachment 407374 [details] [diff] [review]
Atom api #2

Assuming this is more or less obsolete now?

Comment 24

[Steven Johnson]

(In reply to comment #23)
> (From update of attachment 407374 [details] [diff] [review])
> Assuming this is more or less obsolete now?

Too stale to review, yes. I have revised patches locally but am unlikely to be freshening them in the near future... I hope to get back to this in a few weeks.

Comment 25

[Edwin Smith]

Attachment: use atomKind() and atomPtr instead of explicit masking

This doesn't attempt to be comprehensive, but at the same time just cleans up a bunch of code using atomKind() and atomPtr, which already exist, so it should be low risk and uncontroversial.

Comment 26

[Steven Johnson]

Comment on attachment 417316 [details] [diff] [review]
use atomKind() and atomPtr instead of explicit masking

did you mean to change the v8 tests as well? or was that accidental?

Comment 27

[Edwin Smith]

oops, accidental, will exclude from patch when committing.

Comment 28

[Edwin Smith]

Comment on attachment 417316 [details] [diff] [review]
use atomKind() and atomPtr instead of explicit masking

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

Comment 29

[Andre Kruetzfeldt]

Changes will be tracked in a different bug.

Comment 30

[Edwin Smith]

bug# ?  consider marking this one as a dup of the new one, or blocking the new one, so there is a trail.

Comment 31

[Trevor Baker]

bulk verifying resolved !fixed issues