597577 - Integrate "Safe GC" mechanism into avmshell

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Brent Getlin]

The proposed solution for "safer" use of GC memory (https://bugzilla.mozilla.org/show_bug.cgi?id=565664) requires syntax changes to avmshell wherever GC memory is being used.

Comment 1

[Brent Getlin]

Attachment: First iteration of avmshell integration

Comment 2

[Tommy Reilly]

taking this over from Brent.    First order of business is cleaning up avmshell, see blocking bugs

Comment 3

[Lars T Hansen]

Going from eg this

  ScriptObject* self

to this

  MMgc::GCRef<ScriptObject> self

is a bit absurd, why not open the MMgc namespace everywhere?  The amount of clutter coming in from having to state every time that GCRef is in the MMgc namespace is going to be unpalatable.

Comment 4

[Edwin Smith]

+1 to opening mmgc everywhere, although ISTR at least one style guide suggesting it's better to do it at the top of each CPP file instead of in a header file.  But, either way.

Comment 5

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

I seem to recall Tommy hoping we might get rid of the GC prefix on all (or at least many) of the classes defined in the MMgc namespace.  And I've already added a PageMap class without the GC prefix.

Is this going to be in conflict with opening the mmgc namespace "everywhere" ?

(As Ed notes, it may be better to do it within each CPP file, though that will mean that the field declarations themselves within the headers are going to be forced to take the verbose form that Lars described.)

Comment 6

[Steven Johnson]

If we're proposing cleanup, could I modestly propose s/MMgc/mmgc/ ?

Comment 7

[Tommy Reilly]

Well if we're gonna change it why bother keeping the MacroMedia part?   I still like spam (space manager) ;-)

Comment 8

[Steven Johnson]

"gc" is agreeably short but perhaps more likely to collide with something. (nonissue?)

Comment 9

[Tommy Reilly]

gc isn't general enough in my mind, its more than just a garbage collector

Comment 10

[Lars T Hansen]

Naming cleanup is a bit of a distraction, let's spend the time on something more useful.  I brought the issue up because my eyes bleed when I have to read MMgc::GCRawPtrFactory<ScriptObject>::NewWithExtra(...), which is incredibly hard to read (and will all have to change with exact marking anyway, wohoo).

(Or was that GCFactoryPtrRaw<>?)

Comment 11

[Steven Johnson]

(In reply to comment #10)
> (Or was that GCFactoryPtrRaw<>?)

It's spelled "GCRawPtrFactory<>" but it's pronounced "throat warbler mangrove"...

Comment 12

[Tommy Reilly]

Proposal:

GCRef<T> -> Ref<T>
GCMemberRef<T> -> Field<T>

Rationale, since GCRef will show up everywhere making it 3 chars is attractive, besides MMgc::GCRef is redundant.    Member is too general as a member can be a function or a field, field is more precise.

I wouldn't worry about the GCRawPtrFactory, the plan was to leave the core alone and allow it to use raw pointers.

Comment 13

[Lars T Hansen]

(In reply to comment #12)
> Proposal:
> 
> GCRef<T> -> Ref<T>
> GCMemberRef<T> -> Field<T>

I think these suggestions are good, provided they do not bring back the need for 'MMgc::Ref' and 'MMgc::Field' to avoid name clashes.  Maybe Brent has an opinion.

Comment 14

[Brent Getlin]

I prefer GCRef over Ref since the important part of these "references" is that
they're "garbage collected".  I'm not against "field" but I think "member"
makes it more clear that you should only use this definition when defining a
"member" of a class/struct/union.

GCFactoryRawPtr (which is the correct current term) should be used only in very
limited places host code (or avmplus headers that are included in host code)
where the host needs access to creating raw pointers specifically for feeding
back to avmplus.  I'm not against renaming this to something that makes this
clearer, I just can't think of another name that says : "Creates raw pointers
to GC memory that should only be used in avmplus apis"

Comment 15

[Lars T Hansen]

Comment on attachment 476418 [details] [diff] [review]
First iteration of avmshell integration

Removing self from f? because Tommy is working on this patch.

Comment 16

[Dan Smith]

Removed Brent's feedback request for Tommy.  Tommy owns the bug now and will send out his own feedback requests.

Comment 17

[Tommy Reilly]

Attachment: General approach for prepping the patient

This is a patch that applies and compiles.  Its the basis for an approach that can be done partially.  Basically the idea to remove all raw pointer use and then lay in safegc with something like:

#ifdef MMGC_SAFEGC_BIG_RED_BUTTON
//new school
#define MMGC_DECL_REFS(_t)\
        class _t;\
        typedef MMgc::GCRef<_t> _t##Ref;\
        typedef MMgc::GCMember<_t> _t##Field;
#else
//old school
#define MMGC_DECL_REFS(_t) \
        class _t;\
        typedef _t* _t##Ref;\
        typedef MMgc::WriteBarrier<_t*> _t##Field;
#endif

Comment 18

[Tommy Reilly]

If we like this approach then we need to refactor GCMemberRef into something simpler.  This could be done if we fix this: https://bugzilla.mozilla.org/show_bug.cgi?id=608243

Comment 19

[Lars T Hansen]

Comment on attachment 486893 [details] [diff] [review]
General approach for prepping the patient

It's succinct, but I think this is going to make auditing code for correct annotations for exact tracing pretty much impossible, whereas the explicit GCMemberRef fields served as clear visual markers, and if GCMemberRefs were generally required (no escape hatches in shell or player code) then they would even be comprehensive visual markers.

Comment 20

[Tommy Reilly]

I'll buy that, I'm trying to get the API nailed down and working with pointers and then switch to references later (which will make player integration much easier).  How about:

GCField<ClassClosure> GC_POINTER(m_foo);

GC_POINTER(ClassClosure, m_foo);

I'm forgetting why the GC_POINTER was separate out in exact tracing.   Can we have an API where GC_POINTER is used regardless of whether the class is exactly traced?     Like lets have one way to declare everything that's truly universal with no exceptions.     I'm thinking the other annotations would serve as the "switch" that enables the exact tracing code gen.

Comment 21

[Lars T Hansen]

Not every class will be exactly traced.  The SafeGC work is more comprehensive.  I really do not want to mix the two.

I thought we had resolved this once and for all, that the syntax would be GCMemberRef<type> for the property declaration.  Why are we rehashing this?

Comment 22

[Tommy Reilly]

I don't know, just looking to simpilfy things.  I think we agreed to drop the "Ref" part and just have GCMember<t>.    GCMember<t> will still allow me to switch the code over and to the new syntax independent of the pointer->reference switch I think, I'll run with that.

Comment 23

[Tommy Reilly]

Attachment: Take 2

Okay so this uses the new GCMember syntax.   It works as is.   Its not based on the existing safegc patch, the idea is to be able to go in phases, something like:

1) convert all DWB and DRCWB to GCMember
2) introduce GCRef and convert raw pointers to it
3) introduce GCFactory and hide new
4) replace fast and loose pointer exposing GCRef/GCMember with Brent's locked down tight ones

