487032 - CleanStack issues

Status: VERIFIED FIXED

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

Description

[Tommy Reilly]

Whether we like it or not the player uses multithreads in the Debugger.  Access is never simulataneous but we need to handle the situation and CleanStack doesn't.   Fix:

Require a MMGC_GCENTER macro to demarcate stack entry, use this in leui of GetStackTop.   If MMGC_GCENTER didn't happen use GetStackTop and disable stack cleaning.  In the future we should phase out GetStackTop and assert if an alloc happens without it.   This will  also have the following benefits:

1) allow CleanStack to be handled completely in the MMgc and removed from client code

2) prevent the GC from marking some phantom stack reference in portions of the OS stack that aren't really live, what happens is that the stack depth the player is entered at is radically different from say a DoPlay to an event delivering, relying on MMGC_GCENTER will ensure we aren't marking beyond the live region. 

3) Make GC more portable by getting rid of GetStackTop

4) Make GC faster by having the stack scan a lot smaller

Comment 1

[Edwin Smith]

running -Dselftests in a Debug-Debugger build asserts due to a missing GCENTER.

Comment 2

[Tommy Reilly]

That's my bad, do we have the Debug self tests slated to be added to the build bot?

Comment 3

[Tommy Reilly]

Raw allocas:

void GC::CleanStack(bool force)
void CleanEntireStack()
void GCHeap::DumpHeapRep()
const char *MemoryProfiler::Intern(const char *name, size_t len)
void MemoryProfiler::DumpFatties()
void GC::ProbeForMatch(const void *mem, size_t size, uintptr_t value, int recurseDepth, int currentDepth)
void GC::WhosPointingAtMe(void* me, int recurseDepth, int currentDepth)


These are all either uncritical optimizations or debugging code.   We already have VMPI_alloca unfortunately.   I think most of these can either be gotten rid of or converted to something else.  CleanStack is the one exception.   CleanStack wants to memset to zero the area above the stack (below in terms of addresses).  It does this to prevent the GC from seeing dirty stack references.    alloca is used so that the stack pointer is adjusted to include this region before the memset.   On some OS (windows) the OS will interrupt your program and run code on your stack and if are writing belowing esp things can crash badly.    So I think maybe we need VMPI_cleanStack(amt) and most OS's will just use alloca for this and Symbian might have to use some assembly code.   An empty VMPI_cleanStack will work to get off the ground though.

Comment 4

[Lars T Hansen]

Also see 487199.

Comment 5

[Lars T Hansen]

(sorry)

Also see bug 487199.

Comment 7

[Tommy Reilly]

VMPI_cleanStack now handles this, on platforms that can't do alloca VMPI_cleanStack is a noop.