565664 - mmgc: aggressive defense against client c++ coding errors

Status: RESOLVED WONTFIX

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

Description

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

Tommy and I were just discussing bugs that he found recently and started throwing around ideas of how we could try to root out usage errors in the C++ source code of mmgc clients.

Such detection would preferably happen statically; failing that it would happen dynamically in all builds;  finally if necessary only some checks would only occur in a hypothetical pedantic mode selected at build time.

I wrote some early ideas below, but I think I want this bug to be more of a gathering point for similar ideas, as well as a tracker for individual tickets if someone decides to try implementing one of the ideas.

----

As an example of one option a pedantic mode could provide: the DRC(_type) macro could be changed to emit cookie values after (or around?) the emitted field, and then the collector could check that these cookies are present when it sees a pointer to a managed object as it does its scanning.  This way we would catch cases where the DRC() macro was omitted, at the cost of space (larger objects due to the emitted cookies) and time (collector is traversing more state).

Another option that Tommy suggested would be: when an object's refcount hits zero, instead of freeing it, just poison its state.  This would catch cases where the client code is getting away with having a dangling pointer that ends up as an alias for later allocated state later because the allocator happened to give it back the same block of memory and it was initialized to the same kind of object.  (Tommy points out that this occurs with relatively high probability due to how we group blocks by size classes; there might be other ways to attack this problem that are not as extreme with respect to potential space overhead.)

Comment 1

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

