388070 - MMgc doesn't trace pointers to subobjects of an allocation

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Jason Orendorff [:jorendorff]]

In file GC.cpp, function void GC::MarkItem(GCWorkItem &wi, GCStack<GCWorkItem> &work), lines 2112-2117:

  2112				// make sure this is a pointer to the beginning
  2113
  2114				int itemNum = GCAlloc::GetIndex(block, item);
  2115
  2116				if(block->items + itemNum * block->size != item)
  2117					continue;

Here |item| is a value, found in pointer-containing memory, that might be a pointer to an allocation.  By this point in the loop, we've already confirmed that |item| points into a GCBlock.

What lines 2116-2117 do is *ignore* this pointer if it appears to point into the middle of an allocation.

This will cause us trouble using MMgc for XPCOM, because XPCOM classes often multiple inheritance.

I think replacing lines 2116-2117 with an assignment would be enough:

  2116				item = block->items + itemNum * block->size;
  2117

Not sure how this would affect performance for you, though; maybe an #ifdef would be better.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

This patch causes GC::MarkItem to treat a pointer into an allocation (at an offset) as a reference to that allocation.

It also changes DWB to accept interior pointers.  This costs an extra GC::FindBeginning() call in the slow path.  In the fast path you don't get that far.

I think that's as far as Mozilla needs to go.  The patch doesn't change any other public methods, like GC::GetMark() or GC::IsWhite(), to accept interior pointers.  It doesn't change DRC/DRCWB smart pointers to accept interior pointers either.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2 - same as v1 but using #ifdef MMGC_INTERIOR_PTRS

Here's an updated version that conditionally compiles in interior-pointer support.

(The patch changes the cross-platform build to use MMGC_INTERIOR_PTRS; that could be an option to configure.py instead, if desired.)

Comment 3

[Chris Peyer]

Transfer Bug from Core->Tamarin to Tamarin Product.

Comment 4

[Sergey Yanovich]

jorendorff: In this page http://wiki.mozilla.org/Talk:XPCOMGC , there is a proposal for exact incremental GC concept. There is also a proposal to solve this bug by a 'Visitor' design pattern application.

Comment 5

[Jason Orendorff [:jorendorff]]

Sergey, I don't think this would work.  Casting to (void *) does not convert an interior pointer to a pointer-to-the-full-object.  Try it out.

As far as I know, casting a T* to void* doesn't change any bits of the pointer.  The language requires that casting back to T* should produce the same pointer you started with; i.e. that pointers should "round-trip" to void* and back.

Comment 6

[Benjamin Smedberg]

Actually, if you have a polymorphic object (with a vtable), dynamic_cast<void*> will return a pointer to the most-derived object, without knowing the type and without requiring RTTI.

Comment 7

[Sergey Yanovich]

(In reply to comment #5)
> Sergey, I don't think this would work.  Casting to (void *) does not convert an
> interior pointer to a pointer-to-the-full-object.  Try it out.

I definitely agree with this. But I propose to convert *this* pointer to (void *) which should produce the same result each time. I'll check this out now.

Comment 8

[Sergey Yanovich]

Attachment: test for (void *) this

I am getting this output:

$ ./test 
i=804a008, a=804a008, b=804a00c, c=804a008, xa=804a008, xb=804a008
A
B

Comment 9

[Jason Orendorff [:jorendorff]]

Oh, OK, my apologies -- I just didn't see what you meant.  Yes, that part will work fine.  There are other parts of your GC scheme that I don't understand, though (see comments on the wiki page).

Comment 10

[Edwin Smith]

Comment on attachment 281363 [details] [diff] [review]
v2 - same as v1 but using #ifdef MMGC_INTERIOR_PTRS

Nothing obviously wrong with the "#else" code, or the new MMGC_INTERIOR_PTRS code.  good to get treilly to review as well.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to comment #10)
Thanks very much, Ed.  I will wait for treilly's r+ before checking this in, unless I hear otherwise.

Brendan asked me to check with you that we really want this in an #ifdef.  I'd be happy either way.  Code without #ifdefs gets better testing and is easier to maintain; but I think given the uncertainty, it might also make sense to check in with the #ifdef, then do measurements and see if it's OK to remove it.

Comment 12

[Jason Orendorff [:jorendorff]]

I think I want to make another change to this patch before checking it in.

v2 of the patch inserts an additional FindBeginning() call into the write barrier.  Now I think it should be the mutator's responsibility to produce a pointer to the beginning of the referent.  So that part of the patch should be dropped.

With this change, DWB() would *not* support interior pointers, even in an MMGC_INTERIOR_PTRS build.  Our nsCOMPtr smart pointers would need a slightly different write barrier than DWB() provides, but that's a good thing-- only COM pointers would incur the overhead of the extra FindBeginning call.

And I think we can avoid calling FindBeginning() even from nsCOMPtr. Instead of using MMgc's bookkeeping to find the beginning of the object, we can use dynamic_cast<void *>.

Comment 13

[Tommy Reilly]

(In reply to comment #12)

Sounds good Jason, I'll keep my eye out for v3 and review that?  In response to Brendan's plea to go w/o ifdefs I generally agree but don't want to introduce unnecessary changes into how the flash player works.  It might not introduce any problems but the amount of testing required to make that assumption would be large.  Instead I'd like to think about having a dynamic per-object mechanism for allowing interior pointers so we can support the functionality w/o ifdefs.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: v3

Here's the updated patch.  Sorry for the delay--MMGC_THREADSAFE work intervened.

Comment 15

[Jason Orendorff [:jorendorff]]

Pushed change a4259e008469 to tamarin-central.

treilly:  Instead of #ifdef-ing, we could make this a runtime option, "bool interiorPointers;", false by default.  The performance cost for Flash Player should be really tiny: the cost of "if (!interiorPointers)" times the rate of false positives that look like interior pointers.  I'll file a new bug if there's any interest.

Comment 16

[Tommy Reilly]

You're probably right but I'd rather not rely on branch prediction for such a super critical hot spot.