Bug 125719 – CStack Branch: Need an implementation of sanitizeStack for C stack

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

We need  an equivalent to JSStack::sanitizeStack() for running on the C stack.

[Michael Saboff]

Created attachment 219215 [details]
Work in progress

This implementation works, but only zeros on pointer size per loop iteration.  Also, this doesn't include the necessary GC changes.

[Michael Saboff]

Created attachment 219216 [details]
work in progress

Here it is with other work in progress removed.

[Michael Saboff]

Created attachment 219338 [details]
Patch

[Geoffrey Garen]

Comment on attachment 219338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219338&action=review

This doesn't look right.

> Source/JavaScriptCore/heap/Heap.cpp:462
> +#if ENABLE(JIT)
> +        sanitizeStackForVM(m_vm);
> +#endif

It's a bad idiom to conditionalize callers to sanitizeStack. Callers don't know anything about how sanitizeStack is implemented -- they just know that they are a good place to sanitize the stack -- so callers should not be responsible for deciding which kind of sanitization should happen. Instead, any #ifdef- or platform-specific sanitize decisions should happen inside the function implementation.

As it stands, this idiom caused you to introduce a bug: CLoop builds do not sanitize the stack anymore during GC.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:480
> +_sanitizeStackForVM:

Why does this function need to be written in assembly? It's just a loop. I think it would be better to write a shorter function that just returned sp, and to call that function from C++. Then you could write the actual loop in C++, and even use memset.

> Source/JavaScriptCore/runtime/VM.cpp:229
> +    setLastStackTop(stack.origin());

It is bad when one thing becomes two. Is this value a "stack top" or a "stack origin"?

Let's call this function setLastStackOrigin, and the data member m_lastStackOrigin, to match the existing name.

Why is it OK to set this value only when constructing the VM? Are you assuming that a given VM will only ever run on the thread on which it's constructed? That assumption is false.

> Source/JavaScriptCore/runtime/VM.h:381
> +        void setLastStackTop(void *lastStackTop) { m_lastStackTop = lastStackTop; }

Should be "void*".

> Source/JavaScriptCore/runtime/VM.h:510
> +        void *m_lastStackTop;

Should be "void*".

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 219338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219338&action=review
> 
> This doesn't look right.
> 
> > Source/JavaScriptCore/heap/Heap.cpp:462
> > +#if ENABLE(JIT)
> > +        sanitizeStackForVM(m_vm);
> > +#endif
> 
> It's a bad idiom to conditionalize callers to sanitizeStack. Callers don't know anything about how sanitizeStack is implemented -- they just know that they are a good place to sanitize the stack -- so callers should not be responsible for deciding which kind of sanitization should happen. Instead, any #ifdef- or platform-specific sanitize decisions should happen inside the function implementation.

It actually should be #if !ENABLE(LLINT_CLOOP).  It is only needed when we are on the C stack.  I will move the #ifdefs to the implementation and remove it here.

> As it stands, this idiom caused you to introduce a bug: CLoop builds do not sanitize the stack anymore during GC.

The CLoop sanitizeStack call using the JSStackis a few lines below in the same function in a section of code that is protected by #if ENABLE(LLINT_CLOOP).  In that case we need to call JSStack::sanitizeStack() a different function.

> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:480
> > +_sanitizeStackForVM:
> 
> Why does this function need to be written in assembly? It's just a loop. I think it would be better to write a shorter function that just returned sp, and to call that function from C++. Then you could write the actual loop in C++, and even use memset.

If sanitizeStack is written in C++ it will be using the same stack that it wants to clear.  Although we could get things working as a C++ leaf function, any calls out to memset or possible future changes would be problematic as we don't know how much stack is required.  Writing a simple leaf function in the LLInt eliminated these kinds of problems.  I already checked with Phil that a simple loop implementation was sufficient.

> > Source/JavaScriptCore/runtime/VM.cpp:229
> > +    setLastStackTop(stack.origin());
> 
> It is bad when one thing becomes two. Is this value a "stack top" or a "stack origin"?
> 
> Let's call this function setLastStackOrigin, and the data member m_lastStackOrigin, to match the existing name.

It is the last stack top.  It needs an initial value.  The logical initial value is the base of the stack which is stack.origin().
 
> Why is it OK to set this value only when constructing the VM? Are you assuming that a given VM will only ever run on the thread on which it's constructed? That assumption is false.

I'll add code to VMEntryScope() to set it and restore it similar to setStackLimit().

> > Source/JavaScriptCore/runtime/VM.h:381
> > +        void setLastStackTop(void *lastStackTop) { m_lastStackTop = lastStackTop; }
> 
> Should be "void*".

Fixed.

> > Source/JavaScriptCore/runtime/VM.h:510
> > +        void *m_lastStackTop;
> 
> Should be "void*".

Fixed.

[Michael Saboff]

Created attachment 219351 [details]
Updated patch

[Geoffrey Garen]

Comment on attachment 219351 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219351&action=review

> Source/JavaScriptCore/llint/LLIntThunks.h:52
> +    void sanitizeStackForVM(VM*) { }

Please change this function to call vm->stack().sanitizeStack(), and remove the explicit call to stack().sanitizeStack() from Heap.cpp.

> Source/JavaScriptCore/runtime/VM.cpp:229
> +    setLastStackTop(stack.origin());

Please rename lastStackTop to lastStackOrigin.

[Geoffrey Garen]

Comment on attachment 219351 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219351&action=review

>> Source/JavaScriptCore/runtime/VM.cpp:229
>> +    setLastStackTop(stack.origin());
> 
> Please rename lastStackTop to lastStackOrigin.

OK, I understand what you're saying now: we initialize to origin, but the value is not the same as origin -- our assembly code updates it as we go.

[Geoffrey Garen]

Comment on attachment 219351 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219351&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:479
> +# void sanitizeStackForVM(VM *vm)

This should be "VM*".

[Michael Saboff]

Created attachment 219394 [details]
Patch with changes suggested by Geoff

This has the "VM*" typo fix and sanitizeStackForVM() is now declared in VM.h and the C Loop version is defined in VM.cpp.

[Geoffrey Garen]

Comment on attachment 219394 [details]
Patch with changes suggested by Geoff

View in context: https://bugs.webkit.org/attachment.cgi?id=219394&action=review

r=me

> Source/JavaScriptCore/runtime/VM.cpp:737
> +void sanitizeStackForVM(VM *vm)

Should be "VM*".

[Michael Saboff]

Committed r160694: <http://trac.webkit.org/changeset/160694>