628355 - GCMember's inheriting from GCRef opens us up for missing write barriers

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P2)

Description

[Tommy Reilly]

Say I have a function:

void retrieve(GCRef<Foo>& t);

I can do:

GCRef<Foo> t;
retrieve(t);

But I can also pass a GCMember since GCMember inherits from GCRef and blamo, missing write barrier.

Comment 1

[Lars T Hansen]

Memo to all: descriptive bug titles makes it easier to understand whether a bug is important, when skimming a bug list...

Comment 2

[Tommy Reilly]

Spotted in the wild WE #2869733

Comment 3

[Tommy Reilly]

Okay I spent some time searching for a way to solve this w/o making GCRef not inherit from GCMember and none occurred to me or worked.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #3)
> Okay I spent some time searching for a way to solve this w/o making GCRef
> not inherit from GCMember and none occurred to me or worked.

What do we actually need to support the majority of the use cases?  Could we get away with removing the inheritance relationship and putting in a cast operator, i.e. operator (GCMember<T>)()?

Comment 5

[Tommy Reilly]

Attachment: shows issues with two separate smart pointers

Comment 6

[Tommy Reilly]

Attachment: v2 this seems to work

Comment 7

[Tommy Reilly]

Attachment: v3 me likey

Comment 8

[Tommy Reilly]

Attachment: v4, incorporates support for GCRef_Union

Comment 9

[Tommy Reilly]

Attachment: Assert when a GCRef or GCRef_Union is used to write to GC memory

Use asserts to fix this issue, I could find no solution that made GCMember not inherit from GCRef that didn't involve fixing hundreds of compile errors in the player (that would have required .staticCast<T>() to downcast things where it isn't needed today).   An assert is almost as good.

Comment 10

[Lars T Hansen]

Comment on attachment 533044 [details] [diff] [review]
Assert when a GCRef or GCRef_Union is used to write to GC memory

I fear we're heading down the wrong path here.  The reality appears to be that GCRef and GCMember are simply two different things, and should not be related by (sub)type.  That C++ cannot express their relationship reasonably well is unfortunate but should that force us to invent a relationship that does not exist?

Comment 11

[Tommy Reilly]

I agree (otherwise I wouldn't have spent days trying to come up with a solution) but I didn't  want to spend even more time changing hundreds of lines of player code to make this change.    The asserts were effective at catching the two places we did this in the code (we  #2782301 and #2869733).

Comment 12

[Lars T Hansen]

Comment on attachment 533044 [details] [diff] [review]
Assert when a GCRef or GCRef_Union is used to write to GC memory

R+ then.

Comment 13

[Tamarin Bot]

changeset: 6338:15cf30169408
user:      Tommy Reilly <[e-mail]>
summary:   Bug 628355 - Assert when a GCRef is used to write to GC memory bypassing write barrier (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/15cf30169408

Comment 14

[Brent Baker]

(In reply to comment #13)
> changeset: 6338:15cf30169408
> user:      Tommy Reilly <[e-mail]>
> summary:   Bug 628355 - Assert when a GCRef is used to write to GC memory
> bypassing write barrier (r=lhansen)
> 
> http://hg.mozilla.org/tamarin-redux/rev/15cf30169408

Reopening bug, patch has not landed in tamarin-redux-serrano for this serrano targeted bug