602009 - defRefValidation in disrepair

Status: RESOLVED FIXED

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

Description

[Tommy Reilly]

Run's afoul of assert in GCAlloc::Free

Comment 1

[Tommy Reilly]

Commenting out the Sweep in FinishDefRefValidation seems to fix the problem, needs more analysis though.

Comment 2

[Tommy Reilly]

I'm working on this in an attempt to solve a watson bug (#2742097), re-targetting for serrano

Comment 3

[Tommy Reilly]

Attachment: config cleanup

Some code in the player sets these things post GC construction, have them do it via the config object instead.

Comment 4

[Lars T Hansen]

Comment on attachment 533337 [details] [diff] [review]
config cleanup

Nits:

Comment indentation for validateDRC.

Missing period at the end of comment for incrementalValidation.

Observation:

It may be time to require a non-NULL config parameter.

Comment 5

[Tommy Reilly]

+1 there are no clients that don't pass a config, I'll make it so

Comment 6

[Tommy Reilly]

Attachment: make it mandatory arg and make it swallow mode

additive with config cleanup patch

Comment 7

[Tamarin Bot]

changeset: 6320:7a78d16dbd28
user:      Tommy Reilly <[e-mail]>
summary:   Bug 602009 - some GCConfig cleanup related to fixing DRC validation (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/7a78d16dbd28

Comment 8

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

(In reply to comment #7)
> changeset: 6320:7a78d16dbd28
> user:      Tommy Reilly <[e-mail]>
> summary:   Bug 602009 - some GCConfig cleanup related to fixing DRC
> validation (r=lhansen)
> 
> http://hg.mozilla.org/tamarin-redux/rev/7a78d16dbd28

Looks like this is somehow yielding a GCConfig with a collectionThreshold of 0 and that poisons the major allocation budget (which then poisons everything else and a bunch of stuff breaks, at least on the debug builds).

Comment 9

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

Attachment: fix changeset:6320

Comment 10

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

Attachment: fix changeset:6320, v2

Comment 11

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

Comment on attachment 533812 [details] [diff] [review]
fix changeset:6320, v2

(rulohani gave R+ in over IRC #tamarin)

Comment 12

[Tamarin Bot]

changeset: 6321:0321cd94b63f
user:      Felix S Klock II <[e-mail]>
summary:   Bug 602009: fix smoke test failures due to broken initialization (r=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/0321cd94b63f

Comment 13

[Tommy Reilly]

Attachment: shell command line support for drc validation

Comment 14

[Tamarin Bot]

changeset: 6348:719c9ac89fed
user:      Tommy Reilly <[e-mail]>
summary:   Bug 602009 - DRC validation cli switch (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/719c9ac89fed

Comment 15

[Tommy Reilly]

Attachment: almost done, looking for feedback on approach

Still getting some false positives in some smoke tests I need to look at.

Comment 17

[Lars T Hansen]

Generally this seems fine, F+.

Saving myself work in a future review:

Adding code to the fast path in ZCT::Add takes us in the wrong direction, so I might be more comfortable if the new song and dance around KeepPinned is #ifdef DEBUG.  I recognize that with reasonable constant folding in the compiler there's no difference here, I'm just being nervous.

Note %p is not portable and should be avoided in anything meant to be parsed by a machine (which ought to be pretty much everything we print).  (Numerous instances.  Of course, many of those instances used %x, which is just plain wrong, so it's not like it's going in the wrong direction...)

The comment in GCRoot::GCMember<T>::set doesn't make sense to me, the DRC validator isn't supposed to be reaping anything.

Comment for performingDRCValidationTrace: " Tell's that " should be " True if ".

Later: what does "Debugging code that needs to be validated:" mean?

"// Do we care about mark stack overflow?"  Probably at least to the point where you detect it and do something intelligent, like signal a problem.

I don't think that the call to DoPinProgramStack belongs in MarkQueueAndStack, the call should come from DRCValidationTrace.

In setZCTIndexAndMaybeUnpin the 'reaping' flag should not be called that.

You probably want to print the informative message in DefRefValidate about why you're dumping the allocation trace before the trace not after?  I guess it depends on what usability experience you have with it.

Comment 18

[Tommy Reilly]

Attachment: minors fixes, mostly to address Lar's comments and apply ifdef DEBUG more liberally

Comment 19

[Lars T Hansen]

I won't be able to review this today, but if that's unreasonably blocking ZBB then you should either (a) get a review elsewhere or (b) go ahead and push and close the bug, and I will review on Monday.

Comment 20

[Tommy Reilly]

I'm gonna push to tr-serrano and tr today and get Felix to review to lighten your load.   DRC validation wasn't useful in finding any DRC bugs but the frequent heap tracing found a handful of tracing dangling pointer bugs in the player (fixed by Ruchi and I) so I think its worth getting in and having QE start using on a regular basis.

Comment 21

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

Comment on attachment 536962 [details] [diff] [review]
minors fixes, mostly to address Lar's comments and apply ifdef DEBUG more liberally

R+.  Only item 1 below is a "must-address" before pushing.

1. GCMember-inlines.h: The comment left behind here doesn't make sense in its context anymore.  Move it down or delete it.

             //  This is NOOP for GCObject and GCFinalizedObject
-            this->t->DecrementRef();
+            tOld = this->t;

2. GC.cpp: This line:
-        SAMPLE_FRAME("[mark]", core());
-
used to be at the start of GC::Trace, a method now replaced by GC::DRCValidationTrace.  You know more about the interface to the sampler than I do, I don't know if you deliberately meant to remove it or if it was an accident.  (Its debatable whether the sampler should care about internal instrumentation used in validating mmgc itself.)  Maybe it just simplify things to leave it out, but thought I should at least note the change.

3. GC.cpp: This open-ended question:
+        // Do we care about mark stack overflow?
should be tagged with this Bugzilla ticket (Or some followup ticket)?  Its frustrating to re-extract context for such comments when looking at the source code years down the line.  (Alternatively, tou can probably just get rid of the comment; you are already emitting the GCLog message on overflow, and that's all I think we would want to do for right now.)

4. ZCT.cpp: Is it actually unsound to do the DoPinProgramStack loop a second time in ZCT::Reap (see below)?  I'm not asking for you to take this test out (the modules here are already so interdependent that it would be silly to ask that), I'm just curious if this was for efficiency or something else.
+#ifdef DEBUG
+            // During DRC validation stack scanning happened already.
+            // See GC::DRCValidationTrace().
+            if(!gc->validateDefRef)
+#endif
+                VMPI_callWithRegistersSaved(ZCT::DoPinProgramStack, this);

Comment 22

[Tamarin Bot]

changeset: 6380:d44a1df327f2
user:      Tommy Reilly <[e-mail]>
summary:   Bug 602009 - Bring DRC validation back to life (r=fklockii, sr-pending=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/d44a1df327f2

Comment 23

[Tommy Reilly]

(In reply to comment #21)

> 1. GCMember-inlines.h: The comment left behind here doesn't make sense in
> its context anymore.  Move it down or delete it.

Moved down.

> 2. GC.cpp: This line:
> -        SAMPLE_FRAME("[mark]", core());
> -

Intentionally removed, Trace used to be called from Collect and the SAMPLE_FRAME was a relict from those days.

> 3. GC.cpp: This open-ended question:
> +        // Do we care about mark stack overflow?

I addressed it but didn't remove the comment, comment removed from what I pushed, basically in the event of mark stack overflow we bail the validation attempt and turn off validation.

> 4. ZCT.cpp: Is it actually unsound to do the DoPinProgramStack loop a second
> time in ZCT::Reap (see below)?  I'm not asking for you to take this test out
> (the modules here are already so interdependent that it would be silly to
> ask that), I'm just curious if this was for efficiency or something else.
> +#ifdef DEBUG
> +            // During DRC validation stack scanning happened already.
> +            // See GC::DRCValidationTrace().
> +            if(!gc->validateDefRef)
> +#endif
> +                VMPI_callWithRegistersSaved(ZCT::DoPinProgramStack, this);

I need the two stack scans to see the exact same stack or I got lots of false postives.  The process of marking actually pushs garbage pointers onto the stack somehow so the DRC reap would see them and assert.

Comment 24

[Lars T Hansen]

Comment on attachment 536962 [details] [diff] [review]
minors fixes, mostly to address Lar's comments and apply ifdef DEBUG more liberally

Rubber stamp.