The GC scans the stacks of other threads using MachineThreads::gatherFromOtherThread(). gatherFromOtherThread() defines the range of the other thread's stack as being bounded by the other thread's stack pointer and stack base. While the stack base will always be aligned to sizeof(void*), the stack pointer may not be. This is because the other thread may have just pushed a 32-bit value on its stack before we suspended it for scanning. The fix is to round the stack pointer up to the next aligned address of sizeof(void*) and start scanning from there. We ignore the 32-bit word at the bottom of the stack (top of the stack for stacks growing up) because it cannot be a 64-bit pointer anyway since pointers should be stored on 64-bit aligned boundaries (our conservative scan algorithm already depends on this assumption). ref: <rdar://problem/15447622>
> The fix is to round the stack pointer up to the next aligned address of sizeof(void*) and start scanning from there. We ignore the 32-bit word at the bottom of the stack (top of the stack for stacks growing up) because it cannot be a 64-bit pointer anyway since pointers should be stored on 64-bit aligned boundaries (our conservative scan algorithm already depends on this assumption). I think your described fix will work -- but this explanation is wrong because it doesn't consider 32bit.
Created attachment 224905 [details] the patch.
Comment on attachment 224905 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=224905&action=review > Source/JavaScriptCore/ChangeLog:19 > + Pointers should always be stored on 64-bit aligned boundaries (our I'll change "Pointers" to "64-bit pointers" to be even more clearer.
Comment on attachment 224905 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=224905&action=review r=me > Source/JavaScriptCore/heap/MachineStackMarker.cpp:448 > + stackPointer = reinterpret_cast<void*>(WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<size_t>(stackPointer))); Let's use uintptr_t instead of size_t.
(In reply to comment #4) > > Source/JavaScriptCore/heap/MachineStackMarker.cpp:448 > > + stackPointer = reinterpret_cast<void*>(WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<size_t>(stackPointer))); > > Let's use uintptr_t instead of size_t. Sure, but StdLibExtras.h defined WTF::roundUpToMultipleOf() as taking a size_t.
Thanks for the review. Landed in r164500: <http:/trac.webkit.org/r164500>.