Open Bug 1451249 Opened 6 years ago Updated 2 years ago

[Static analysis] Ensure ScratchRegisterScope scopes are not nested

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: lth, Unassigned)

References

(Blocks 1 open bug)

Details

The context here is bug 1448552:

The JIT has an RAII class ScratchRegisterScope, which claims the integer scratch register.  When the scratch register has been claimed by a scope A and is subsequently claimed by another scope B while A is still active, there's a runtime assertion (in debug builds).  However, the claiming of the scratch register by B can be an unlikely event, it can depend on register pressure or the local bytecode shape.  Our tests do not cover all situations on all platforms.  We'd therefore like to perform a conservative static analysis that this situation cannot occur.

There are several complicating factors:

- ScratchRegisterScope is defined per-platform
- Not all platforms have ScratchRegisterScope (ARM64 for sure)
- The Wasm baseline compiler subclasses ScratchRegisterScope on some 
  platforms that have it with its own ScratchI32 type; on platforms that
  don't have ScratchRegisterScope, or where the baseline compiler uses
  a non-subclass definition anyway (ARM), we still want the analysis for
  ScratchI32
- We have ScratchDoubleScope and ScratchFloat32Scope ditto, with ScratchF32
  and ScratchF64 in the wasm baseline compiler (same situation, sometimes
  a subclass and sometimes not)
- ScratchDoubleScope and ScratchFloat32Scope may or may not alias; this is
  platform-dependent though I believe that at present, they always alias on
  all platforms.  If they do alias they should be treated as the same 
  scratch register, despite different names.
- ARM and MIPS also have SecondScratchRegisterScope definitions for another
  integer register and we'd like to handle those as well

Many of the RAII type definitions ultimately derive from AutoGenericRegisterScope<>.  The wasm baseline compiler's private ones do not and probably could not, but they could derive from some common marker type to make them clearly visible to analysis.
Yeah, that's a little tangled, but it sounds like a mechanism like __attribute__(("non-nestable-scope", "scratch1")) where the "scratch1" is a unique token for the sort of scope you're interested in would work. You'd have to arrange the conditional compilation to use the same token for aliased types, and make sure there's a different token for SecondScratchRegisterScope.

(The actual attribute probably wouldn't be quite that. __attribute__((tag("non-nestable-scope", "scratch1")) from js/public/GCAnnotations.h or __attribute__((annotate("moz_non_nestable_scope", "scratch1")) from mfbt/Attributes.h look more likely. But that's just syntax.)

Hopefully there aren't relationships between scratch registers, like if you could use either one 64-bit scratch register or two 32-bit scratch registers, but the 32-bit ones are actually the two halves of the 64-bit register? I guess that could be handled too, but would obviously require something more complex.
Blocks: hazard
(In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #1)
> 
> Hopefully there aren't relationships between scratch registers, like if you
> could use either one 64-bit scratch register or two 32-bit scratch
> registers, but the 32-bit ones are actually the two halves of the 64-bit
> register? I guess that could be handled too, but would obviously require
> something more complex.

At the moment, things should not be that complicated because we don't have int64 scratch registers on 32-bit systems and we don't have a second scratch register for any of the floating point types.  I don't see that changing, either.
No longer blocks: hazard
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.