Bug 201138 – testmasm: save r6 in JIT'ed code on ARM_THUMB2

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Guillaume Emont]

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.

[Guillaume Emont]

Created attachment 377243 [details]
Patch

Patch.

[Caio Lima]

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?

[Guillaume Emont]

(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).

[Caio Lima]

(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.

[Guillaume Emont]

Created attachment 377505 [details]
Patch

New patch.

[Caio Lima]

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.

[Guillaume Emont]

Created attachment 377566 [details]
Patch

New patch with only ASCII chars.

[Guillaume Emont]

Ping reviewer.

[Mark Lam]

Comment on attachment 377566 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 377566 [details]
Patch

Clearing flags on attachment: 377566

Committed r249576: <https://trac.webkit.org/changeset/249576>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55115864>

[Radar WebKit Bug Importer]

<rdar://problem/55115866>