Closed Bug 608831 Opened 14 years ago Closed 6 years ago

GCList, RCList, WeakRefList should use GCRef in SafeGC mode

Categories

(Tamarin Graveyard :: Virtual Machine, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
Future

People

(Reporter: stejohns, Unassigned)

References

Details

(Whiteboard: safegc)

Attachments

(4 files)

What it says. Also, arguably, GCList and RCList should combine into one.
Depends on: 565664
Note: currently, we require you to declare GCList and friends in the form GCList<GCObject*>, but GCRef<GCObject> (ie, explicit * vs no *) -- this patch won't attempt to change the List suites to match GCRef, as that would require considerable extra code motion.
revise GCList to use GCRef<> for all interface methods, with minimal changes external to use new API. Still uses raw GCObject* internally and gets "friend" access to GCRef.
Assignee: nobody → stejohns
Attachment #487443 - Flags: review?(bgetlin)
Attachment #487443 - Flags: feedback?(treilly)
Same treatment for RCList.
Attachment #487444 - Flags: review?(bgetlin)
Attachment #487444 - Flags: feedback?(treilly)
(In reply to comment #1)
> Note: currently, we require you to declare GCList and friends in the form
> GCList<GCObject*>, but GCRef<GCObject> (ie, explicit * vs no *) -- this patch
> won't attempt to change the List suites to match GCRef, as that would require
> considerable extra code motion.

Responding to this point -- this is almost certainly required in order to consolidate GCList and RCList into one, but seems challenging to manage without a great deal of code motion, most of which will be required for SafeGC implementation in the first place. (ie, you can't reliably declare GCRef<Foo> or GCList<Foo> unless Foo has been actually declared, not merely forward-declared, otherwise we can't infer its base class). Thus I'm not going to attempt to unify these yet.
(In reply to comment #4)
> Responding to this point -- this is almost certainly required in order to
> consolidate GCList and RCList into one, but seems challenging to manage without
> a great deal of code motion, most of which will be required for SafeGC
> implementation in the first place. (ie, you can't reliably declare GCRef<Foo>
> or GCList<Foo> unless Foo has been actually declared, not merely
> forward-declared, otherwise we can't infer its base class).

Isn't the explicit GCObjectType template parameter available to work around this sort of issue?  Or do I misunderstand?

  enum GCObjectType { kTypeGCObject = 0, kTypeRCObject = 1, kTypeGCFinalizedObject = 2 };
(In reply to comment #5)
> Isn't the explicit GCObjectType template parameter available to work around
> this sort of issue?  Or do I misunderstand?

I don't consider that an acceptable option; the old List<> class had that as an equivalent option and it was the source of numerous bugs. If SafeGC is relying on this for such situations, I am concerned.
Attachment #487454 - Flags: review?(bgetlin)
Attachment #487454 - Flags: feedback?(treilly)
(In reply to comment #6)
> (In reply to comment #5)
> > Isn't the explicit GCObjectType template parameter available to work around
> > this sort of issue?  Or do I misunderstand?
> 
> I don't consider that an acceptable option; the old List<> class had that as an
> equivalent option and it was the source of numerous bugs. If SafeGC is relying
> on this for such situations, I am concerned.

I'm not familiar with what the old List<> class had; did it have safeguards in debug mode that dynamically confirmed that the parameter was selected properly?  (See e.g. the New methods of GCFactory.)  It seem like it would be difficult for bugs here to lie dormant for very long.
(In reply to comment #8)
> I'm not familiar with what the old List<> class had; did it have safeguards in
> debug mode that dynamically confirmed that the parameter was selected properly?
>  (See e.g. the New methods of GCFactory.)  It seem like it would be difficult
> for bugs here to lie dormant for very long.

An additional objection is that it forces us to sprout new warts at the end of many List declarations, at great detriment to readability. The new status quo is (IMHO) a great improvement that was put in place at substantial pain; I won't entertain going backwards in this respect.
Take a look at what I did in take 2 of the shell integration patch for my experimental GCMember you should be able make gclist/rclist in to one that way.
(In reply to comment #10)
> Take a look at what I did in take 2 of the shell integration patch for my
> experimental GCMember you should be able make gclist/rclist in to one that way.

It looks like it still has dependency requirements, but I'll take a poke at it...
(In reply to comment #11)
> (In reply to comment #10)
> > Take a look at what I did in take 2 of the shell integration patch for my
> > experimental GCMember you should be able make gclist/rclist in to one that way.
> 
> It looks like it still has dependency requirements, but I'll take a poke at
> it...

Yeah, that's not going to work for the List suite; GCRef<> is so lightweight that you can deal with the dependencies without too much pain, but List<> is much more complex and would require massive restructuring. We'd be better off living with the GCList/RCList dichotomy for now, especially since RCList will be going away when RCObject goes away.
What was the problem, was it that the type isn't defined yet and T::WriteBarrier can't be resolved by the compiler?
No, but I thought of another approach to try last night, I'll give it a shot first...
Comment on attachment 487443 [details] [diff] [review]
Patch1, v1: GCList uses GCRef<>

We want the list classes to support raw pointers for the avmcore.   Like if AVMSHELL_BUILD or AVMPLAYER_BUILD ( I made that up) are defined hide the pointer methods.  Doable?
Attachment #487443 - Flags: feedback?(treilly) → feedback-
Comment on attachment 487444 [details] [diff] [review]
Patch2, v2: RCList -> GCRef<>

What's going on with CodegenLIR.cpp?
Attachment #487444 - Flags: feedback?(treilly) → feedback+
Attachment #487444 - Flags: feedback+ → feedback-
Attachment #487454 - Flags: feedback?(treilly) → feedback-
(In reply to comment #15)
> We want the list classes to support raw pointers for the avmcore.   Like if
> AVMSHELL_BUILD or AVMPLAYER_BUILD ( I made that up) are defined hide the
> pointer methods.  Doable?

I suppose, but is this really necessary? Why can't everything use GCRef?

(In reply to comment #16)
> What's going on with CodegenLIR.cpp?

Um, what do you mean?
(In reply to comment #17)
> (In reply to comment #16)
> > What's going on with CodegenLIR.cpp?
> 
> Um, what do you mean?

I mean there's big diff chunks for this file om patch #2 that don't appear to change anything (whitespace?)
(In reply to comment #18)
> I mean there's big diff chunks for this file om patch #2 that don't appear to
> change anything (whitespace?)

Could be, or line endings, though I didn't do anything explicitly here.
(In reply to comment #17)
> I suppose, but is this really necessary? Why can't everything use GCRef?

Fair question, the plan from the outset has been to leave the core alone, the presumption being that the VM team can handle pointers and changing that code to GCRef will just be useless churn.

I'd like to eradicate DWB/DRCWB when GCMember lands but I don't see any value in using GCRef in core code.
I'm gonna disagree; what's good for the goose is good for gander, and I'm thinking that most code outside MMgc proper should migrate to GCRef if we're really going down that road.
Let's table the discussion until safegc has been landed in the shell/player.   No need to do everything all at once.
I think the question is can we have one list support pointers and GCRef?   I don't think so unless we make GCRef automatically construct and cast to the pointer type (as a temporary measure).
(In reply to comment #21)
> I'm gonna disagree; what's good for the goose is good for gander, and I'm
> thinking that most code outside MMgc proper should migrate to GCRef if we're
> really going down that road.

It was my call that the safegc infrastructure be targeted at the player, not at the vm, and I stand by it.  If there's any disagreement about it we should take it to a meeting (which I'm happy to do).

At this point there is a nontrivial chance that the safegc patch is going to be delayed and we should not land anything disruptive in the vm unless and until we're sure that it's needed.
Attached patch GCRefListSplinter Review
How about this approach?
Attachment #491204 - Flags: feedback?(stejohns)
Comment on attachment 491204 [details] [diff] [review]
GCRefList

I'm not opposed to a GCRefList, but using GCListHelper will only work properly for GCRefs that hold GCObjects; if the GCRef holds an RCObject then this is totally wrong. To make this work you need to also make a "GCRefListHelper" that knows how to do the right thing either way.
Attachment #491204 - Flags: feedback?(stejohns) → feedback-
k, I'm gonna table it for the moment, safegc has changed course again and we will have GCRef pointer auto conversion for the foreseeable future so the current list classes should suffice.
Comment on attachment 487443 [details] [diff] [review]
Patch1, v1: GCList uses GCRef<>

The general idea here seems right, however recent changes to the SafeGC syntax (namely the removal of the raw to GCRef conversion utilities) are now gone in favor of implicit conversion.  Let's table this until 565664 is approved.
Attachment #487443 - Flags: review?(bgetlin) → review-
Attachment #487444 - Flags: review?(bgetlin) → review-
Attachment #487454 - Flags: review?(bgetlin) → review-
Retargeting SafeGC work items to "future".
Flags: in-testsuite-
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Target Milestone: --- → Future
Whiteboard: safegc
Assignee: stejohns → nobody
No longer depends on: safegc-tracker
No longer depends on: 565664
Tamarin is a dead project now. Mass WONTFIX.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: