Bug 175772 - [Win][Release] Crash when running testmasm executable.
Summary: [Win][Release] Crash when running testmasm executable.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-21 09:17 PDT by Per Arne Vollan
Modified: 2017-08-22 08:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2017-08-21 09:19 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-08-21 09:17:34 PDT
I believe we need to save and restore the modified registers in case one or more registers are callee saved on the relevant platforms.
Comment 1 Per Arne Vollan 2017-08-21 09:19:51 PDT
Created attachment 318637 [details]
Patch
Comment 2 Alex Christensen 2017-08-21 14:40:08 PDT
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.
Comment 3 Mark Lam 2017-08-21 22:19:13 PDT
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.
Comment 4 Mark Lam 2017-08-21 22:21:07 PDT
(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.
Comment 5 Per Arne Vollan 2017-08-22 08:22:22 PDT
(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!
Comment 6 WebKit Commit Bot 2017-08-22 08:50:54 PDT
Comment on attachment 318637 [details]
Patch

Clearing flags on attachment: 318637

Committed r221012: <http://trac.webkit.org/changeset/221012>
Comment 7 WebKit Commit Bot 2017-08-22 08:50:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-08-22 08:51:32 PDT
<rdar://problem/34013544>