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 | ||
Geza Lore
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
Can we add support for DisallowMacroScratchRegisterUsage?
Geza Lore
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
(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
(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.