663491 - GCRoot: expose a data member for use by offsetof in a static assert

Status: RESOLVED WONTFIX

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

Description

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

Bug 663159 points out that multiple inheritance (MI) of GCRoot (or its subclasses) can break the gc terribly, in particular by violating the invariant that the GCRoot needs to know where its object starts (which is stored in its 'object' member).

The comments there include discussion of both:
- solutions to remove MI, (and perhaps GCRoot entirely) and
- safeguards to check that the value inferred to be used for 'object' actually does match the beginning of the object

One idea, discussed in Bug 663159, comment 13, is to use a STATIC_ASSERT to inspect that the GCRoot is laid out at the beginning of the object.  But to actually realize this, we need to use the offsetof macro to inspect a data member of GCRoot, which means we need to expose a data member to be inspected.

That's the motivation for this bug.

Comment 1

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

Attachment: expose gc member of GCRoot (as const to eschew modification)

This is a truly nose-pinch worthy patch.  :(

But look at it this way: the sooner we fix Bug 663159, the sooner we can back this change back out again.

Comment 2

[Steven Johnson]

Comment on attachment 538575 [details] [diff] [review]
expose gc member of GCRoot (as const to eschew modification)

F+, with similarly held nose, but may I suggest renaming "gc" to something more unique, on the possibility that some subclass may also define a member with this name...

Comment 3

[Lars T Hansen]

Comment on attachment 538575 [details] [diff] [review]
expose gc member of GCRoot (as const to eschew modification)

That's pretty gross.

Comment 4

[Tommy Reilly]

Comment on attachment 538575 [details] [diff] [review]
expose gc member of GCRoot (as const to eschew modification)

Why do we need this?  If we have FixedMalloc FindBeginning we don't need this right and the STATIC_ASSERT can go in GCRoot's ctor.   Please don't tell me we're seriously considering sprinkling STATIC_ASSERTS aren't the code base.  We can and should get the checking safely within the confines of MMgc.

Comment 5

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

(In reply to comment #4)
> Comment on attachment 538575 [details] [diff] [review] [review]
> expose gc member of GCRoot (as const to eschew modification)
> 
> Why do we need this?  If we have FixedMalloc FindBeginning we don't need
> this right and the STATIC_ASSERT can go in GCRoot's ctor.

A dynamic assert based upon FindBeginning could go into the GCRoot ctor, but not a static one.

See Bug 663159, comment 12: Steven wanted compile-time assertions if he was going to accept the solution of moving the GCRoot base class to the front of the base class list.

> Please don't
> tell me we're seriously considering sprinkling STATIC_ASSERTS aren't the
> code base.  We can and should get the checking safely within the confines of
> MMgc.

I was indeed seriously considering sprinkling STATIC_ASSERTs within the code base, temporarily, until we could remove GCRoot entirely (or enforce that it be solely singly-inherited).

The other options I see here (let the above be named option 1.), are:

2. Trust that the Debug-mode dynamic assertions (planned to be added to GCRoot as part of Bug 663177) actually serve as checks on Release-mode as well.  I actually would be fine with this, I have some difficulty believing that one of our current compilers will select a different class layout for Release-mode versus Debug-mode.

3. Switch to using FixedMalloc::FindBeginning unconditionally in the GCRoot constructor that is attempting to derive the object's start from 'this'; that is, instead of checking that the found beginning matches 'this' solely in Debug mode, do the computation and use the found beginning in Debug *and* Release modes.  We would need to expose FindBeginning to the Release-mode code, and incur the time hit accordingly in the GCRoot construction.  This is what Steven originally wanted to do; I pushed back because I fear the performance impact, but maybe we need to live with such compromises.

4. Hypothetical: Figure out some way to get the GCRoot::operator new to communicate the allocated address forward to the GCRoot constructor in some way.

Comment 6

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

(In reply to comment #5)

> 4. Hypothetical: Figure out some way to get the GCRoot::operator new to
> communicate the allocated address forward to the GCRoot constructor in some
> way.

5. (a variant of 4, but not hypothetical): Revise constructors for all subclasses of GCRoot so that they are all split into two variants: a 'protected' constructor that takes an explicit 'object' argument, and a 'private' constructor that uses the 'this' pointer to fill in the 'object' argument when calling its parent class's protected constructor.  No public constructors allowed for extensions of GCRoot.  Then apply the same create() static method pattern we've been using for the extractly traced clases.

I find this ugly and distasteful, and probably too much churn to actually implement for Serrano.  But it will have better runtime efficiency than 3, and there's no question about whether the C++ compiler will be messing things up behind our back.

Comment 7

[Tommy Reilly]

I think for now the best solution is #3.  In fact the call to FixedMalloc::Size and FindBeginning could be collapsed, ie:

    GCRoot::GCRoot(GC * _gc)
    { 
        void *mem;
        size_t size;
        FixedMalloc::GetAllocationInfoFromDerivedPointer(this, mem, size);
        init(_gc, mem, size, false, false);
    }

a) I think we can do it efficiently and b) I think performance doesn't matter here

I think any effort spent mucking with the Flash code base should be focused on exactly tracing roots and eradicating MI.   With this fix in place we don't even need to worry about implementing the GCRoot must be first hack.

Comment 8

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

(In reply to comment #7)
> I think for now the best solution is #3.  In fact the call to
> FixedMalloc::Size and FindBeginning could be collapsed, ie:
> 
>     GCRoot::GCRoot(GC * _gc)
>     { 
>         void *mem;
>         size_t size;
>         FixedMalloc::GetAllocationInfoFromDerivedPointer(this, mem, size);
>         init(_gc, mem, size, false, false);
>     }
> 
> a) I think we can do it efficiently and b) I think performance doesn't
> matter here
> 
> I think any effort spent mucking with the Flash code base should be focused
> on exactly tracing roots and eradicating MI.   With this fix in place we
> don't even need to worry about implementing the GCRoot must be first hack.

Okay, I assume Steven's already on board for this since it was his idea, so unless Lars voices an objection, I will start down this path.

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > I think for now the best solution is #3.  In fact the call to
> > FixedMalloc::Size and FindBeginning could be collapsed, ie:
> > 
> >     GCRoot::GCRoot(GC * _gc)
> >     { 
> >         void *mem;
> >         size_t size;
> >         FixedMalloc::GetAllocationInfoFromDerivedPointer(this, mem, size);
> >         init(_gc, mem, size, false, false);
> >     }
> > 
> > a) I think we can do it efficiently and b) I think performance doesn't
> > matter here
> > 
> > I think any effort spent mucking with the Flash code base should be focused
> > on exactly tracing roots and eradicating MI.   With this fix in place we
> > don't even need to worry about implementing the GCRoot must be first hack.
> 
> Okay, I assume Steven's already on board for this since it was his idea, so
> unless Lars voices an objection, I will start down this path.

Filed as Bug 664137.

Comment 10

[Steven Johnson]

> Please don't tell me we're seriously considering sprinkling STATIC_ASSERTS aren't the
> code base

I proposed this as a band-aid on the assumption the problem was localized to a small number of problem spots. Felix' investigation makes it clear this would be inadequate.

Comment 11

[Steven Johnson]

> We would need to expose FindBeginning to the Release-mode code, and incur the time hit accordingly 
> in the GCRoot construction.  This is what Steven originally wanted to do; I pushed back because I 
> fear the performance impact, but maybe we need to live with such compromises.

Crowder's Law definitely applies here ("If the answer doesn't have to be correct, the code can be made arbitrarily fast").