When we mark objects, we conservatively mark the stack and live registers. This is done by creating a local variable "void *dummy" in Heap::markRoots(). The address of dummy is passed as a parameter to Heap::gatherStackRoots(), which passes it to MachineThreads::gatherConservativeRoots() which in turn passes it to MachineThread::gatherFromCurrentThread(). The address of dummy is used to mark the "top of the current stack". In MachineThread::gatherFromCurrentThread(), we use setjmp() to get all the register values. If a value we want to mark is in a callee save register and that register is saved and subsequently overwritten in either Heap::gatherStackRoots() or MachineThreads::gatherConservativeRoots(), then it won't show up as saved on the stack or in the register value we get from calling setjmp(). This can be fixed by having MachineThread::gatherFromCurrentThread() use one of its locals for the top of stack. rdar://problem/19440752
Created attachment 244433 [details] Patch
Comment on attachment 244433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244433&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + Move the address of the local variable that is used to mark the top of the stack for By “mark the top”, do you mean “demarcate the top"?
Comment on attachment 244433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244433&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:242 > + // We need to mark the stack top in this function so that callee saves are either already on the stack, I think “mark” here should also be “demarcate”. Marking the top of the stack in GC code has a different meaning then demarcating the top of the stack. I think you meant "indicating the top of the stack" (demarcating) instead of "recording as live” (marking). Is that right?
(In reply to comment #3) > Comment on attachment 244433 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244433&action=review > > > Source/JavaScriptCore/heap/MachineStackMarker.cpp:242 > > + // We need to mark the stack top in this function so that callee saves are either already on the stack, > > I think “mark” here should also be “demarcate”. Marking the top of the > stack in GC code has a different meaning then demarcating the top of the > stack. I think you meant "indicating the top of the stack" (demarcating) > instead of "recording as live” (marking). Is that right? I'll change the comment to "demarcate".
Committed r178266: <http://trac.webkit.org/changeset/178266>
This broke a JS test: 2015-01-12 09:21:06.385 testapi[45813:35152987] TEST: "weak value == nil": FAILED
Re-opened since this is blocked by bug 140363
Comment on attachment 244433 [details] Patch Rolled out in <https://trac.webkit.org/r178284>.
Created attachment 244499 [details] Updated patch that fixes broken API test
Comment on attachment 244499 [details] Updated patch that fixes broken API test View in context: https://bugs.webkit.org/attachment.cgi?id=244499&action=review r=me > Source/JavaScriptCore/ChangeLog:16 > + We now get the register contents at the same place that we demarcate the current top of > + stack using the address of a local variable, in Heap::markRoots(). The register contents > + buffer is passed along with the demarcation pointer. These need to be done at this level > + in the call tree and no lower, as markRoots() calls various functions that visit object > + pointers that may be latter proven dead. Any of those pointers that are left on the > + stack or in registers could be incorrectly marked as live if we scan the stack contents > + from a called function or one of its callees. The stack demarcation pointer and register > + saving need to be done in the same function so that we have a consistent stack, active > + and spilled registers. I think you should also have a comment about the original bug and why this fix works i.e. that flushing in non-volatile registers in gatherFromCurrentThread() is too late because those registers may have been spilled on the stack and replaced with other values by the time the GC got to gatherFromCurrentThread(), and that this patch fixes it by ensuring that they are spilled into the MachineThreads::RegisterState where we demarcate the top of stack for GC scanning. Hence, any root values are guaranteed to be either on the stack or in the captured RegisterState.
Committed r178364: <http://trac.webkit.org/changeset/178364>