GCList, RCList, WeakRefList should use GCRef in SafeGC mode

NEW
Unassigned

Status

Tamarin
Virtual Machine
8 years ago
7 years ago

People

(Reporter: Steven Johnson, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Future
Bug Flags:
in-testsuite -
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug -

Details

(Whiteboard: safegc)

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
What it says. Also, arguably, GCList and RCList should combine into one.
(Reporter)

Updated

8 years ago
Depends on: 565664
(Reporter)

Comment 1

8 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

8 years ago
Created attachment 487443 [details] [diff] [review]
Patch1, v1: GCList uses GCRef<>

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

8 years ago
Created attachment 487444 [details] [diff] [review]
Patch2, v2: RCList -> GCRef<>

Same treatment for RCList.
Attachment #487444 - Flags: review?(bgetlin)
Attachment #487444 - Flags: feedback?(treilly)
(Reporter)

Comment 4

8 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.
(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

8 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

8 years ago
Created attachment 487454 [details] [diff] [review]
487444: Patch3, v1: WeakRefList -> GCRef<>
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.
(Reporter)

Comment 9

8 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

8 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

8 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

8 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

8 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

8 years ago
No, but I thought of another approach to try last night, I'll give it a shot first...

Comment 15

8 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

8 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

8 years ago
Attachment #487444 - Flags: feedback+ → feedback-

Updated

8 years ago
Attachment #487454 - Flags: feedback?(treilly) → feedback-
(Reporter)

Comment 17

8 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

8 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

8 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

8 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

8 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

8 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

8 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

8 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.

Comment 25

8 years ago
Created attachment 491204 [details] [diff] [review]
GCRefList

How about this approach?
Attachment #491204 - Flags: feedback?(stejohns)
(Reporter)

Comment 26

8 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

8 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

8 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

8 years ago
Attachment #487444 - Flags: review?(bgetlin) → review-

Updated

8 years ago
Attachment #487454 - Flags: review?(bgetlin) → review-

Comment 29

7 years ago
Retargeting SafeGC work items to "future".
Flags: in-testsuite-
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Target Milestone: --- → Future

Updated

7 years ago
Whiteboard: safegc

Updated

7 years ago
Assignee: stejohns → nobody

Updated

7 years ago
No longer depends on: 607651

Updated

7 years ago
Blocks: 607651

Updated

7 years ago
No longer depends on: 565664
You need to log in before you can comment on or make changes to this bug.