Am I crazy?

Comment 24

[Tommy Reilly]

oh the shell doesn't open the MMgc namespace so I put these in the default namespace, thoughts?   Should we have a SafeGC namespace?   Should it nest under the MMgc namespace?   We could put the fast/loose impls in a separate namespace and potentially have both active at once.

Like my RC object vs. non-RC object template trick?   No hacky extra template parameter required!

Comment 25

[Lars T Hansen]

Comment on attachment 487027 [details] [diff] [review]
Take 2

This is pretty sweet, though I don't know what Brent will think about its ability to catch errors.  At a minimum we require is an assert in GCMember::GCMember that the container is a GC object, I guess, but the lack of compile-time errors is worrisome - it's one of those problems I think the SafeGC project was supposed to fix.

The overriding of WriteBarrier in RCObject is probably a source of bugs (type hierarchy breaks down), may want an assert on GCFinalizedObject::WriteBarrier to ensure that the object is not an RCObject.  Again, not great that we don't resolve these problems at compile time (in this case by making RCObject not derive from GCFinalizedObject - yeah, that would not be popular).

WriteBarrier definitions in GCObject.h must be REALLY_INLINE.

Comment 26

[Tommy Reilly]

We're rolling this approach into the safegc player branch and its working nicely, we've extended it to remove the type parameter on GCFactory too.   No compile time safety is lost because these methods aren't ever called directly.   Brent's on board.

We could tighten things up by making the value parameter GCObject */GCFinalizedObject */RCObject* and possibly making the WriteBarrier functions private exposed to GCMember with friend decls or both.

Comment 27

[Tommy Reilly]

Comment on attachment 487027 [details] [diff] [review]
Take 2

Canceling feedback, Brent likes it.

Comment 28

[Dan Smith]

Retargeting SafeGC work items to "future".

Comment 29

[Tommy Reilly]

Remaining work:

- remove one "Alloc" from FileClass::read
- use GCRef instead of raw pointers

Comment 30

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.