Bug 232373

Summary:	Macro assembler scratchRegister() is unsafe on ARMv7, MIPS and RISCV64
Product:	WebKit	Reporter:	Geza Lore <glore>
Component:	JavaScriptCore	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	saam
Priority:	P2
Version:	WebKit Nightly Build
Hardware:	All
OS:	All

Description Geza Lore 2021-10-27 04:43:52 PDT

Handing out the MacroAssembler scratch register is unsafe on ARMv7, MIPS and RISCV64, because these macro assemblers do not respect the DisallowMacroScratchRegisterUsage RAII exclusion. Existing code happens to work, but is prone to silent errors on change. Example usage:

https://github.com/WebKit/WebKit/blob/4117bbbfeba9d1095ddd152b2ddc876bd8b641cc/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp#L307-L310

Comment 1 Saam Barati 2021-10-27 10:00:58 PDT

Can we add support for DisallowMacroScratchRegisterUsage?

Comment 2 Geza Lore 2021-10-27 10:08:47 PDT

On ARMv7 sometimes we need both scratch registers reserved for the macro assembler, so the best we might be able to do is a RELEASE_ASSERT when we handed out the scratch GPR but then end up needing it. Still would be better than the current situation. Other than that caveat, we could add support for this.

Comment 3 Saam Barati 2021-10-27 11:14:09 PDT

(In reply to Geza Lore from comment #2)
> On ARMv7 sometimes we need both scratch registers reserved for the macro
> assembler, so the best we might be able to do is a RELEASE_ASSERT when we
> handed out the scratch GPR but then end up needing it. Still would be better
> than the current situation. Other than that caveat, we could add support for
> this.

I think you're misunderstanding what DisallowMacroScratchRegisterUsage does. All it does is crash if we use the scratch register. It's not preventing us from using it.

Comment 4 Geza Lore 2021-10-27 12:51:43 PDT

(In reply to Saam Barati from comment #3)
> (In reply to Geza Lore from comment #2)
> > On ARMv7 sometimes we need both scratch registers reserved for the macro
> > assembler, so the best we might be able to do is a RELEASE_ASSERT when we
> > handed out the scratch GPR but then end up needing it. Still would be better
> > than the current situation. Other than that caveat, we could add support for
> > this.
> 
> I think you're misunderstanding what DisallowMacroScratchRegisterUsage does.
> All it does is crash if we use the scratch register. It's not preventing us
> from using it.

Yes I was wondering about that actually. In that case this should definitely work in all macro assemblers, though currently it does not.