On Thursday 22 February 2007 00:28, you wrote: > chromatic wrote:
> >> + if (saved_log_data == NULL) > >> + return; > >> + > >> saved = saved_log_data; > >> mem_sys_free(saved->reads); > >> mem_sys_free(saved->writes); > > I'm not sure this is the right fix; there's little reason to use an > > STMLog PMC if it doesn't get a valid transaction log. My vote is for the > > PMC's init() to throw an exception. > > If that's not the case, then at least both of Parrot_STM_mark_extracted() > > and Parrot_STM_replay_extracted() need similar null guards. > > It makes sense here to return with no action, since all the routine does > is free memory. If the memory wasn't used, it doesn't need to be freed. Yes, if STMLog can ever possibly hold a null pointer. I'm asking if that should ever be possible. All these guards do is protect against STMLog being in an inconsistent state. Now that may be the right solution. It may not. I dug into this a bit more to try to find out *why* STMLog can be in an inconsistent state. In src/pmc/stmlog.pc, init() calls Parrot_STM_extract() and uses the return value, unchecked, for the PMC's struct val. destroy(), mark(), and replay() all pass the struct val unchecked to other Parrot_STM_*() functions. particle's test exposed a condition in which that struct val can be a null pointer, and dereferencing that in Parrot_STM_*() causes segfaults. Putting the null guard in the Parrot_STM_*() functions papers over that symptom, but it also means that we have to catch every use of the STMLog PMC's struct val and add a null guard there too, or we'll have more code paths that cause segfaults. How could the struct val be a null pointer? It's in src/stm/backend.c, the function Parrot_STM_extract(), on line 1241. If there's no current transaction, it's not possible to return a pointer for the transaction log, and so this function returns null, and the STMLog PMC stores a null pointer. Semantically, to me that means that there *is* no valid STMLog PMC. I don't know if that means that STMLog should throw an exception in its init() if there's no transaction log available, or if it should morph to Undef, or if it should continue doing what it's doing, but I do think it should be clearer that there's no valid transaction log at this point, as it's outside of any transaction. Fixing the problem there--not allowing an STMLog PMC to have a null struct val--would mean never having to worry if it's in an inconsistent state, and we could remove all of the null guards and never have to worry if we've missed one. I think that's an important characteristic of the right solution, whatever the right solution is. -- c
