[webkit-reviews] review denied: [Bug 125719] CStack Branch: Need an implementation of sanitizeStack for C stack : [Attachment 219338] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 125719: CStack Branch: Need an implementation of sanitizeStack for C stack
https://bugs.webkit.org/show_bug.cgi?id=125719

Attachment 219338: Patch
https://bugs.webkit.org/attachment.cgi?id=219338&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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*".