Need a story around Atom for safegc

VERIFIED DUPLICATE of bug 629746

Status

Tamarin
Virtual Machine
P3
normal
VERIFIED DUPLICATE of bug 629746
7 years ago
7 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

(Blocks: 1 bug)

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-bug -

Details

(Assignee)

Description

7 years ago
For Atom fields we need a HeapAtom class.  The existing avmplus::AtomWB class serves this purposes, we should just eradicate the ATOM_WB macro and rename it HeapAtom (parity with Multiname approach).

For Atom stack references I think we need an AtomRef class or somesuch which is a real class which has rules like GCRef (has to be used on stack not heap, can't be used to obtain pointers, can return ScriptObjectRef/StringRef etc).
(Assignee)

Comment 1

7 years ago
Requirements: 

1) Raw Atom type should be hidden from player/shell
2) It should be a compile error to put a HeapAtom class in anything other than a GCRoot/GCObject/GCFinalizedObject
3) It should be an assert (don't think compile time it possible) error to put an AtomRef in any other than the stack or a stack only allocated object

Feedback requested:

Are HeapAtom and AtomRef acceptable names?   They would live in the avmplus namespace.

Should AtomRef be an Atom in release builds and a class in debug builds (worried about performance, premature?)

Comment 2

7 years ago
Could we simply use GCMember<Atom> and GCRef<Atom>?  I know there's a bit of overloading of conceptual machinery, but it seems that fewer names are better than more names.

I'm sure that will run afoul of some existing nefarious uses of Atom in the Flash Player code base though...

I'm not on board with HeapAtom - HeapHashtable is not a good name.
(Assignee)

Comment 3

7 years ago
GCMember and GCRef assume T is a class, specifically one derived from GCObject or GCFinalizedObject.   And the storage is set up as T* which isn't what we want.

We have the Heap prefix in multiple places and its spreading, HeapMultiname, HeapList, HeapAtomList, HeapHashtable, HeapE4XNodeList, HeapNamespaceList.   

avmplus::AtomMember and avmplus::AtomRef?

Comment 4

7 years ago
I could absolutely live with AtomMember and AtomRef.  They should presumably be in the same namespace as GCMember and GCRef, and all four should normally be available in the player without further qualification (IMO).

Comment 5

7 years ago
Is it impossible to use garden variety template specialization for GCMember<Atom> (etc) because of the type sniffing that we do?
(In reply to comment #3)
> GCMember and GCRef assume T is a class, specifically one derived from GCObject
> or GCFinalizedObject.   And the storage is set up as T* which isn't what we
> want.

I took comment 2 to mean that we would introduce a distinct template
specialization (and a *complete* specialization) for GCMember<Atom> and
GCRef<Atom> that would not reuse the machinery in GCMember<T> or GCRef<T>, so
it would not be setting up the storage in the same way.  Assuming its feasible
to set up such a specialization.

(I'm not saying I like the conceptual overloading; it just seemed like there
was miscommunication between comments 2 and 3. . .)
(Assignee)

Comment 7

7 years ago
My bad, I can't think of any reason GCMember<Atom> and GCRef<Atom> wouldn't work with complete template specialization.     My template foo is weak ;-)

Comment 8

7 years ago
I could live with either AtomMember+AtomRef, or GCMember<Atom>+GCRef<Atom>.

Better yet, make both acceptable.

Comment 9

7 years ago
But for GCMember<Atom> to work, have to change Atom to be a unique type, not a uintptr. (Needs to be done anyway, it's just more work.)
(Assignee)

Comment 10

7 years ago
(In reply to comment #9)
> But for GCMember<Atom> to work, have to change Atom to be a unique type, not a
> uintptr. (Needs to be done anyway, it's just more work.)

Why?  I thought we had established that we could specialize GCMember<Atom> to avoid any dependencies on it being a type.  We want to change Atom to be a real class yes, but I don't think doing that needs to block the change to GCMember<Atom>.

Comment 11

7 years ago
templates can't specialize on a typedef; since "typedef intptr_t Atom", Atom looks like intptr_t to the type system, which means you could also do GCMember<int32_t>, which would be bad
(In reply to comment #11)
> templates can't specialize on a typedef; since "typedef intptr_t Atom", Atom
> looks like intptr_t to the type system, which means you could also do
> GCMember<int32_t>, which would be bad

I had assumed that such usage would, in the meantime, be caught during code reviews.  But perhaps that is assuming too much.  So maybe we should just go to AtomMember and AtomRef for now.

Comment 13

7 years ago
(In reply to comment #12)
> I had assumed that such usage would, in the meantime, be caught during code
> reviews.  

That would be nice; also, I would like a pony. (When it comes to glue code, assume that if it *can* be misused, it *will* be misused)
(Assignee)

Updated

7 years ago
Blocks: 607651
(Assignee)

Updated

7 years ago
No longer blocks: 607651

Updated

7 years ago
Flags: flashplayer-bug-
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 629746

Comment 15

7 years ago
bulk verifying resolved !fixed issues
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.