MacroAssemblerArmv7 uses r6 as a temporary register, and it is a callee-saved register. The JITs use AssemblyHelpers::emitSaveCalleeSaves() and friends to save callee-saved registers, but there is no such mechanism in testmasm, which seems to make the assumption that the macroassembler does not use callee-saved registers (which I guess is true for all other architectures, but not for Armv7). This issue means that testmasm crashes on Armv7 since code generated by gcc uses r6, and it gets modified by JIT'ed code.
Created attachment 377243 [details] Patch Patch.
Comment on attachment 377243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377243&action=review LGTM. I left some comments. > Source/JavaScriptCore/ChangeLog:17 > + by gcc uses r6, and it gets modified by JIT'ed code. It would be good point to the source where you've found this information. Also, is it a problem if we use a different version of GCC or even Clang?
(In reply to Caio Lima from comment #2) > Comment on attachment 377243 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377243&action=review > > LGTM. I left some comments. > > > Source/JavaScriptCore/ChangeLog:17 > > + by gcc uses r6, and it gets modified by JIT'ed code. > > It would be good point to the source where you've found this information. > Also, is it a problem if we use a different version of GCC or even Clang? gcc, or any compiler, can use r6 and expect it not to change during a call to masm-compiled code because arm EABI says it is a callee-saved register. Do you think I should add a comment saying where we use it in MacroAssemblerARMv7? (answer: a bunch of methods, and more will likely be added in the future).
(In reply to Guillaume Emont from comment #3) > gcc, or any compiler, can use r6 and expect it not to change during a call > to masm-compiled code because arm EABI says it is a callee-saved register. In this case, I think this info is available into ARMv7 manual, right? Maybe it is worth it point to the section where this is stated. > Do you think I should add a comment saying where we use it in > MacroAssemblerARMv7? (answer: a bunch of methods, and more will likely be > added in the future). I'm not sure if this is necessary.Maybe a comment into `emitFunctionPrologue` is already sufficient.
Created attachment 377505 [details] Patch New patch.
Comment on attachment 377505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377505&action=review LGTM. I'm wondering if we should add a FIXME to add the concept of callee-saved register into macroassembler. > Source/JavaScriptCore/assembler/testmasm.cpp:187 > + // http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf Nice. I think there are some strange characters there.
Created attachment 377566 [details] Patch New patch with only ASCII chars.
Ping reviewer.
Comment on attachment 377566 [details] Patch r=me
Comment on attachment 377566 [details] Patch Clearing flags on attachment: 377566 Committed r249576: <https://trac.webkit.org/changeset/249576>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55115864>
<rdar://problem/55115866>