[Webkit-unassigned] [Bug 79191] [JSC] The end atom of the marked block should be considered to decide if the cell is live

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79191





--- Comment #6 from Filip Pizlo <fpizlo at apple.com>  2012-02-26 21:12:45 PST ---
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (From update of attachment 128150 [details] [details] [details] [details])
> > > > This feels strange.  Either it is possible, due to the conservative nature of the stack scans, that we will see a pointer that passes the not-cell-middle test but is nonetheless beyond m_endAtom, or it isn't.  If it is, this patch will make us crash in debug mode and do the right thing in release mode.  If it is not possible, then this patch just adds noise.
> > > > 
> > > > So which is it?  Can you justify why you've added code that results in assertion failures for the case that you're claiming to handle?
> > > 
> > > The situation you described is the reason why I've added code. 
> > > There were some crashes, I've analyzed, caused by a pointer beyond m_endAtom.
> > > That unusual pointer was added to registerFileRoots while gathering conservative roots, but I've not figured out how the pointers beyond m_endAtom were used and could be existed in the register file.
> > > I expect this patch will make crash with more information in debug mode and do the right this in release.
> > 
> > Why not just do the right thing in both debug and release?
> > 
> > I don't like the idea of our GC having ASSERT()'s that are motivated by us not knowing what our GC is doing.  Seems like a bad precedent.
> 
> I got it. 
> Then, is it fine that the conditional check for m_endAtom only without ASSERT()'s ??

Only if we know that we actually need that check.

Do you have some case where you believe that this check would be necessary?  Should be easy enough to try to reproduce it with an ASSERT() placed in your local copy.  If you can demonstrate that the check is necessary (and I'm willing to believe that it is if I have evidence) then certainly we should commit the check, along with a test case that demonstrates its need!

But I don't want a check that isn't known to be necessary, because its presence may confuse us into later thinking that it *is* necessary.  Makes maintenance hard.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.