NEW 232373
Macro assembler scratchRegister() is unsafe on ARMv7, MIPS and RISCV64 
https://bugs.webkit.org/show_bug.cgi?id=232373
Summary Macro assembler scratchRegister() is unsafe on ARMv7, MIPS and RISCV64
Geza Lore
Reported 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
Attachments
Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2021-10-27 10:00:58 PDT
Can we add support for DisallowMacroScratchRegisterUsage?
Geza Lore
Comment 2 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.
Saam Barati
Comment 3 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.
Geza Lore
Comment 4 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.
Note You need to log in before you can comment on or make changes to this bug.