RESOLVED FIXED 129169
gatherFromOtherThread() needs to align the sp before gathering roots
https://bugs.webkit.org/show_bug.cgi?id=129169
Summary gatherFromOtherThread() needs to align the sp before gathering roots
Mark Lam
Reported 2014-02-21 13:58:03 PST
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>
Attachments
the patch. (3.29 KB, patch)
2014-02-21 14:09 PST, Mark Lam
ggaren: review+
Geoffrey Garen
Comment 1 2014-02-21 14:05:42 PST
> 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.
Mark Lam
Comment 2 2014-02-21 14:09:02 PST
Created attachment 224905 [details] the patch.
Mark Lam
Comment 3 2014-02-21 14:09:56 PST
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.
Geoffrey Garen
Comment 4 2014-02-21 14:14:21 PST
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.
Mark Lam
Comment 5 2014-02-21 14:16:04 PST
(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.
Mark Lam
Comment 6 2014-02-21 14:22:47 PST
Thanks for the review. Landed in r164500: <http:/trac.webkit.org/r164500>.
Note You need to log in before you can comment on or make changes to this bug.