Bug 175772 – [Win][Release] Crash when running testmasm executable.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Per Arne Vollan]

I believe we need to save and restore the modified registers in case one or more registers are callee saved on the relevant platforms.

[Per Arne Vollan]

Created attachment 318637 [details]
Patch

[Alex Christensen]

Comment on attachment 318637 [details]
Patch

Let's definitely just do this for win64, which has this as a requirement in its calling convention, and not for every platform.

[Mark Lam]

Comment on attachment 318637 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        We need to save and restore the modified registers in case one or more registers are callee saved
> +        on the relevant platforms.

When I wrote the tests, I chose argument registers on purpose because I thought they would not be callee saved registers.  Of course, I forgot that 32-bit x86 does not actually pass arguments in registers, and it turns out that argumentGPR3 is ebx, which is a callee saved register.

I think it doesn't hurt to do what you're doing with pushing and popping the argument registers for all platforms.  It makes the test agnostic of whether the target platform treats any of the registers as callee saved or not, and therefore, is more robust in a self-contained way.

[Mark Lam]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 318637 [details]
> Patch
> 
> Let's definitely just do this for win64, which has this as a requirement in
> its calling convention, and not for every platform.

Actually, this only needed for win32.  However, I think it doesn't hurt to make the test more robust and agnostic of platform details as to whether those registers are callee saved or not.  The test really doesn't care about that detail.  It only wants to set some register values for test.  So, I think we should go forward with this fix.

[Per Arne Vollan]

(In reply to Mark Lam from comment #4)
> (In reply to Alex Christensen from comment #2)
> > Comment on attachment 318637 [details]
> > Patch
> > 
> > Let's definitely just do this for win64, which has this as a requirement in
> > its calling convention, and not for every platform.
> 
> Actually, this only needed for win32.  However, I think it doesn't hurt to
> make the test more robust and agnostic of platform details as to whether
> those registers are callee saved or not.  The test really doesn't care about
> that detail.  It only wants to set some register values for test.  So, I
> think we should go forward with this fix.

Thanks for reviewing, all!

[WebKit Commit Bot]

Comment on attachment 318637 [details]
Patch

Clearing flags on attachment: 318637

Committed r221012: <http://trac.webkit.org/changeset/221012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34013544>