I believe we need to save and restore the modified registers in case one or more registers are callee saved on the relevant platforms.
Created attachment 318637 [details] Patch
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 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.
(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.
(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 on attachment 318637 [details] Patch Clearing flags on attachment: 318637 Committed r221012: <http://trac.webkit.org/changeset/221012>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34013544>