(Is there some way to use C++ template and type tricks to get some static checking that the DRC macro is present?  I don't see how, unless we first introduced a requirement that all member field definitions be adorned with some macro, ie you choose either DRC(Foo*) or NAKED(Foo*), but never just Foo*.  Then we might be able to get somewhere, but its probably not worth the headache.)

Comment 2

[Lars T Hansen]

Also keep in mind that we're using more compilers all the time and that the number of quirks we must deal with is growing, and will likely continue to grow.  So a solution that does not depend on hard-to-reach corners of C++ is probably preferable over one that does.

Comment 3

[Lars T Hansen]

Another possibility I've pondered is representing pointers to garbage collected storage not as raw pointers but as small structs that are passed around:

  template class GCRef<class T> { ... }

These would have value semantics.  Since they are not pointers, it would enforce the declaration of fields in classes as GCRef<T>.  Missing write barriers would therefore be a non-issue if this API is used.  Internally the VM might use a lighter weight mechanism, but the cost of passing structs around would probably be OK for other uses.  It really need take no more space and time than currently, but whereas we now have the DRC(P*) style which can be abused as P*, requiring the correct type on both the left and right hand side of an assignment would create a synchronization point that would allow problems to be caught at compile time.

Comment 4

[Steven Johnson]

(In reply to comment #3)
> Another possibility I've pondered is representing pointers to garbage collected
> storage not as raw pointers but as small structs that are passed around:

I like it, but we would want to reality-check the lamer compilers we have to support to ensure they will always registerize the structs when being passed/returned as args. (Don't laugh, I've seen compilers that always flush structs to stack memory regardless of size...)

Comment 5

[Lars T Hansen]

(In reply to comment #4)
> (In reply to comment #3)
> > Another possibility I've pondered is representing pointers to garbage collected
> > storage not as raw pointers but as small structs that are passed around:
> 
> I like it, but we would want to reality-check the lamer compilers we have to
> support to ensure they will always registerize the structs when being
> passed/returned as args. (Don't laugh, I've seen compilers that always flush
> structs to stack memory regardless of size...)

I was actually proposing this assuming that most compilers would do this lamely; if the use of GC'd storage matters that much then (a) maybe it's being used inappropriately or (b) special APIs to optimize access can be provided.

(Again, I'd expect the VM to use more direct APIs.)

Comment 6

[Brent Getlin]

From a client API standpoint, I like Lars' recommendation of using a templated storage object to protect access to GC memory.  In fact, our existing WriteBarrier and WriteBarrierRC implementation already does something similar.  It seems like a first step to simplifying usage would be to unify naming and syntax using separate localized definitions of WriteBarrier and WriteBarrierRC and commonly naming those definitions "GCRef" or similar.  The same usage syntax could also be implemented for GCRoot (and possibly a global GCRef definition for stack usage).

For Example:

class GCObject
{
    template <class T> class  GCRef
    {
        //  Move WriteBarrier implementation here.
    }

    friend GCRef;
    friend GCFinalizedObject::GCRef;   
    friend GCRoot::GCRef;
    private:
        //  Lock access to GCObject ptr through GCRef 
        operator=
        operator() 
}

class GCFinalizedObject
{
    template <class T> class  GCRef
    {
        //  Move WriteBarrier and WriteBarrierRC implementation here.
        //  If IsRCObject(), then use WriteBarierRC implementation
    }
    friend GCRef;
    friend GCFinalizedObject::GCRef;   
    friend GCRoot::GCRef;
    private:
        //  Lock access to GCObject ptr through GCRef 
        operator=
        operator() 
}

class GCRoot
{
    template <class T> class  GCRef
    {
        //  This GCRef Wouldn't use WriteBarriers, but acts to grant access to GCObject, etc.
    }
}

Comment 7

[Steven Johnson]

(In reply to comment #6)
> It seems like a first step to simplifying usage would be to unify naming and
> syntax using separate localized definitions of WriteBarrier and WriteBarrierRC
> and commonly naming those definitions "GCRef" or similar. 

Alternately... we could adapt the type-sniffing code from List<> and have a single GCRef<> type that specializes automatically to the proper behavior (WB vs WBRC).

Comment 8

[Brent Getlin]

Attachment: SafeGC Infrastructure

Comment 9

[Edwin Smith]

Any chance you could upload this as a unified diff? (hg diff does this by default, or use diff -u, or p4 diff -du).  Then, the HTML patch viewer will produce nice side-by-side diffs.

Comment 10

[Lars T Hansen]

Comment on attachment 471970 [details] [diff] [review]
SafeGC Infrastructure

A general comment is that GCRef/GCMemberRef should be isolated in individual header files as much as possible.

A style comment is that TAB characters are verboten in MMgc files (we have some issues with them creeping back in but we're trying to keep them out).

Comment 11

[Brent Getlin]

Attachment: cleaned up, unified patch

Created unified patch with tab spaces removed and GCRef/GCFactory separated out into separate header files

Comment 12

[Lars T Hansen]

Comment on attachment 472813 [details] [diff] [review]
cleaned up, unified patch

This seems OK as far as it goes, pretty clean code all things considered.

I'm worried about churn in the VM rewriting all allocations in terms of GCFactoryRawPtr but it seems manageable, provided it's necessary.  That it's actually necessary is not quite obvious yet - it ought to be possible to provide global 'new' operator definitions to the VM code alone by defining them in a header file included in every VM .cpp file, say.  I'm not likely to OK a rewrite until we've discussed plausible alternatives, but if a rewrite is necessary then what you have here is basically OK with me.

(The usual mix of bug reports, nits, suggestions, misunderstandings, and sarcasms follows.)

GCReference.h:

In the implementation of IsAddressOnStack, using 'stackTop' as the variable name for something that is the stack base is just plain wrong and leads us into the same mess we've been trying to get out of.  The stack top is the location where the next pushed value will go, or the next popped value will come from; the stack base (or stack bottom if you like) is the other end.

If GCReference.h defines only GCRef then it should be called GCRef.h.

I'd be interested in having a #define controlling the availability of Delete() the way you control the availability of ptr().  Explicitly deleting managed storage is an evil thing, and we will need to phase it out.  Might as well start now.

GCMemberRef and GCRef still need to be elsewhere than GCObject.h.

You could simplify

  if (!bShouldBeOnStack)
    GCAssert(!IsAddressOnStack(&this->t));
  else
    GCAssert(IsAddressOnStack(&this->t));

as

  bShouldBeOnStack == IsAddressOnStack(&this->t)


GCFactory.h:

This is not quite a sentence: "The second template parameter automagically whether the object is RCObject or not."

The legal values of the parameter N of GCFactory are not documented anywhere.

Would encourage all variants of New and NewWithExtra with up to 10 parameters to be pre-defined.

You need a Calloc API to GCFactory because Alloc() is susceptible to overflowing its size computation (easy security hole); the existing Calloc handles a safe computation efficiently.  In fact, I would probably be in favor of getting rid of the Alloc API and only supporting Calloc, we can have a fast path for '1' element.

Note Alloc and Calloc /must/ support allocation flags (mandatory flags arg == good, default flags arg == bad) supporting only PtrZero as now is unsatisfactory for a large number of use cases and will completely break down with exact tracing.

How about renaming GCFactoryRawPtr as simply GCFactoryRaw?  It seems your thinking is that this will be used everywhere inside the VM, and short descriptive names are better than long descriptive names.

GC.h:

I'm not fond of DecrementRef and IncrementRef being open-coded in GCMemberRef inside GCRoot, to say the least - we'll want a little abstraction here, probably not hard to provide.

GCObject.h:

REALLY_INLINE void *operator new should just call GCHeap::Abort or similar function, not return some random value that may or may not be a valid heap address.

I'm sure I don't understand why there has to be multiple GCMemberRef definitions and why the GCMemberRefBase and GCMemberRef definitions are in GCObject.h.  Clarification on this point would be welcome.

The flag 'isRCObject' is going to be the wrong thing if there are more base classes than we have now.  A third is coming with the exact marking rewrite; we've also discussed that it's not ideal that all RC-objects are pointer-containing, and we're discussing whether pointer-containedness needs to be expressed in the type hierarchy (it's desirable to do so from the exact tracing point of view).  Suggest you consider alternative namings that admit more than a binary choice.

Comment 13

[Brent Getlin]

Attachment: Updated 'SafeGC' infrastructure

Comment 14

[Brent Getlin]

Attachment: Updated 'SafeGC' infrastructure

Significant Changes:
-> Add new factory methods for 'AllocFinalizedObject'
-> convert 'magic number' template argument to typedef enum
-> add "reinterpretCast" method on GCRef for direct casting
-> remove privatization of operator& to enable usage of pointers to GCRef/GCMemberRef

Comment 15

[Brent Getlin]

Attachment: Updated Safe GC - response to comments from Lars

Minor updates in response to comments from Lars (2010-09-16).
-> Rename GCReference.h -> GCRef.h
-> A few variable name, comment, optimization cleanup items
-> Removal of GCFactory::Alloc

Comment 16

[Brent Getlin]

In response to Lars' comments from 2010-09-16, most items were addressed in patch 476898. 

RE : Using GCFactoryRawPtr throughout the VM
[bg] I don't suggest rewrite all VM allocations using this utility at this point. This utility is designed to be used in player/shell source that implements/invokes avmplus methods that require raw pointers or in avmplus headers that are directly included in player/shell projects. 

RE: All variations up to 10 params
[bg] I'm not opposed to this, but would prefer to add these on an as-needed basis in the short term.

RE: Alloc and Calloc.  
[bg] For now, I've removed Alloc.  When we find a need to add "Calloc" (during integration) I will make sure it's added

RE: Renaming GCFactoryRawPtr
[bg] I'm not against renaming GCFactoryRawPtr to something else, but I do prefer the descriptiveness of RawPtr vs Raw.  Since I'm not proposing this is everywhere in the VM (see first item) , perhaps the slightly longer syntax is ok?


RE: Abstraction of IncrementRef/DecrementRef 
[bg] I'm not sure why what's there is bad, can you elaborate?

RE: 'isRCObject' 
[bg] This has been converted to gcObjectType in patch 476424.  I assume this relieves your concern here?

Comment 17

[Brent Getlin]

Attachment: Updated 'Safe GC' infrastructure

Significant changes from previous patch:

* Added GCRef_Union class to handle GC object declarations within unions.  (This class does not declare constructors, so no defaulting GC pointers to NULL and no IsAddressOnStack checks)

* Fix a number of methods for const correctness

* Fix GCFactory for RC objects so that they don't automatically increment ref-count

* avoid using temporaries in GCFactory methods by explicitly defining "mem" variable to hold placement-new mem.

*  Enable ability to utilize references to GCObjects as function parameters (via the operator* on a GCRef<T>).

*  Remove ability to get pointers to GCObjects.

Comment 18

[Brent Getlin]

Please note comments, precompiler definitions, and macro definitions  on lines 44 through 125 of GCRef.h, used for incremental integration of this mechanism and for calling out potentially unsafe usage of the garbage collector.

Comment 19

[Lars T Hansen]

Comment on attachment 481097 [details] [diff] [review]
Updated 'Safe GC' infrastructure

(First set of comments, to be completed tomorrow.)


General:

There are tabs in this patch, please avoid them, they make understanding the patch difficult.  (And never commit them.)


Objections:

The SetGCToNull changes do not belong in this patch, probably (GC.h).

GCMemberRefGCBase needs to move out of GCObject.h and into GCRef.h, IMO.  Quoting myself from my previous review:

  I'm sure I don't understand why there has to be multiple GCMemberRef
  definitions and why the GCMemberRefBase and GCMemberRef definitions are in
  GCObject.h.  Clarification on this point would be welcome.

I don't think any of those points were adequately addressed later (neither in prose nor code), and I have the same concerns now.

"GCREF CHANGE" comments don't belong in these files, probably - is there some way of avoiding that clutter?  Ditto "GCREF_CHANGE".

The use of "GCAssert(false);" is an antipattern because it does not provide any information about the problem without digging into the code and rerunning the program, hoping that the problem can be reproduced.  I prefer to use 
"!" before a string: GCAssert(!"Global operator new(size_t,GC*) should not be called").  Or use GCAssertMsg.



Nits:

GC.h:

Some random insertion of blank lines should be reverted.  (Those lines would come in handy around the definition of gcObjectType in GCObject.h, for example.)

Style: for functions as small as those in GCRoot::GCMemberRef, there is no need to proliferate #ifdef DEBUG like done here, instead there should be multiple versions of the methods, one within #ifdef DEBUG, another without.  A single #ifdef DEBUG block is probably sufficient.  (There are other approaches, eg, using a macro that does or does not insert an argument, and where the macro is used unconditionally but defined conditionally, such as:

  #ifdef DEBUG
    #define DEBUG_ARG(x) x
  #else
    #define DEBUG_ARG(x)
  #endif

With

  GCMemberRef() : GCRef<T>(DEBUG_ARG(false))

GCObject.h:

Trailing blank lines in the class body of GCObject::GCMemberRef.

Pointless comments in GCMemberRefGCBase should be removed or replaced with useful comments.  For 'set', the comment says the method "handles the logic" but not what the logic might be or why the method would handle it.  For the constructor, which is protected, the comment says that it is protected so that only subclasses can use it.  That's tautological; what we want is a comment that explains why only subclasses should be able to use it.  For the first operator= the comment says "Allow Any GCRef to be assigned to this GCMemberRefGCBase", but that's obvious from the signature.  For the second operator= the comment is informative but misleading, since the GCAssert will ensure that the method isn't "mostly" for setting it to NULL, it is /only/ for setting it to NULL.  It might be useful to know why the Clear() method is insufficient for that.

"Make sure reset the WriteBarrier... " is ungrammatical.

Comment 20

[Brent Getlin]

RE: Moving GCMemberRefGCBase out of GCObject.h and into GCRef.h
-> Since GCMemberRefGCBase is ONLY valid for GCObject types, and these object types are all defined in GCObject.h (GCObject, GCFinalizedObject, RCObject ) this seems like the best spot for this definition.  Since this definition is tightly related to GCRef, I see how it "could" also be placed there, but I have a slight preference to where it is now.  

RE: Why there are multiple GCMemberRef definitions
-> Localizing the definition of GCMemberRef to only the scope of GCObject , GCFinalizedObject, and GCRoot (via inner class definitions on each of the three) means GCMemberRef only be used with those classes, otherwise you get compile errors. This is an important feature of the "safer gc".  Additionally, even though the usage syntax is is standardized across all three separate definitions, the logic of GCMemberRef is different based on where it's used.  If, for example, it's used in the scope of GCRoot, WriteBarriers are not used.  If we only had one global definition of GCMemberRef, the container class would need to be dynamically determined, which would be a costly operation.

I will address the remainder of Lars' comments in the next patch...

Comment 21

[Brent Getlin]

RE: gcRefInstance.Clear() vs. gcRefInstance = (T*)NULL;
->  We had some discussion some time ago about which way is the "preferred" way to "set a GC variable to NULL", and we ended up with a solution that supports both "Clear" and "assign to pointer", which is exactly what is supported today.  Do you suggest we remove completely remove one of these syntax features in favor of the other?

Comment 22

[Tommy Reilly]

Allowing any GCMemberRef to be assigned any GCRef seems too loose, really we want T to be T2 or a superclass right?

At least it should be a compile error to stuff a GCRef to an RCObject into a GCMemberRef of a non-RC type and vice versa.

> //  GCFactory creates GCRefs

really a GCFactory creates GC objects and returns GCRefs

Why GCFactoryRaw?   Shouldn't the avmplus core just use raw new?   We can hide raw GC new from the player and make it available to the core.

> GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT

Hard to read, precedence is to use an underscore between words we should fix this if only for things that won't go away anytime soon.

Why does GCRoot's GCMemberRef need rc vs. non-rc specialization if we
added noop-inc/dec functions to GCObject?

We should stipulate that GCRef_Union isn't safe and it will go away and its just a temporary crutch, maybe we should just leave it out of tamarin as a forceful reminder?

Comment 23

[Tommy Reilly]

Comment on attachment 481097 [details] [diff] [review]
Updated 'Safe GC' infrastructure

review minus because the comments need to be addressed and the formatting needs to be fixed and SetGCToNull needs to go etc.   Want help cleaning up this patch?  I could do it pretty quickly I think...

Comment 24

[Tommy Reilly]

Attachment: Cleaned up version of Brent's patch

Only change I made was to comment out GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT which breaks StringObject.  Won't ask for reviews until I get shell integration done.

Comment 25

[Tommy Reilly]

Brent added this to STring to fix the problem:

	public:
		//  Define operator& public here to get around private operator& in GCFinalizeObject for GCRef security
		REALLY_INLINE String * operator&(){return this;}

Instead I'm gonna try a solution where things like GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT are defined for shell/player code but not defined for core code.

Comment 26

[Steven Johnson]

(In reply to comment #25)
>         REALLY_INLINE String * operator&(){return this;}

Nooooooooooooooo! Overloading operator& is a recipe for horrible pain. I don't care what the justification is, this is a bad bad bad idea.

Comment 27

[Brent Getlin]

Why is it a recipe for horrible pain?

Comment 28

[Steven Johnson]

(In reply to comment #27)
> Why is it a recipe for horrible pain?

I'm probably overreacting here, due a horrible misuse of this I encountered years ago, with the most horrible written C++ string class I've ever seen.

If we're overloading only to privatize the operator (and then re-publicize it elsewhere) that is probably hard to screw up.

Comment 29

[Brent Getlin]

Regarding Tom's comments from 2010-10-13:

>Allowing any GCMemberRef to be assigned any GCRef seems too loose, really we want T to be T2 or a superclass right?
> At least it should be a compile error to stuff a GCRef to an RCObject into a GCMemberRef of a non-RC type and vice versa.

[bg] Yes that's the way it works.  Implicit casting to non superclasses will throw a compiler error.  There's also a "reinterpretCast" option that allows the dev to explicitly cast to something "known" by the developer.

> //  GCFactory creates GCRefs
> really a GCFactory creates GC objects and returns GCRefs
[bg] Changing this comment sounds fine

> Why GCFactoryRaw?   Shouldn't the avmplus core just use raw new?   We can hide raw GC new from the player and make it available to the core.

[bg] This is for "shell/consumer" code to use for avmplus interface code (like "createInstance").  Also this is so avmplus header files that create raw pointers can be included in "shell/consumer" code without tripping the "you cannot use new" alarms.

> GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT
> Hard to read, precedence is to use an underscore between words we should fix this if only for things that won't go away anytime soon.
[bg] Changing this to GCREF_NO_ADDRESS_OPERATOR_GCFINALIZEDOBJECT sounds fine. Likewise changing GCREF_NO_ADDRESS_OPERATOR_GCOBJECT…

> Why does GCRoot's GCMemberRef need rc vs. non-rc specialization if we added noop-inc/dec functions to GCObject?
[bg] Good point.

We should stipulate that GCRef_Union isn't safe and it will go away and its just a temporary crutch, maybe we should just leave it out of tamarin as a forceful reminder?
[bg] Is it temporary?  We'd have to say "you can't use unions with GC objects" if it's not temporary.

Comment 30

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

Comment on attachment 484818 [details] [diff] [review]
Cleaned up version of Brent's patch

Here are my comments from reading over the patch.  I'll wait to settle
the "r?" question until after I address my first point (below), but you
might as well get this feedback now:

Do we have selftests?  If not, and assuming that it still makes sense
to test this functionality (I think it does), I think I might make
some selftests (I've been meaning to play with this stuff and a
selftest seems like the right place to do it).

nit: I find "Removed allowance of <verb>ing ..." questionable english;
Please replace with e.g. "Removed capability to <verb> ..."

On GCREF_RETURNMEMBER(type, var) { ... }
I would slightly prefer that it use the pattern:
   GCREF_RETURNMEMBER(type, var) do { ... } while (0)
as that would interoperate better with a semicolon delimiter at the
point of invocation (see also
http://stackoverflow.com/questions/154136/why-are-there-sometimes-meaningless-dowhile-and-ifelse-statements-in-cc-macros
)

This phrase occurs a couple of times: "Ultimately, we'll probably add
some utility class to replace this macro."  Should this include a
bugzilla bug number so a reader can followup on future events?

nit: in all three occurrences of the following note: "additional debug
parameter is for checking isAddressOnStack()": its actually checking
the predicate: !isAddressOnStack().  Adding the exclamation point to
the comment might save some head scratching.

"GCFactory creates GCRefs" -- weren't you complaining about this
choice of language in an earlier comment; thought it should be
"creates GCObjects and returns GCRefs" ?

Above GCFactory: "The second template parameter automagically whether
the object is RCObject or not" is missing a verb. See also similar
issues above GCFactoryRawPtr.

Above GCFactory: "If the user attepts to specify a NON-RCObject as
true here, will fail to compile." what does "specify as true" mean
here?  Do you mean to describe an attempt to instantiate GCFactory
without supplying the second template parameter?  See also similar
above GCFactoryRawPtr.

Nit: The P3-overloaded GCFactory::New variant has been placed in
between the P3- and P4-variants rather than coming after the
P5-variant; moving it to after P5-variant would make the code more
uniform in appearance.  Likewise the P4-variant of NewWithExtra seems
like it was inserted in an ad-hoc fashion.

You pointed out that "This line of code causes a compile error if the
user attempts to set the wrong "gcObjectType" value for primitives"
in GCFactory<T, kTypeRCObject>; it stands to reason that a similar
note belongs above the analagous assertions in
GCFactory<T, kTypeGCFinalizedObject>

typo: "implemenation"

typo: "invetigage"

typo: "devloper"

typo: "GCMeberRef"

typo: "subclasses don't think they're destructor will get called"
(should be "their")

Comment 31

[Lars T Hansen]

Comment on attachment 484818 [details] [diff] [review]
Cleaned up version of Brent's patch

SR- for 

General:

There is no documentation on how this is supposed to be used, or how
it's supposed to hang together.  (Wiki docs do not count, partly
because Tamarin is open-source and zerowing is internal to the
corporation, partly because wiki docs are too far away from the code
to be reliable and evolve with the code.)

I think studying an extended usage example (eg the shell code) will 
be required in order to have confidence in the code, even after the
objections listed below are corrected.


Functionality / substantive:

GCRef.h:

Names like GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT are unreadable (which
I consider a problem with functionality, not mere style) and need to be
fixed; GCREF_NO_ADDRESS_OPERATOR_GCFinalizedObject might work, though a fix
will probably run into a 31-character preprocessor limit on some
compilers.

(As proof that it's unreadable, observe that the spelling actually is
GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT but should be
GCREF_NOADDRESSOPERATORGCFINALIZEDOBJECT and that I accidentaly
corrected it in the rewrite, where the error becomes obvious.  Ditto
for GCREF_NOADDRESOPERATORGCOBJECT.  This kind of error will be tricky
to catch sometimes since it's just an #ifdef / #ifndef - the lack of
redundancy prevents the compiler from flagging obvious errors.)

Even then the name appears to be misleading;
GCREF_NO_ADDRESS_OF_GCFinalizedObject would be more correct (the
effect is to privatize the operator).

Avoiding negative #define names is an important goal, so
GCREF_PRIVATE_ADDRESS_OPERATOR_GCFinalizedObject might be even better,
and actually states the same thing as the comment documenting the
#define.

(I will continue to give this R- / SR- until most or all negative
#defines are gone unless I can be convinced that it's for the greater
good to keep them the way they are.)

Some of the macros, like GCREF_UNPROTECTEDMEMBER, makes me question
whether the GCRef machinery lives up to its billing by making the API
simpler.  (Also it's not at all clear what an "unprotected" member is.)

How important is it to use eg GCREF_CASTFROMVOID and
GCREF_AVMPLUSRAWPTR etc?  Since they have straightforward expansions
it would be astonishing if code did not bypass these and go straight
to the expansion.  That undermines the value of the safegc work.  The
comment says, "Ultimately, we'll probably add some utility class to
replace this macro."  IMO that needs to happen sooner rather than
later.

GCRef_Union seems to defeat the purpose of the safegc work.
Furthermore, its documentation is not good - a usage example and
tighter prose would serve it well.  (Also GCRef_Union is not an ideal
name, GCRef_UnsafeNobarrierNoRC more directly expresses what it does.)

This is wrong in both GCRef and GCRef_Union.

  // lock this up.  We don't want GCRefs to automatically convert into
  // generic ptr types (like Atom).
  REALLY_INLINE operator int32_t() const ;

The reason it's wrong is because Atom is intptr_t, not int32_t, though
that is anyway secret.  It might be OK to use avmplus::Atom here if
that conversion is the only one you worry about.  (The exactgc work
exposes the Atom type to the GC.)  If Atom is not the only one you
worry about then more need to be added here.


GCFactory.h:

I see no reason not to provide NewWithExtra in all cases where New is
provided.  This is true for several other factories too, and sometimes
NewWithExtra is provided but New is not.  It all seems completely
arbitrary.

GCFactoryRawPtr specialized for kTypeGCFinalizedObject is the only one
that has an Alloc method.  First, this is strange - one more example
of ad-hoc method suites on these factories.  Second, this should
almost certainly be a Calloc method because only having an Alloc
method forces a client that wants to allocate an array to compute the
size of the array and that code will always be prone to overflow, thus
opening a security issue; Calloc gets the computation right always.

GC.h:

This comment (actually two of them) in GCMemberRef specialized for
kTypeRCObject seems wrong/meaningless, as the code is specialized for
RCObject:

  //  This is NOOP for GCObject and GCFinalizedObject

GCObject.h:

There's an #ifdef GCREF_RAWPTRASSIGNMENT here (actually at least two)
but the name used in GCRef.h is instead GCREF_NORAWPTRASSIGNMENT.



Stylistic, both major and minor:

GCRef.h:

8-spaces-indent must be fixed; standard indentation is 4.

There are extraneous blank lines in this file: at the beginning and
end of function bodies (IsAddressOnStack); in operator=; near the end
of the file.

The use of the word "allowance" several places in comments is unusual
English (but then I'm not a native speaker).

GCREF_TEMPCONVERT is not a useful name.

This does not make sense to me, which MACROS are being referenced
here?  What does "OFF" mean?

  // Full Safe-GC implementation is ON when the definitions are ALL
  // DEFINED and the MACROS are ALL OFF

Comments of this form are not helpful when it's evident from the code
that that's what's going on:

  //  Forward Declare the GCFactory class

  //  GCRef Definition

  //  Grant friend access to all types of GCRefs

Contrast that with this, which documents intent and not mechanism:

  // Grant friend access to the GCRefFactory so that it can create/set
  // the raw GC memory

There's a mix of #if !defined and #ifndef, for no apparent reason.

Which "debug only parameter"?  (The method is #ifdef DEBUG.)

  //  The additional debug only parameter is used by GCMemberRef
  explicit REALLY_INLINE GCRef(bool bShouldBeOnStack) : t(0)
 

GCFactory.h:

The comment above GCFactory does not scan, ditto above
GCFactory::New(gc).  Ditto above GCFactoryRawPtr.  And above its New
method.

The comment above GCFactoryRawPtr specialized for RCObject says
"GCFactory", should say "GCFactoryRawPtr".


GC.h:

Bad indentation in GCMemberRef::set (RCObject case).

A lot of the comments on GCRoot::GCMemberRef are about mechanism and
are thus obvious from the code; they add nothing.


GCObject.h:

This is not pretty: if you mean NULL, say NULL:

   ~GCMemberRefGCBase()
   {
     if (this->t)
     {
       set(0);
     }

Lots of blank lines in this file.

Comment 32

[Lars T Hansen]

(In reply to comment #31)
> Comment on attachment 484818 [details] [diff] [review]
> Cleaned up version of Brent's patch
> 
> SR- for 

Should have read:

SR- for substantive problems and many stylistic issues.  The functionality seems OK such as it is, but I'm relatively worried that the system introduced here does not actually do what it's supposed to do, ie, make it easier and less error prone to write C++ code that uses managed storage.  (Maybe "easier" is not a goal?)

Comment 33

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

Comment on attachment 484818 [details] [diff] [review]
Cleaned up version of Brent's patch

Finalizing on r- mostly because I (like Lars), want illustrations of usage for the functionality.  I would think selftests are the appropriate method (rather than relying on the patch to the shell code).  I am still hoping to write some, but I have a couple other things that come first on my plate at the moment.  Tommy, let me know if you want me to push up the priority on writing selftests myself.

Comment 34

[Lars T Hansen]

(In reply to comment #33)
> Comment on attachment 484818 [details] [diff] [review]
> Cleaned up version of Brent's patch
> 
>  I would think selftests are the appropriate method (rather
> than relying on the patch to the shell code).

The reason I'm asking to see patches to Shell and Player code is that that's where the complexity will show up with full force.  Selftest code is likely to be too well behaved to really explore that aspect of the mechanism.

(Selftest code is desirable for other reasons, of course.)

Comment 35

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

(In reply to comment #34)
> (In reply to comment #33)
> > Comment on attachment 484818 [details] [diff] [review] [details]
> > Cleaned up version of Brent's patch
> > 
> >  I would think selftests are the appropriate method (rather
> > than relying on the patch to the shell code).
> 
> The reason I'm asking to see patches to Shell and Player code is that that's
> where the complexity will show up with full force.  Selftest code is likely to
> be too well behaved to really explore that aspect of the mechanism.
> 
> (Selftest code is desirable for other reasons, of course.)

I agree that selftests written in a vacuum would be likely to omit some (most) of the complexities that arise.

Ideally any inherent complexities that are not reflected in a selftest should be reduced to their essence and then encoded as a selftest.  (Ideally -- not necessarily realistic.  But it seems like a nice goal to strive towards)

Comment 36

[Tommy Reilly]

Another thing that won't necessarily come up in the shell is code that passes around pointers to fields.  In the player there's DWB(Foo*) *'s being passed around.   With things as they are these become:

MMgc::GCFinalizedObject::GCMemberRef<Foo>*

So a couple things:

1) MMgc namespce isn't open in the player.   Opening it might be a challenge.   We could put the Ref classes in a new namespace and open that instead.   Like maybe MMgc::References or something.  Actually that's probably not possible because GCMemberRef is actually 3 classes that are inner classes of GCRoot/GCObject/GCFinalizedObject and you put an inner class in a different namespace than the outter class I suspect

2) The GCObject/GCFinalizedObject separation is nasty here.  I'd really like to get this down to:

GCObject::Member<Foo>*

maybe we should revisit having GCFinalizedObject inherit from GCObject

3) Can we revisit the "GC" prefix it really sucks IMHO.

Comment 37

[Tommy Reilly]

> GCObject::Member<Foo>*

Actually GCMember<Foo>* is really what we want.  Need to figure out how to do away with the multiple Member definitions.

Comment 38

[Lars T Hansen]

(In reply to comment #36)
> 2) The GCObject/GCFinalizedObject separation is nasty here.  I'd really like to
> get this down to:
> 
> GCObject::Member<Foo>*
> 
> maybe we should revisit having GCFinalizedObject inherit from GCObject

For exact tracing there are additional wrinkles, basically there is a GCTraceableBase base class that's distinct from GCObject.

> 3) Can we revisit the "GC" prefix it really sucks IMHO.

You mean on GCRef and GCMemberRef?  I agree it seems unnecessarily verbose.

Comment 39

[Brent Getlin]

(In reply to comment #30)
> Do we have selftests?  If not, and assuming that it still makes sense
> to test this functionality (I think it does), I think I might make
> some selftests (I've been meaning to play with this stuff and a
> selftest seems like the right place to do it).
[bg] A number of weeks ago we discussed this and decided that self tests are redundant once AVMShell code is updated to use this new mechanism since the avmshell selftests will sufficiently exercise the GC. 
> 
> On GCREF_RETURNMEMBER(type, var) { ... }
> I would slightly prefer that it use the pattern:
>    GCREF_RETURNMEMBER(type, var) do { ... } while (0)
> as that would interoperate better with a semicolon delimiter at the
> point of invocation (see also
> http://stackoverflow.com/questions/154136/why-are-there-sometimes-meaningless-dowhile-and-ifelse-statements-in-cc-macros
> )
[bg] This deprecated Macro is no longer used and should be removed.
> 
> This phrase occurs a couple of times: "Ultimately, we'll probably add
> some utility class to replace this macro."  Should this include a
> bugzilla bug number so a reader can followup on future events?
> 
[bg] Good point.  All temporary macros and precompiler defines that we're planning on checking in should have bug written agains them.

[bg]  All other comment/organization comments: Ok.
> 
>

Comment 40

[Brent Getlin]

(In reply to comment #38)
> (In reply to comment #36)
> > 2) The GCObject/GCFinalizedObject separation is nasty here.  I'd really like to
> > get this down to:
> > 
> > GCObject::Member<Foo>*
> > 
> > maybe we should revisit having GCFinalizedObject inherit from GCObject
> 
> For exact tracing there are additional wrinkles, basically there is a
> GCTraceableBase base class that's distinct from GCObject.
> 
> > 3) Can we revisit the "GC" prefix it really sucks IMHO.
> 
> You mean on GCRef and GCMemberRef?  I agree it seems unnecessarily verbose.

[bg] I disagree. I think the GC prefix makes it clear to developers that their using Garbage Collected memory rather than unmanaged memory.  All "members" of GCObjects or GCRoot's do not have to be managed, therefore using the generic term "Member" seems too vague.

Comment 41

[Brent Getlin]

(In reply to comment #37)
> > GCObject::Member<Foo>*
> 
> Actually GCMember<Foo>* is really what we want.  Need to figure out how to do
> away with the multiple Member definitions.

[bg] It's not ideal, but it's really not that bad either.  The only alternative I see here is to globally define GCMemberRef and implement dynamic type checking of the parent class (something we definitely don't want to do)

Comment 42

[Brent Getlin]

(In reply to comment #34)
> (In reply to comment #33)
> > Comment on attachment 484818 [details] [diff] [review] [details]
> > Cleaned up version of Brent's patch
> > 
> >  I would think selftests are the appropriate method (rather
> > than relying on the patch to the shell code).
> 
> The reason I'm asking to see patches to Shell and Player code is that that's
> where the complexity will show up with full force.  Selftest code is likely to
> be too well behaved to really explore that aspect of the mechanism.
> 
> (Selftest code is desirable for other reasons, of course.)

[bg] Bug below contains progress on patches to the shell code: 
https://bugzilla.mozilla.org/show_bug.cgi?id=597577

Comment 43

[Tommy Reilly]

> [bg] I disagree. I think the GC prefix makes it clear to developers that their
> using Garbage Collected memory rather than unmanaged memory.  All "members" of
> GCObjects or GCRoot's do not have to be managed, therefore using the generic
> term "Member" seems too vague.

Remember we're replacing DWB and DRCWB.   No one ever complained that they didn't have a GC prefix.     Its a smart pointer class in the MMgc namspace wrapping a class that already has to inherit from GCObject, how many "GC"s do we need!   I don't think the vagueness of Member will have any real impact.   I still think Member sucks because its 3 more letters than DWB ;-)   And all I really care about is letters, seriously, people will come to learn what it means whatever we chose just like they did for DWB/DWBRC.   Other alternatives:

GCM<t> // GC Member
GCF<t> // GC Field
MF<t> // Managed Field (my favorite!)

But I'll concede Member<t> if it ends the debate.

Comment 44

[Brent Getlin]

(In reply to comment #32)
> (In reply to comment #31)
> > Comment on attachment 484818 [details] [diff] [review] [details]
> > Cleaned up version of Brent's patch
> > 
> > SR- for 
> 
> Should have read:
> 
> SR- for substantive problems and many stylistic issues.  The functionality
> seems OK such as it is, but I'm relatively worried that the system introduced
> here does not actually do what it's supposed to do, ie, make it easier and less
> error prone to write C++ code that uses managed storage.  (Maybe "easier" is
> not a goal?)

[bg] Easier and less error prone is definitely the goal. ( This could be a long conversation, but here are the main points of pain that will be eliminated with this problem:
1) Garbage Collected memory is now explicit (making it easier to decipher what's going on in the code).  No more bald pointers that might be unmanged or managed.  
2) Single-syntax member declarations ( via GCMemberRef) make it easier to code since you never have to know which writebarrier/rc macro to use. 
3) GCRef/GCMemberRef use strong typing and C++ inheritance to catch more coding errors at compile time compared to debug asserts using the existing mechanism ( like using DWB() as a member of an unmanaged class instance)

Comment 45

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

(In reply to comment #43)
> I still think Member sucks because its 3 more letters than DWB ;-)   And all I
> really care about is letters, seriously, people will come to learn what it
> means whatever we chose just like they did for DWB/DWBRC.   Other alternatives:
> 
> GCM<t> // GC Member
> GCF<t> // GC Field
> MF<t> // Managed Field (my favorite!)
> 
> But I'll concede Member<t> if it ends the debate.

I'd prefer Member<t> over the suggestions above.  All caps is hard to read.

A collection of similar looking acronyms drawn from an alphabet soup of all
caps is even worse.  I'm pretty sure that there was an email that Lars
forwarded some months ago with a Player engineer ranting in part about the mmgc
acronyms.  (But I could not find the email in question, so perhaps I dreamed
it.)

I have no preference about GCMember<t> vs Member<t>.  (Maybe that reflects the
fact that I'm still in the reading-more-than-writing stage.)

Comment 46

[Brent Getlin]

(In reply to comment #43)
> > [bg] I disagree. I think the GC prefix makes it clear to developers that their
> > using Garbage Collected memory rather than unmanaged memory.  All "members" of
> > GCObjects or GCRoot's do not have to be managed, therefore using the generic
> > term "Member" seems too vague.
> 
> Remember we're replacing DWB and DRCWB.   No one ever complained that they
> didn't have a GC prefix.     Its a smart pointer class in the MMgc namspace
> wrapping a class that already has to inherit from GCObject, how many "GC"s do
> we need!   I don't think the vagueness of Member will have any real impact.   I
> still think Member sucks because its 3 more letters than DWB ;-)   And all I
> really care about is letters, seriously, people will come to learn what it
> means whatever we chose just like they did for DWB/DWBRC.   Other alternatives:
> 
> GCM<t> // GC Member
> GCF<t> // GC Field
> MF<t> // Managed Field (my favorite!)
> 
> But I'll concede Member<t> if it ends the debate.

1) FixedMalloc is also in the MMgc namespace, but it doesn't mean you're allocating GC memory
2) Just because it's defined on GCObject, doesn't mean it has to be garbage collected.  Members of GC objects CAN be unmanaged memory pointers.
3) Why does the number of letters matter?

Comment 47

[Tommy Reilly]

(In reply to comment #46)
> 2) Just because it's defined on GCObject, doesn't mean it has to be garbage
> collected.  Members of GC objects CAN be unmanaged memory pointers.

Right and those are raw pointers not UnmanagedMember<t>.   We call you Brent, not ManBrent ;-)

> 3) Why does the number of letters matter?

Because people have to type these things in.   If you look at most smart pointer classes in the world they use ptr not pointer, brevity is readability and makes for happier developers.

Comment 48

[Brent Getlin]

(In reply to comment #31)
> Comment on attachment 484818 [details] [diff] [review]
> Cleaned up version of Brent's patch
> 
> SR- for 
> 
> General:
> 
> There is no documentation on how this is supposed to be used, or how
> it's supposed to hang together.  (Wiki docs do not count, partly
> because Tamarin is open-source and zerowing is internal to the
> corporation, partly because wiki docs are too far away from the code
> to be reliable and evolve with the code.)
> 
> I think studying an extended usage example (eg the shell code) will 
> be required in order to have confidence in the code, even after the
> objections listed below are corrected.
> 
> 
> Functionality / substantive:
> 
> GCRef.h:
> 
> Names like GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT are unreadable (which
> I consider a problem with functionality, not mere style) and need to be
> fixed; GCREF_NO_ADDRESS_OPERATOR_GCFinalizedObject might work, though a fix
> will probably run into a 31-character preprocessor limit on some
> compilers.
> 
> (As proof that it's unreadable, observe that the spelling actually is
> GCREF_NOADDRESOPERATORGCFINALIZEDOBJECT but should be
> GCREF_NOADDRESSOPERATORGCFINALIZEDOBJECT and that I accidentaly
> corrected it in the rewrite, where the error becomes obvious.  Ditto
> for GCREF_NOADDRESOPERATORGCOBJECT.  This kind of error will be tricky
> to catch sometimes since it's just an #ifdef / #ifndef - the lack of
> redundancy prevents the compiler from flagging obvious errors.)
> 
> Even then the name appears to be misleading;
> GCREF_NO_ADDRESS_OF_GCFinalizedObject would be more correct (the
> effect is to privatize the operator).
> 
> Avoiding negative #define names is an important goal, so
> GCREF_PRIVATE_ADDRESS_OPERATOR_GCFinalizedObject might be even better,
> and actually states the same thing as the comment documenting the
> #define.
> 
> (I will continue to give this R- / SR- until most or all negative
> #defines are gone unless I can be convinced that it's for the greater
> good to keep them the way they are.)
> 
> Some of the macros, like GCREF_UNPROTECTEDMEMBER, makes me question
> whether the GCRef machinery lives up to its billing by making the API
> simpler.  (Also it's not at all clear what an "unprotected" member is.)
> 
[bg] The fact is, preexisting code has been written that DOES NOT use ref counting or write barriers where it seems that it should. Perhaps this was done as an optimization, perhaps because the dev had preexisting knowledge that it was unnecessary, or perhaps it was just a mistake.  This mechanism is available to circumvent the safety of the new system by using an explict, dangerous sounding MACRO that can be easily identified in the code.
>
> How important is it to use eg GCREF_CASTFROMVOID and
> GCREF_AVMPLUSRAWPTR etc?  Since they have straightforward expansions
> it would be astonishing if code did not bypass these and go straight
> to the expansion.  That undermines the value of the safegc work.  The
> comment says, "Ultimately, we'll probably add some utility class to
> replace this macro."  IMO that needs to happen sooner rather than
> later.
> 
[bg] Agree.  Per Felix's earlier comment, we should definitely write a separate bug for work items like this.

> GCRef_Union seems to defeat the purpose of the safegc work.
> Furthermore, its documentation is not good - a usage example and
> tighter prose would serve it well.  (Also GCRef_Union is not an ideal
> name, GCRef_UnsafeNobarrierNoRC more directly expresses what it does.)
> 
[bg] Unions do not allow non-default constructors to be defined on members.   Our choice here is to allow ONLY unions to circumvent this piece of our machinery (ie. performing Debug runtime checks to make sure GCRefs are only declared on the stack and automatically setting the raw pointer to NULL), remove all unions, or remove that checking from all code.  We chose removing this check for ONLY unions, which seems reasonable. 

> This is wrong in both GCRef and GCRef_Union.
> 
>   // lock this up.  We don't want GCRefs to automatically convert into
>   // generic ptr types (like Atom).
>   REALLY_INLINE operator int32_t() const ;
> 
> The reason it's wrong is because Atom is intptr_t, not int32_t, though
> that is anyway secret.  It might be OK to use avmplus::Atom here if
> that conversion is the only one you worry about.  (The exactgc work
> exposes the Atom type to the GC.)  If Atom is not the only one you
> worry about then more need to be added here.
> 
[bg] Good catch.  We want to stop all ptr casting on this, not just Atom.
> 
> GCFactory.h:
> 
> I see no reason not to provide NewWithExtra in all cases where New is
> provided.  This is true for several other factories too, and sometimes
> NewWithExtra is provided but New is not.  It all seems completely
> arbitrary.
[bg] I agree, right now we've only added GCFactory methods as they've come up.  We should add a bug to add GCFactory for all "in-between" numbers of parameters.
> 
> GCFactoryRawPtr specialized for kTypeGCFinalizedObject is the only one
> that has an Alloc method.  First, this is strange - one more example
> of ad-hoc method suites on these factories.  Second, this should
> almost certainly be a Calloc method because only having an Alloc
> method forces a client that wants to allocate an array to compute the
> size of the array and that code will always be prone to overflow, thus
> opening a security issue; Calloc gets the computation right always.
> 
[bg] Same comment as above for the "only on GCFinalized Object".  Also, we've recently added Calloc, but it's not yet landed in this patch.

[bg] I'm fine with all other comments based on code commenting and blank lines.

Comment 49

[Brent Getlin]

(In reply to comment #47)
> (In reply to comment #46)
> > 2) Just because it's defined on GCObject, doesn't mean it has to be garbage
> > collected.  Members of GC objects CAN be unmanaged memory pointers.
> 
> Right and those are raw pointers not UnmanagedMember<t>.   We call you Brent,
> not ManBrent ;-)
> 
[bg] The "default" kind of member in c++ is "unmanaged" so I don't think it needs to be explicit.  The distinguishing factor for GC members is that they're "GC".  Therefore, GC or managed should be in the name somewhere. The reason "Member" is in there is so that it's clear to the dev that this "type" should be used "as a member" and NOT on the stack or as a temporary variable.

> > 3) Why does the number of letters matter?
> 
> Because people have to type these things in.   If you look at most smart
> pointer classes in the world they use ptr not pointer, brevity is readability
> and makes for happier developers.

[bg] I just don't see how naming a class as an acronym is readable.

Comment 50

[Tommy Reilly]

> [bg] The "default" kind of member in c++ is "unmanaged" so I don't think it
> needs to be explicit.  The distinguishing factor for GC members is that they're
> "GC".  Therefore, GC or managed should be in the name somewhere. 

The fact that you have to use Member is enough distinction in my mind.   Member will mean "reference to managed memory" b/c that's what it is.

Once people learn the definition of something how its spelled its not important (a rose by any other name...). 

> [bg] I just don't see how naming a class as an acronym is readable.

I agree, GC is an acronym, therefore we shouldn't use it.   Glad we agree!

Comment 51

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

Is there some way that the player team could use namespaces or similar to put a prefix back on (e.g. GC::Member) if it turns out that they agree with Brent's point of view?  That is, is there one choice here that is easier to swap in the other approach?

At worst, we could put these Member thingies into a MMgc::GC namespace ourselves, but then immediately open that namespace within the AVM code... is this craziness?

(I'm trying to put an end to what seems to be turning into a bike shed argument.)

Comment 52

[Tommy Reilly]

typedef?   its friday maybe we should do one of those free internet polls!

Comment 53

[Tommy Reilly]

Survey!

http://www.surveymonkey.com/s/KMCBKRL

Comment 54

[Tommy Reilly]

GCMember wins the survey so I'm going with that unless Lar's decides to trump the popular opinion

Comment 55

[Steven Johnson]

> This is not pretty: if you mean NULL, say NULL:

Newer versions of gcc will warn (and thus error) if you try to use 0 and NULL interchangeably in some situations, so it's no longer merely stylistic.

Comment 56

[Steven Johnson]

Vaguely related question: from looking thru player code, we could really use a strongly-typed-template wrapper around GCWeakRef, eg

  GCWeakPtr<ScriptObject*>

to avoid repeated, error-prone explicit casts. Is such a thing part of the SafeGC proposal (he asked, too lazy to look thru the patch himself)? If not, it should be (or we should open a new bug for it IMHO)

Comment 57

[Steven Johnson]

(In reply to comment #47)
> Because people have to type these things in.   If you look at most smart
> pointer classes in the world they use ptr not pointer, brevity is readability
> and makes for happier developers.

Brevity and Readability are totally good reasons, but IMHO time to type them in is not. Code should be optimized for reading, not writing.

Comment 58

[Brent Getlin]

(In reply to comment #54)
> GCMember wins the survey so I'm going with that unless Lar's decides to trump
> the popular opinion

BTW, in these discussions, the "Ref" part has been automatically dropped off.  I have no special affinity for "Ref" but I do like the correlation it brings GCMemberRef/GCRef .  Are you still suggesting GCRef is called GCRef?

Comment 59

[Tommy Reilly]

> BTW, in these discussions, the "Ref" part has been automatically dropped off. 
> I have no special affinity for "Ref" but I do like the correlation it brings
> GCMemberRef/GCRef .  Are you still suggesting GCRef is called GCRef?

Actually I've never liked Ref, its a lie, the underlying rep is not a C++ reference its a pointer.   GCPtr would make more sense to me and follow smart pointer conventions.   But I say we leave it GCRef.

Comment 60

[Brent Getlin]

Ok, so the final decision is to leave everything as is?  GCMemberRef & GCRef

Comment 61

[Tommy Reilly]

(In reply to comment #60)
> Ok, so the final decision is to leave everything as is?  GCMemberRef & GCRef

Oh god, I thought GCMember was decided?

Comment 62

[Brent Getlin]

Hah... I must've missed that part.  That being said, I don't have a strong preference that GCMemberRef has the "Ref" since it derives from "GCRef", but I do like the parity of telling devs "Use a GCRef or GCMemberRef".   Set up another one of those surveys :)

Comment 63

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

(In reply to comment #62)
> Hah... I must've missed that part.  That being said, I don't have a strong
> preference that GCMemberRef has the "Ref" since it derives from "GCRef", but I
> do like the parity of telling devs "Use a GCRef or GCMemberRef".   Set up
> another one of those surveys :)

If you do, please include another alternative like GCLocal (or GCVar, GCStack) to GCRef, or something.  That, or include a "none of the above" option.  (I am thinking that we are discussing a false dichotomy; tom and i already talked about in the chat room.

Comment 64

[Tommy Reilly]

Oh lookey here, Mr Bike Shed wants a survey! ;-)

So let's talk about GCRef is it purely for stack refs or for stack refs and member of stack objects.   

Option #1: GCRef is only for stack variables and GCMember has to be used in stack objects and we add a new GCStackObject that stack only objects with GC pointers has to inherit from

Option #2: GCRef is for any untracked variable and can be a member in class that are only stack allocated.

I think #1 is more in keeping with the safegc philosophy and I think I'm leaning that way

Comment 65

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

(In reply to comment #64)
> Oh lookey here, Mr Bike Shed wants a survey! ;-)

well, there's a difference between wanting a survey, and not wanting a bad survey.  :)

Comment 66

[Steven Johnson]

Current patch doesn't apply to current TR tip -- could someone rebase it?

Comment 67

[Tommy Reilly]

Attachment: rebased to 5471

Comment 68

[Tommy Reilly]

Transplant from bug 608831, I agree with Steven here, its worth spending some time to see if we can fix this:

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

Comment 69

[Tommy Reilly]

Also DEBUG_ARG should MMGC_DEBUG_ARG, the rule for awhile has been all CPP macros that aren't undef'd have to start with MMGC (I'm aware that there are historical counter-examples, we just don't want to introduce new ones).

Comment 70

[Tommy Reilly]

Pile on:

If MMGC_GCENTER and a GCObject* are in the same stack frame (or MMGC_GCENTER is missing) we may miss the object in our stack scan.   GCRef should assert that &t within the stack extents set up by MMGC_GCENTER.

Comment 71

[Tommy Reilly]

Attachment: Update patch simplified

based to tip, I turned off git so the revision is in the patch.  Highlights/new stuff:

GCFactory gone (for now will come later)
GetThisRef<T> replaced with "thisRef" macro
GCRef and GCMember auto convert with raw pointers to grease the skids on landing this stuff in the player
Macros that were required to deal with raw pointers are gone

Comment 72

[Brent Getlin]

Syntax/Style issue:
I see no reason for the "thisRef()" macro to contain parentheses. Following the pattern of other "parameterless macros", the parens should be removed.  This will also make usage syntax more similar to usage of "this" pointer, which is what we really want here.

Comment 73

[Tommy Reilly]

I left parens on thisRef thinking the macro would go away and be replaced by a real function later declared on each class that wanted to use thisRef().   If we can live with the macro long term I'm fine with dropping the parens.   Just "thisRef" would be cool.   Lars?

Comment 74

[Tommy Reilly]

I'll repost the patch with s/thisRef()/thisRef/ if Lar's is okay with it.

Comment 75

[Lars T Hansen]

(In reply to comment #73)
> I left parens on thisRef thinking the macro would go away and be replaced by a
> real function later declared on each class that wanted to use thisRef().   If
> we can live with the macro long term I'm fine with dropping the parens.   Just
> "thisRef" would be cool.   Lars?

Should be OK to remove the parens.  If we decide to make it a function later then it's easy enough to search for it.

Comment 76

[Tommy Reilly]

Attachment: s/thisRef()/thisRef

Updated to remove old GetThisRef and drop parens on thisRef.

Comment 77

[Tommy Reilly]

Attachment: Update, GCMemberGCBase fixes

This is same as last patch except s/GCMemberGCBase/GCMemberBase/

Comment 78

[Lars T Hansen]

Comment on attachment 494695 [details] [diff] [review]
Update, GCMemberGCBase fixes

The design seems fine.

At the top of GCRef.h you write thisRef() in a comment but that's wrong, the parens are not allowed.

GCREF_CASTTOVOID is unreadable (ditto the others); GCREF_TO_VOID (or GCREF_CAST_TO_VOID, if you insist) would have been the better choice IMHO.

Comment 79

[Tommy Reilly]

Will fix parens and continue my efforts to eradicate all GCREF_ macros.

Comment 80

[Tamarin Bot]

changeset: 5629:5f1e9bd9fc1d
user:      Tommy Reilly <[e-mail]>
summary:   Bug 565664 - Safegc infrastructure (r=lhansen,r=treilly,author=bgetlin)

http://hg.mozilla.org/tamarin-redux/rev/5f1e9bd9fc1d

Comment 81

[Dan Smith]

Retargeting SafeGC work items to "future".

Comment 82

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 83

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 84

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 85

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 86

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 87

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.