Bug 129169 – gatherFromOtherThread() needs to align the sp before gathering roots

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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>

[Geoffrey Garen]

> 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]

Created attachment 224905 [details]
the patch.

[Mark Lam]

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 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]

(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]

Thanks for the review.  Landed in r164500: <http:/trac.webkit.org/r164500>.