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)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
RESOLVED
WONTFIX
Future
People
(Reporter: stejohns, Unassigned)
References
Details
(Whiteboard: safegc)
Attachments
(4 files)
34.32 KB,
patch
|
bgetlin
:
review-
treilly
:
feedback-
|
Details | Diff | Splinter Review |
25.53 KB,
patch
|
bgetlin
:
review-
treilly
:
feedback-
|
Details | Diff | Splinter Review |
11.19 KB,
patch
|
bgetlin
:
review-
treilly
:
feedback-
|
Details | Diff | Splinter Review |
7.32 KB,
patch
|
stejohns
:
feedback-
|
Details | Diff | Splinter Review |
What it says. Also, arguably, GCList and RCList should combine into one.
Reporter | ||
Comment 1•14 years ago
|
||
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.
Reporter | ||
Comment 2•14 years ago
|
||
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)
Reporter | ||
Comment 3•14 years ago
|
||
Same treatment for RCList.
Attachment #487444 -
Flags: review?(bgetlin)
Attachment #487444 -
Flags: feedback?(treilly)
Reporter | ||
Comment 4•14 years ago
|
||
(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.
Comment 5•14 years ago
|
||
(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 };
Reporter | ||
Comment 6•14 years ago
|
||
(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.
Reporter | ||
Comment 7•14 years ago
|
||
Attachment #487454 -
Flags: review?(bgetlin)
Attachment #487454 -
Flags: feedback?(treilly)
Comment 8•14 years ago
|
||
(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.
Reporter | ||
Comment 9•14 years ago
|
||
(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.
Comment 10•14 years ago
|
||
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.
Reporter | ||
Comment 11•14 years ago
|
||
(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...
Reporter | ||
Comment 12•14 years ago
|
||
(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.
Comment 13•14 years ago
|
||
What was the problem, was it that the type isn't defined yet and T::WriteBarrier can't be resolved by the compiler?
Reporter | ||
Comment 14•14 years ago
|
||
No, but I thought of another approach to try last night, I'll give it a shot first...
Comment 15•14 years ago
|
||
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 16•14 years ago
|
||
Comment on attachment 487444 [details] [diff] [review] Patch2, v2: RCList -> GCRef<> What's going on with CodegenLIR.cpp?
Attachment #487444 -
Flags: feedback?(treilly) → feedback+
Updated•14 years ago
|
Attachment #487444 -
Flags: feedback+ → feedback-
Updated•14 years ago
|
Attachment #487454 -
Flags: feedback?(treilly) → feedback-
Reporter | ||
Comment 17•14 years ago
|
||
(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?
Comment 18•14 years ago
|
||
(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?)
Reporter | ||
Comment 19•14 years ago
|
||
(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.
Comment 20•14 years ago
|
||
(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.
Reporter | ||
Comment 21•14 years ago
|
||
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.
Comment 22•14 years ago
|
||
Let's table the discussion until safegc has been landed in the shell/player. No need to do everything all at once.
Comment 23•14 years ago
|
||
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).
Comment 24•14 years ago
|
||
(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.
Reporter | ||
Comment 26•14 years ago
|
||
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-
Comment 27•14 years ago
|
||
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 28•14 years ago
|
||
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-
Updated•14 years ago
|
Attachment #487444 -
Flags: review?(bgetlin) → review-
Updated•14 years ago
|
Attachment #487454 -
Flags: review?(bgetlin) → review-
Comment 29•13 years ago
|
||
Retargeting SafeGC work items to "future".
Flags: in-testsuite-
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Target Milestone: --- → Future
Updated•13 years ago
|
No longer depends on: safegc-tracker
Updated•13 years ago
|
Blocks: safegc-tracker
Comment 30•6 years ago
|
||
Tamarin is a dead project now. Mass WONTFIX.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Comment 31•6 years ago
|
||
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in
before you can comment on or make changes to this bug.
Description
•