> On Wed, Aug 21, 2002 at 04:17:30AM -0400, Mike Lambert wrote: > > Just to complete this thread, I have committed the current version of my > > COW code, as I promised earlier this week. > > Did you try running tests with GC_DEBUG on? I get numerous failures. > Here's a patch with a couple of fixes (not all of them gc-related), > though I should warn you that it is rather roughly carved out of my > local copy, which has far too many modifications at the moment.
Look at the hypocrite! He writes GC_DEBUG code to make others fix GC problems, then forgets to do the same for his own GC problems. I have another GC_DEBUG patch in the wings which should make this easier to test with if you compile with --debugging, when I get around to properly cleaning it up. > With this patch, things work for me, but I punted in one place: if you > look at unmake_COW() in string.c, I just disabled garbage collection > around the reallocation. The problem seems to be that you change where > s->bufstart points to, then call Parrot_reallocate_string() on s. But > that can trigger a collection, and it gets confused by the > inconsistent state. Hrm, yeah. When I did that in unmake_COW, it seemed like a neat hack. Unfortunately, all hacks are bad hacks with GC. :) Your patch looks good for now, although I'll have to think about a better way to solve the problem than blocking GC. > This patch also contains a debugging aid that I was speculating on > earlier. Whenever a buffer is marked as being live, it checks to see > if the buffer is on the free list. If so, it whines and complains. > (This is for finding premature killing of newborn Buffers; they'll go > through one sweep and get put on the free list, then get anchored to > the root set somehow. The next sweep will find them.) I see your clever use of a version tag to find the source of the problem. This should certainly help with buffers, and I imagine something similar can be done to PMCs as well. I'll take care of applying this patch, as I've other changes to submit. (See below.) > This is not 100% accurate, because the stackwalk is conservative: it > assumes that anything whose pointer is in the appropriate range and > otherwise smells right must be a live PMC or Buffer. I thought that > finding old Buffers on the stack would be rare, but I was wrong -- the > problem is that if they were buried in some deeply nested call chain, > then because stack frames are not zeroed out upon allocation (which > would be horribly slow), a later call chain will dig them back up. And > the stackwalking code itself is deep enough and has large enough stack > frames to dig up quite a bit of junk. This problem is theoretically identical to there being perfectly valid data on the stack that appears to be a pointer to a freed header. Even if we could memset(0), it would not eliminate this particular problem. > Note that this is a real bug; we really shouldn't be poking into dead > Buffers. There's no telling what the current state of decomposition > is. A seg fault might jump out and bite us, or worse, because that > pointer may have been used by something else for its own twisted > purposes. And that something else could get very upset when it returns > to find that we've jammed flags into the corpse and used its liver for > a link in the ->next_for_GC chain. I would mostly disagree about it being a bug. I originally thought it was a problem as well, until I talked it out loud on IRC once. First, free buffer/pmcs have one field you can't touch: bufstart/vtable. This first field of the structure is used as a pointer to create the freed-header linked list. However, every other bit of data in the header is part of allocated, valid memory. Parrot will dole out this memory in their current form to code requesting headers later. We can't possibly segfault by messing with the flag fields. Other than that, dead headers just sit around as part of this linked list waiting for someone to request them. So...if we find a buffer pointer on the stack, we modify it's flags in buffer_lives. This is perfectly harmless. When we perform the DOD free_unused_buffers, we only add it to the free list (and modify bufstart) if it's not BUFFER_on_free_list_FLAG or BUFFER_live_FLAG. So stuff already on the free list will stay that way. > It is unlikely to cause problems accidentally, I suppose -- pointers > are checked to make sure they're within one of the pools, so the only > way to run into problems is to have a pointer on the stack to > somewhere within a Buffer's memory, kill off the Buffer by forgetting > about it, then have DOD add the bogus Buffer back onto the free list. > Or have the COW code chase down a bogus tail pointer. Or use a bogus > PMC instead -- then you have ->vtable->mark() to worry about. Hm, > maybe it is dangerous. Your logic here is mostly correct, aside from "then have DOD add the bogus Buffer back onto the free list." As mentioned above, DOD will only add it if it isn't on the free list already. COW code during compaction only executes if the buffer is not BUFFER_on_free_list_FLAG. So given that we added it to the free list at some point in the past, compaction will ignore it completely, regardless of whether we find it as free. Finally, your last point is completely correct, and needs addressing. If we find a dead PMC on the list, the free_list linked list will have overwritten the vtable field of the PMC, and a ->vtable->mark() is extremely dangerous. I've modified mark_used to check flags & PMC_on_free_list_FLAG before adding it tho the list of things to call mark() on. This should rsolve that particular issue. We should document the above behavior and semantics so that no one can change the GC system (assuming they've read the documentation) and have these subtle assumptiopns come back to bite them in the ass. > Am I missing some reason why this is not a problem? It would slow > allocation and stackwalking way down if we have to keep a hashtable of > valid pointers on the side. Oh yes, I can't imagine what it would be like to have a hashtable of pointers. The thought of that apalls me, thanks to Dan's brainw^H^H^H^H^H^Hconditi^H^H^H^H^H^H^Hteachings. Thanks for looking into these issues, Mike Lambert
