[webkit-reviews] review denied: [Bug 119657] JSC: Change StackIterator to not require writes to the JS stack : [Attachment 208481] the patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 119657: JSC: Change StackIterator to not require writes to the JS stack
https://bugs.webkit.org/show_bug.cgi?id=119657

Attachment 208481: the patch.
https://bugs.webkit.org/attachment.cgi?id=208481&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
In C++ code, I'd like CallFrame to be the only class that encodes the layout of
the JavaScript stack in memory. It's OK if other classes add features on top of
CallFrame. But they should access those features through the CallFrame
abstraction, instead of doing direct memory reads and pointer accesses relative
to a casted CallFrame pointer.

These parts of your patch don't meet this goal:

- StackIterator::FrameReader::fromReaderFrame
    - reinterpret_casts between Frame* and Register*
    - reinterpret_casts between Register* and FrameReader*
    - Uses manual pointer math to calculate the "beginning" of the stack frame.


I put "beginning" in quotes because, once Michael is finished reversing the
stack, this code, due to its fragile design, will no longer access the true
beginning, and will get the wrong answer.

- FrameReader::m_scratchRegisters
    - Attempts a bitwise copy of the CallFrame in memory

I say "attempts" because, once Michael is finished,
StackIterator::FrameReader::frame() will likely get the wrong answer.

- StackIterator::Frame
    - publicly inherits from CallFrame
    - create() function reinterpret_casts between CallFrame* and Frame*

Public inheritance communicates an "is a" relationship. A StackIterator::Frame
is *not* a CallFrame, since it represents data that might have come from a
CallFrame, or might have come from a logically inlined function that
corresponds to no physical CallFrame.

What I'd like to see instead is for the iterator class to hold a CallFrame*, a
"next action" indicator, which tells you if your next action should be to go to
the next inlined function, or to go to the next CallFrame, and a normal
struct/class that contains the data we care about -- JSFunction*, CodeBlock*,
etc. Incrementing the iterator should use the "next action" indicator to decide
what to do, decode the relevant information into the struct/class, and update
the "next action" indicator. (A "last action" indicator would be a fine
equivalent, if that works better.)

Later, the transition to a callback-based API can remove the "next action"
indicator, since it will be implicit based on control flow on the stack, and
can pass a reference to the struct/class, allocated on the stack, to the
callback function.