[webkit-reviews] review granted: [Bug 116586] JIT probe for ARMv7 and ARM + bug fixes : [Attachment 203089] corrected patch.

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 116586: JIT probe for ARMv7 and ARM + bug fixes
https://bugs.webkit.org/show_bug.cgi?id=116586

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

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203089&action=review


r=me, with comments.

> Source/JavaScriptCore/jit/JITStubsARM.h:290
> +    // 2. Issue 1 means we will need to write to the stack location at
> +    //    ProbeContext.cpu.sp - 4. But if the user probe function had 
modified
> +    //    the value of ProbeContext.cpu.sp to point in the range between
> +    //    &ProbeContext.cpu.ip thru &ProbeContext.cpu.aspr, then the action
for
> +    //    Issue 1 may trash the values to be restored before we can restore
> +    //    them.

Do we really care if the user modified sp?  Seems like something we shouldn't
have to worry about.

> Source/JavaScriptCore/jit/JITStubsARMv7.h:393
> +    // 2. Issue 1 means we will need to write to the stack location at
> +    //    ProbeContext.cpu.sp - 4. But if the user probe function had 
modified
> +    //    the value of ProbeContext.cpu.sp to point in the range between
> +    //    &ProbeContext.cpu.ip thru &ProbeContext.cpu.aspr, then the action
for
> +    //    Issue 1 may trash the values to be restored before we can restore
> +    //    them.

Same comment as above.

> Source/JavaScriptCore/jit/JITStubsX86.h:196
> +    // 2. The user probe function may have altered the restore value of esp
to
> +    //    point to the vicinity of one of the restore values for the
remaining
> +    //    registers left to be restored.
> +    //    That means, for requirement 1, we may end up writing over some of
the
> +    //    restore values. We can check for this, and first copy the restore
> +    //    values to a "safe area" on the stack before commencing with the
action
> +    //    for requirement 1.

Ditto.

> Source/JavaScriptCore/jit/JITStubsX86_64.h:225
> +    // 2. The user probe function may have altered the restore value of esp
to
> +    //    point to the vicinity of one of the restore values for the
remaining
> +    //    registers left to be restored.
> +    //    That means, for requirement 1, we may end up writing over some of
the
> +    //    restore values. We can check for this, and first copy the restore
> +    //    values to a "safe area" on the stack before commencing with the
action
> +    //    for requirement 1.

Ditto