627032 - We should assert on allocations during shutdown

Status: RESOLVED FIXED

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

Description

[Tommy Reilly]

We do assert but later on when we delete the GCAlloc instances and there's allocations.  Better to assert when the allocation happens.

Comment 1

[Steven Johnson]

When we do this, let's add a way to signal "might allocate"... ie, say we have

    void foo() { if (something) gc->Alloc()... }

we want *every* call to foo() to warn us if called during finalization. So maybe add a debug-only trigger so we could write it as

    void foo() { GC_MIGHT_ALLOC(); if (something) gc->Alloc()... }

this would be useful to put in methods that don't obviously allocate and might be tempting to call from finalizers (eg List::clear)

Comment 2

[Tommy Reilly]

Attachment: add assert

Comment 3

[Tamarin Bot]

changeset: 5836:2901a5e58c3e
user:      Tommy Reilly <[e-mail]>
summary:   Bug 627032 - We should assert on allocations during shutdown (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/2901a5e58c3e

Comment 4

[Tommy Reilly]

(In reply to comment #1)
> When we do this, let's add a way to signal "might allocate"... ie, say we have
> 
>     void foo() { if (something) gc->Alloc()... }
> 
> we want *every* call to foo() to warn us if called during finalization. So
> maybe add a debug-only trigger so we could write it as
> 
>     void foo() { GC_MIGHT_ALLOC(); if (something) gc->Alloc()... }
> 
> this would be useful to put in methods that don't obviously allocate and might
> be tempting to call from finalizers (eg List::clear)


Why not just do:

AvmAssertMsg(!gc->Destroying(), "Don't call clear shutdown finalizer");

Comment 5

[Steven Johnson]

(In reply to comment #4)
> AvmAssertMsg(!gc->Destroying(), "Don't call clear shutdown finalizer");

Sure, that would work, but encapsulating it in a purpose-built macro makes for cleaner code.

#define GC_MIGHT_ALLOC() vmAssertMsg(!gc->Destroying(), "Not legal to call during GC shutdown");

Comment 6

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

(In reply to comment #1)
> When we do this, let's add a way to signal "might allocate"... ie, say we have
> 
>     void foo() { if (something) gc->Alloc()... }
> 
> we want *every* call to foo() to warn us if called during finalization. So
> maybe add a debug-only trigger so we could write it as
> 
>     void foo() { GC_MIGHT_ALLOC(); if (something) gc->Alloc()... }
> 
> this would be useful to put in methods that don't obviously allocate and might
> be tempting to call from finalizers (eg List::clear)

I've been mulling over trying to layer an ad-hoc "effect system" (a la Lucassen+Gifford '88) atop dehydra, so that we could determine these sorts of thing statically.

It is probably the sort of thing that would be too much work for one off cases like this, but if it were sufficiently generalized it could also catch e.g. allocations from finalizers.

Comment 7

[Dan Smith]

Clearer error message should beadded and the change submitted to both Wasabi and Serrano.  Targeting Wasabi, assigned to Felix.

Comment 8

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

Pushed fix to Wasabi.  P4 CL 877496.

Comment 9

[Brent Baker]

Fix submitted to Wasabi with P4 CL: 877921, Verified fixed by player qe with player 10.3.180.77

I will check with Tommy to see if there is a shell testcase that makes sense to add here.