986680 - Add jit-time checks for scratch register misuse

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: DetectTempMisuse-r0.patch

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8395044 [details] [diff] [review]
DetectTempMisuse-r0.patch

Review of attachment 8395044 [details] [diff] [review]:
-----------------------------------------------------------------

This sounds like a safe and good thing to assert about, but not something that we want to base our logic on on the future.  So we should definitely make these data structures no-op on optimized builds.

::: js/src/jit/IonMacroAssembler.cpp
@@ +524,1 @@
>          break;

style-nit:
  case Foo: {
    code;
    code;
    break;
  }

?

::: js/src/jit/Registers.h
@@ +147,5 @@
> +        inUse_ = true;
> +    }
> +    void unuse() {
> +        inUse_ = false;
> +    }

bool inUse_ and accessors and modifiers should be DEBUG only functions.  And we should not base our logic on the availability of these registers, so making it Debug only should be fine.

@@ +166,5 @@
> +class AutoTemp {
> +    CheckedTemp<RegisterType> &check_;
> +    bool init_;
> +    bool overridden_;
> +    RegisterType override_;

Same here, these 3 fields should be DEBUG only.

@@ +169,5 @@
> +    bool overridden_;
> +    RegisterType override_;
> +  public:
> +    AutoTemp(CheckedTemp<RegisterType> &check) : check_(check), init_(false), overridden_(false) {
> +    }

style-nit: move the initialization list on new lines.

@@ +176,5 @@
> +    }
> +    operator RegisterType() {
> +        if (overridden_) {
> +            return override_;
> +        }

style-nit: remove braces.

Comment 2

[Nicolas B. Pierron [:nbp]]

CC: rankov, this patch will cause MIPS to go out of sync as Scratch registers are moving to the Assembler as CheckTemp<Register>.

Comment 3

[Branislav Rankov [:rankov]]

I will add these changes to MIPS port. Should I add the patch to this bug?

Comment 4

[Marty Rosenberg [:mjrosenb]]

(In reply to Branislav Rankov from comment #3)
> I will add these changes to MIPS port. Should I add the patch to this bug?

Sure! Out of curiosity, do you have instructions for cross-compiling to mips, or do you just build on a mips machine? If not, I'll just hit things with hammers, until mips compiles for me :-)

Comment 5

[Branislav Rankov [:rankov]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #4)
> Sure! Out of curiosity, do you have instructions for cross-compiling to
> mips, or do you just build on a mips machine? If not, I'll just hit things
> with hammers, until mips compiles for me :-)

The crosscompile instructions are here: https://github.com/rankov-img/gecko-dev/wiki/Linux-build
But MIPS port is still not completely submitted. There are still pieces missing on mozilla-central.

Comment 6

[Branislav Rankov [:rankov]]

I've been working on making this work for MIPS. 

I'm having trouble with cases where i have function f1 and f2 that both use ScratchRegister. Function f1 calls f2 and after that, f1 doesn't use ScratchRegister. Function f2 will use ScratchRegister, but this is not a problem because f1 no longer needs it.

I cannot handle this case with current setup and I have a few cases where I cannot use other scratch.

My proposal is to add a unuse function to AutoTemp class that would look like this:
    RegisterType unuse() {
        JS_ASSERT(check_.inUse());
        check_.unuse();
        return check_.reg_;
    }

I would that call this function in f1 before calling f2. This would enable the case I described above.

Do you think this is a good solution?

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Branislav Rankov from comment #6)
> I've been working on making this work for MIPS. 
> 
> I'm having trouble with cases where i have function f1 and f2 that both use
> ScratchRegister. Function f1 calls f2 and after that, f1 doesn't use
> ScratchRegister. Function f2 will use ScratchRegister, but this is not a
> problem because f1 no longer needs it.
> 
> I cannot handle this case with current setup and I have a few cases where I
> cannot use other scratch.
> 
> My proposal is to add a unuse function to AutoTemp class that would look
> like this:
>     RegisterType unuse() {
>         JS_ASSERT(check_.inUse());
>         check_.unuse();
>         return check_.reg_;
>     }
> 
> I would that call this function in f1 before calling f2. This would enable
> the case I described above.
> 
> Do you think this is a good solution?

From what I understand f2 is stealing the register from f1, right?  Can we accept an AutoTemp as argument of f2?  Otherwise, would the following work:

  void f1() {
    AutoTempRegister tmp(…);
    f2(Register(tmp));
  }

Comment 8

[Branislav Rankov [:rankov]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> From what I understand f2 is stealing the register from f1, right?  Can we
> accept an AutoTemp as argument of f2?  Otherwise, would the following work:
> 
>   void f1() {
>     AutoTempRegister tmp(…);
>     f2(Register(tmp));
>   }

You are right. Function f2 is stealing scratch register. 

Your solution might work. I would have to add an optional scratch parameter to f2. I will check if this works for all the cases and let you know.

Comment 10

[Sean Stangl [:sstangl]]

Poke.

VIXL has a mechanism for asserting scratch register safety similar to what nbp proposed. A bunch of problems on ARM64 were from scratch register collisions, and it's currently a minefield.

Comment 11

[Sean Stangl [:sstangl]]

I will take a look.

Comment 12

[Sean Stangl [:sstangl]]

Attachment: Part 1/4 - Define-AutoGenericRegisterScope.patch

Part 1 - Define debug-only scratch register checking data structure. This works for all platforms but ARM64, which needs runtime scratch register dispatch. ARM probably should also have runtime register dispatch, but it looks like we are just getting by without it, because the cases that require it are very rare.

Just asking for feedback for now. It passes tests but I'd like to test it more thoroughly before actually submitting for review.

Comment 13

[Sean Stangl [:sstangl]]

Attachment: Part 2/4 - Define-per-platform-ScratchRegisterScope.patch

Classes like this one are then used to avoid typing ScratchRegister all the time. The goal is to prevent the use of ScratchRegister-related errors by just removing the ScratchRegister keyword, as we did on ARM64.

I haven't yet made classes for floating point scratch registers or ARM's second scratch register, but those would follow the same format.

Comment 14

[Sean Stangl [:sstangl]]

Attachment: Part 3/4 - Convert-x64-to-ScratchRegisterScope.patch

This is X64 converted to use ScratchRegisterScope instead of ScratchReg. Looking for feedback on aesthetics. Passes all tests.

Comment 15

[Sean Stangl [:sstangl]]

Attachment: Part 4/4 - Convert-ARM-to-ScratchRegisterScope.patch

And the same deal for ARM.

The cases that could actually trip the asserts are extremely rare and usually based on specific offsets given to jumps. I can see how these remained hidden for a long time.

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8647780 [details] [diff] [review]
Part 1/4 - Define-AutoGenericRegisterScope.patch

Review of attachment 8647780 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking over this, I really like this approach even if it will require way more work to get the instrumentation in every instruction. :)

::: js/src/jit/MacroAssembler.cpp
@@ +2774,5 @@
> +
> +#ifdef DEBUG
> +template <>
> +TypedRegisterSet<Register>&
> +MacroAssembler::getTrackedRegisterSet<Register>()

Double check with "make check-masm", but be aware that template specialization is not supported by the check_macroassembler_style.py script at the moment. (Bug 1184958)

@@ +2798,5 @@
> +    TypedRegisterSet<RegisterType>& set = masm.getTrackedRegisterSet<RegisterType>();
> +
> +    // Check that the register is not already owned by a competing scope object.
> +    MOZ_ASSERT(!set.hasRegisterIndex(reg));
> +    set.addRegisterIndex(reg);

We should not manipulate TypedRegisterSet without wrappers such as LiveSet, or AllocatableSet.

This is code duplicates the logic of a  LiveSet<TypedRegisterSet<RegisterType>>, which will only catch errors if the same register is used, but not if aliased registers are used at the same time.  For example, the register index does not prevent you from using the Float32ScratchReg and the DoubleScratchReg, even if both overlap in memory.

I suggest to use an  AllocatableRegisterSet  as a backend for storing registers which are not allocated yet.
In addition, the  AllocatableRegisterSet  already does these assertions for you ;)
Also the polymorphism of the add/take functions give you even some support for AnyRegister, ValueOperand, and TypedOrValueRegister.

::: js/src/jit/MacroAssembler.h
@@ +417,5 @@
> +#ifdef DEBUG
> +    // Used to track register scopes for debug builds.
> +    // Manipulated by the AutoGenericRegisterScope class.
> +    RegisterSet debugTrackedRegisters_;
> +    template <class T> TypedRegisterSet<T>& getTrackedRegisterSet();

Following the previous comment, I suggest to use an  AllocatableRegisterSet  instead of a RegisterSet

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +32,5 @@
>  {
>    protected:
> +    // Perform a downcast. Should be removed by Bug 996602.
> +    MacroAssembler& asMasm();
> +    const MacroAssembler& asMasm() const;

These function should remain private, even if we have to duplicate them in the MacroAssemblerARM and in the MacroAssemblerArmCompat.
If these functions are not private, then they woud be visible in the generic MacroAssembler, which exactly what I wanted to avoid, such that the uses of these functions are highlighting where we still have some technical debt in our MacroAssembler.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8647784 [details] [diff] [review]
Part 2/4 - Define-per-platform-ScratchRegisterScope.patch

Review of attachment 8647784 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Assembler-arm.h
@@ +50,5 @@
> +// of code thinks it has exclusive ownership of the scratch register.
> +struct ScratchRegisterScope : public AutoRegisterScope
> +{
> +    ScratchRegisterScope(MacroAssembler& masm)
> +      : AutoRegisterScope(masm, ScratchRegister)

Would it be possible to inline the definition of ScratchRegister inside the initialization list, and remove the above definition of ScratchReg?

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8647787 [details] [diff] [review]
Part 4/4 - Convert-ARM-to-ScratchRegisterScope.patch

Review of attachment 8647787 [details] [diff] [review]:
-----------------------------------------------------------------

I will review more carefully as soon as the previous patches are good, but this patch looks fine at first sight.

Comment 19

[Sean Stangl [:sstangl]]

Attachment: Part 1/4 - Define-AutoGenericRegisterScope.patch

Comment 20

[Sean Stangl [:sstangl]]

Attachment: Part 2/4 - Define-per-platform-Scopes.patch

Comment 21

[Sean Stangl [:sstangl]]

Attachment: 0003-Convert-x64-and-x86-to-Scopes.patch

Comment 22

[Sean Stangl [:sstangl]]

Attachment: Part 4/4 - Convert-ARM-to-ScratchRegisterScope.patch

Comment 23

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8649557 [details] [diff] [review]
Part 1/4 - Define-AutoGenericRegisterScope.patch

Review of attachment 8649557 [details] [diff] [review]:
-----------------------------------------------------------------

Fix the following comment and r=me.

::: js/src/jit/MacroAssembler.h
@@ +422,5 @@
>    public:
> +#ifdef DEBUG
> +    // Used to track register scopes for debug builds.
> +    // Manipulated by the AutoGenericRegisterScope class.
> +    AllocatableRegisterSet debugTrackedRegisters_;

This section is reserved for the "Frame manipulation functions".  I do not think this fields belongs in this section.
Would it make sense to make a new section for the "Register allocation fields"?

Also, this field deserves to be private. Thus this class should marked AutoRegisterScope and AutoFloatRegisterScope as friends.
Add the friend annotation right above this field's comment.

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8649558 [details] [diff] [review]
Part 2/4 - Define-per-platform-Scopes.patch

Review of attachment 8649558 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Assembler-arm.h
@@ +52,5 @@
> +{
> +    ScratchRegisterScope(MacroAssembler& masm)
> +      : AutoRegisterScope(masm, ScratchRegister)
> +    { }
> +};

This sounds like a recurrent pattern, I don't think the following will work, as this implies that we giving an instance as a template parameter:

template <Register r>
struct RegisterScope : public AutoRegisterScope
{
    RegisterScope(MacroAssembler& masm)
      : AutoRegisterScope(masm, r)
    { }
};

and replace the above lines by:

using ScratchRegisterScope = RegisterScope<ScratchRegister>;

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8649559 [details] [diff] [review]
0003-Convert-x64-and-x86-to-Scopes.patch

Review of attachment 8649559 [details] [diff] [review]:
-----------------------------------------------------------------

Fix the following and r=me.
Thanks :)

::: js/src/jit/x64/BaselineIC-x64.cpp
@@ +162,5 @@
> +      case JSOP_URSH: {
> +        // The scratch register is only used in the case of !allowDouble_
> +        // below, which immediately jumps to the revertRegister label.
> +        // The ScratchRegisterScope is picked up again at that label.
> +        ScratchRegisterScope scratch(masm);

I don't think this is wise, as this scope does not cover all the time where this register is live.
It seems that we should do something like:

  mozilla::Maybe<ScratchRegisterScope> scratch;
  …
    case JSOP_URSH:
      if (!allowDouble) {
          scratch.emplace(masm);
          masm.movq(R0.valueReg(), scratch.ref());
      }
   …
   if (op_ == JSOP_URSH && !allowDouble_) {
       …
       masm.movq(scratch.ref(), R0.valueReg());
       …
   }

Also note that in addition to keep the register while it is supposed to be live, this would prevent us from doing emplace(masm)  twice in a row, and will also prevent use from requesting a scratch register that we have not yet initialized.

@@ +216,5 @@
>  
>      // Revert the content of R0 in the fallible >>> case.
>      if (op_ == JSOP_URSH && !allowDouble_) {
> +        // Scope continuation from JSOP_URSH case above.
> +        ScratchRegisterScope scratch(masm);

See above comment.

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +493,5 @@
>      }
>  
>      void cmpPtr(Register lhs, const ImmWord rhs) {
> +        ScratchRegisterScope scratch(asMasm());
> +        MOZ_ASSERT(lhs != scratch);

follow-up: This function declares the scratch register as the top-level for the assertion, but the assertion belongs only to the else-part of the function.  We should (in a follow-up) move these to the else-part of these functions.

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +2756,4 @@
>      // but can't be reached because operands would get swapped (bug 1084404).
>      if (ins->lanesMatch(2, 3, 6, 7)) {
>          if (AssemblerX86Shared::HasAVX()) {
> +            ScratchSimdScope scratch(masm);

nit: Move this line above the condition, and remove the one from the else-part.

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8649560 [details] [diff] [review]
Part 4/4 - Convert-ARM-to-ScratchRegisterScope.patch

Review of attachment 8649560 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +610,5 @@
> +        // If the remainder is != 0, bailout since this must be a double.
> +        {
> +            // The bailout code also needs the scratch register.
> +            ScratchRegisterScope scratch(masm);
> +            masm.as_mov(scratch, lsl(lhs, 32 - shift), SetCC);

Comment that the scratch register purpose is only to be an unused target while setting the CC flags for the branch.

@@ +1932,5 @@
> +    {
> +        ScratchRegisterScope scratch(masm);
> +        masm.ma_mov(Imm32(viewType), scratch);
> +        masm.passABIArg(scratch);
> +    }

passABIArg is effectively a no-op which record a move.  This statement should wrap everything up to the callWithABI.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +1252,5 @@
> +        // Often this code is called with rt as the ScratchRegister.
> +        // The register is logically owned by the caller, so we cannot ask
> +        // for exclusive ownership here. If full checking is desired,
> +        // this function should take an explicit scratch register argument.
> +        const Register& scratch = ScratchRegister;

Move the   MOZ_ASSERT(rn != ScratchRegister);  here.

@@ +2313,4 @@
>  void
>  MacroAssemblerARMCompat::loadPtr(AbsoluteAddress address, Register dest)
>  {
> +    MOZ_ASSERT(dest != pc); // Use dest as a scratch register.

Nice catch.

Comment 27

[Sean Stangl [:sstangl]]

Tryserver success: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe8ce6471ac0

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7afe43b2288e
https://hg.mozilla.org/integration/mozilla-inbound/rev/277ae9b3ad6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbda2856156
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de8ebf59df0

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acda082f036e

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7afe43b2288e
https://hg.mozilla.org/mozilla-central/rev/277ae9b3ad6f
https://hg.mozilla.org/mozilla-central/rev/9bbda2856156
https://hg.mozilla.org/mozilla-central/rev/6de8ebf59df0
https://hg.mozilla.org/mozilla-central/rev/acda082f036e