1303178 - Make ARM ScratchRegister Usage Explicit

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 0001-Bug-Make-ARM-ScratchRegister-usage-explicit.-r.patch

The ARM backend has had a number of security problems from improper ScratchRegister usage, even despite fuzzing. The problems kept occurring because the MacroAssemblerARM sometimes used a ScratchRegister for its own internal purposes, of which the caller was unaware.

We introduced ScratchRegisterScopes to allow fuzzing to solve these problems, which caught some bugs, but many still remained. Even with the scoped assertions, it was very difficult for the fuzzers to actually find these bugs, since the internal ScratchRegister usage was only on extremely rare fallback paths that get used when address space is mostly full.

This patch solves the problems by removing all internal acquisition of ScratchRegisters by the MacroAssemblerARM. No function beginning with "ma_" or "as_" may acquire a ScratchRegister. By doing this, the caller is always aware of ScratchRegister state, which must be provided to the MacroAssemblerARM to satisfy the scratch requirements of the most pessimistic case.

Although this doesn't guarantee that scratch register usage is correct, (that could still be done via static analysis given some constraints), it does provide sufficient bubbling of information so that the fuzzers are now extremely likely to catch scratch register misuse, and it should not be a problem in the future.

Note that ScratchDoubleReg is used outside of ARM code, unscoped, and so may still be misused, although the probability of that is lower since it is much rarer.

Gary: This should apply against 29af101880db.
Nicolas: Sorry for the huge review! Most of it is pretty mechanical.

Comment 1

[Nicolas B. Pierron [:nbp]]

I did a brief read-through, I think that this is a really good thing to have in terms of sanity.

The only detail, which I am inclined to accept, is that scratch registers are listed before default values, such as in ma_cmp.  I do not yet have any better alternative to deal with this problem.

I will carefully review this patch on Monday.

Comment 2

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !has(reg), at js/src/jit/RegisterSets.h:852
Build version: mozilla-central revision f1dbeb5dee22+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm
Runtime options: --fuzzing-safe --ion-eager --ion-offthread-compile=off --arm-hwcap=vfp

Testcase:

assertEq(((dbg | 0) % -1) | 0, 0);

Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x083720a6 in js::jit::SpecializedRegSet<js::jit::AllocatableSetAccessors<js::jit::RegisterSet>, js::jit::RegisterSet>::add (reg=..., this=0xf11784f4) at js/src/jit/RegisterSets.h:852
#0  0x083720a6 in js::jit::SpecializedRegSet<js::jit::AllocatableSetAccessors<js::jit::RegisterSet>, js::jit::RegisterSet>::add (reg=..., this=0xf11784f4) at js/src/jit/RegisterSets.h:852
#1  js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope (this=0xffffcd14, masm=..., reg=...) at js/src/jit/MacroAssembler.cpp:2873
#2  0x0834e90f in js::jit::ScratchRegisterScope::ScratchRegisterScope (masm=..., this=0xffffcd14) at js/src/jit/arm/Assembler-arm.h:54
#3  js::jit::MacroAssembler::branchTest32<js::jit::Label*> (cond=js::jit::Assembler::Equal, label=0xffffcd0c, rhs=..., lhs=..., this=0xf1178030) at js/src/jit/arm/MacroAssembler-arm-inl.h:1599
#4  js::jit::MacroAssembler::branchTestPtr (label=0xffffcd0c, rhs=..., lhs=..., cond=js::jit::Assembler::Equal, this=0xf1178030) at js/src/jit/arm/MacroAssembler-arm-inl.h:1630
#5  js::jit::MacroAssembler::branchTestStackPtr<js::jit::Imm32> (label=0xffffcd0c, t=..., cond=js::jit::Assembler::Equal, this=0xf1178030) at js/src/jit/MacroAssembler-inl.h:679
#6  js::jit::MacroAssembler::assertStackAlignment (offset=0, alignment=8, this=0xf1178030) at js/src/jit/MacroAssembler-inl.h:744
#7  js::jit::MacroAssembler::setupAlignedABICall (this=0xf1178030) at js/src/jit/MacroAssembler.cpp:2599
#8  0x0846d955 in js::jit::CodeGeneratorARM::visitSoftModI (this=0xf1178000, ins=0xf1172340) at js/src/jit/arm/CodeGenerator-arm.cpp:803
#9  0x0847d8c1 in js::jit::LSoftModI::accept (this=0xf1172340, visitor=0xf1178000) at js/src/jit/arm/LIR-arm.h:262
#10 0x0822cb54 in js::jit::CodeGenerator::generateBody (this=0xf1178000) at js/src/jit/CodeGenerator.cpp:5148
#11 0x0822d732 in js::jit::CodeGenerator::generate (this=0xf1178000) at js/src/jit/CodeGenerator.cpp:9261
#12 0x08256344 in js::jit::GenerateCode (mir=0xf11700f8, lir=0xf1171740) at js/src/jit/Ion.cpp:2010
#13 0x082bc2d2 in js::jit::CompileBackEnd (mir=0xf11700f8) at js/src/jit/Ion.cpp:2032
#14 0x082bced7 in js::jit::IonCompile (cx=cx@entry=0xf792d000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=0x0, constructing=false, recompile=false, optimizationLevel=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2303
#15 0x082bd5e2 in js::jit::Compile (cx=cx@entry=0xf792d000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=0x0, constructing=false, forceRecompile=false) at js/src/jit/Ion.cpp:2479
#16 0x082bd7ce in js::jit::CanEnter (cx=0xf792d000, state=...) at js/src/jit/Ion.cpp:2571
#17 0x086d2a7c in js::RunScript (cx=0xf792d000, state=...) at js/src/vm/Interpreter.cpp:377
[...]
#26 main (argc=6, argv=0xffffd884, envp=0xffffd8a0) at js/src/shell/js.cpp:7663
eax	0x0	0
ebx	0x8c18ff4	146903028
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0x1000	4096
edi	0xffffcd14	-13036
ebp	0xffffccd8	4294954200
esp	0xffffccd0	4294954192
eip	0x83720a6 <js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register)+86>
=> 0x83720a6 <js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register)+86>:	movl   $0x0,0x0
   0x83720b0 <js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register)+96>:	ud2    


This happens with high frequency, I can't continue testing without this issue being fixed first.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8791784 [details] [diff] [review]
0001-Bug-Make-ARM-ScratchRegister-usage-explicit.-r.patch

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

Ok, the core changes in ALUNeg, ma_alu, and ma_dataTransferN seems to be ok.
I still have to review carefully the rest of the mechanical changes in the upcoming days.

Just a few nits which might appear more than once in the rest of the patch:

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +3219,4 @@
>          masm.ma_vxfer(scratch, output);
>  
>          // int32_t(UINT32_MAX) == -1.
> +        masm.ma_cmp(output, Imm32(-1), scratchReg);

nit: masm.ma_cmn(output, Imm8(1));  ?

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +153,5 @@
>          // Test and bail for -0.0, when integer result is 0. Move the top word
>          // of the double into the output reg, if it is non-zero, then the
>          // original value was -0.0.
>          as_vxfer(dest, InvalidReg, src, FloatToCore, Assembler::Equal, 1);
> +        ma_cmp(dest, Imm32(0x80000000), scratch, Assembler::Equal);

nit: as_cmp(dest, Imm8(0x8000000), Assembler::NotEqual);

should work.  Note, Imm8 is about the encoding strategy and not the actual value (data:8, rot2:4)
This would remove the need for using a scratch register in convertDoubleToInt32.

Comment 4

[Sean Stangl [:sstangl]]

Attachment: 0001-Bug-1303178-Make-ARM-ScratchRegister-usage-explicit..patch

Updated patch to fix the issue Decoder found.

Comment 5

[Sean Stangl [:sstangl]]

(In reply to Christian Holler (:decoder) from comment #2)
> This happens with high frequency, I can't continue testing without this
> issue being fixed first.

Ah, there is a mild problem here: every anticipated fuzzing failure with this patch will have the same signature, but will be for different reasons. The most common failure should be a scoping error with ScratchRegisterScope, which will always fail in the same place (a class constructor).

We can go through a few iterations of this hoping that the number of errors is small, but if that goes on for a while it may be faster to just send me a dump of a few failing testcases so I can go through them all at once.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

for (var i = 0; i < 2; ++i) {
    print(!0 - 1);
}


$ ./js-dbg-32-dm-clang-intlDisabled-armSim-darwin-1303178-c4-2458e8837d15-eaf5eb6f8fa0 --fuzzing-safe --no-threads --ion-eager testcase.js
0
0

$ ./js-dbg-32-dm-clang-intlDisabled-armSim-darwin-1303178-c4-2458e8837d15-eaf5eb6f8fa0 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
0
1

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8792604 [details] [diff] [review]
0001-Bug-1303178-Make-ARM-ScratchRegister-usage-explicit..patch

The testcase mismatch on stdout in the prior comment happens often too.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Sean Stangl [:sstangl] from comment #5)
> Ah, there is a mild problem here: every anticipated fuzzing failure with
> this patch will have the same signature, but will be for different reasons.
> The most common failure should be a scoping error with ScratchRegisterScope,
> which will always fail in the same place (a class constructor).

Does MOZ_ALWAYS_INLINE undo debugger-tracking of the frame?  Failing that, maybe a (ugh) macro is necessary.

Comment 9

[Sean Stangl [:sstangl]]

Attachment: patch v3

Fixes the issue in Comment 6. The problem was in the template of branchSub32(), where the code was correct for all types except for Register. In its case, the code erroneously caused the output to be written to a scratch register -- since no scratch is necessary for a 2-register subtract, it was using (src1,src2,dest) form.

The diff from the previous patch is:

> --- a/js/src/jit/arm/MacroAssembler-arm-inl.h
> +++ b/js/src/jit/arm/MacroAssembler-arm-inl.h
> @@ -1564,8 +1564,7 @@ template <typename T>
>  void
>  MacroAssembler::branchSub32(Condition cond, T src, Register dest, Label* label)
>  {
> -    ScratchRegisterScope scratch(*this);
> -    ma_sub(src, dest, scratch, SetCC);
> +    sub32(src, dest);
>      j(cond, label);
>  }

Thanks for the fuzzing. That would have been very difficult to catch in review.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Sean Stangl [:sstangl] from comment #9)
> Created attachment 8793101 [details] [diff] [review]
> patch v3

Note: this patch applies to m-c rev 62f79d676e0e

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8792604 [details] [diff] [review]
0001-Bug-1303178-Make-ARM-ScratchRegister-usage-explicit..patch

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

This patch is a large amount of changes, and I admit I found it harder to read, mostly because of the modification of the arguments order on some ma_* functions, which is not replicated on all of them.  I guess this is an artifact of the method use to make this patch (error-message-driven patching) which is leaking in the final patch.

Overall this patch looks good, and I admit I did not caught the error reported by fuzzers on comment 6, during my review.  Still I think the following suggestion would have caught it.

I think we should more carefully distinguish the SratchRegister from any ordinary Register. If we add a new wrapper type, such as:

template <class RegisterType>
struct ScratchRegister : public RegisterType {
  const RegisterType reg;
  ScratchRegister(const ScratchRegister& other) = delete;
  explicit ScratchRegister(RegisterType&& reg) : reg(mozilla::Forward(reg)) {}
}

struct ScratchRegisterScope : public AutoGenericRegisterScope<ScratchRegister<Register>> { … };

Then we would be able to specialize the function arguments type to be ScratchRegister instead of Register:
 - This forbids implicit coercion from ScratchRegister to Register (comment 6).
 - This forbids implicit coercion from a Register into a ScratchRegister.
 - This would remove any need to reorder arguments, after of the addition of ScratchRegister in one signature.
 - Help distinguish between scratch register values used as operand (temp) from scratch register values used as values (logical).

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +1168,5 @@
> +
> +        // Load lhs into scratch2.
> +        if (lhs.offset != 0) {
> +            ma_add(base, Imm32(lhs.offset), scratch, scratch2);
> +            ma_ldr(DTRAddr(scratch, DtrRegImmShift(lhs.index, LSL, scale)), scratch2);

follow-up: This code is all over the place to handle BaseIndex inputs.  I feel that we could make a ma_BaseIndexToDTRAddr function which generates the ma_add and returns the right DTRAddr to be used in the load/store function.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ -291,5 @@
>  
> -    // Often this code is called with dest as the ScratchRegister.  The register
> -    // is logically owned by the caller after this call.
> -    const Register& scratch = ScratchRegister;
> -    MOZ_ASSERT(src1 != scratch);

Keep this assertion, as we erase the scratch register before reading the src1 register.

@@ +590,5 @@
>  
>  // Arithmetic-based ops.
>  // Add with carry.
>  void
> +MacroAssemblerARM::ma_adc(Imm32 imm, Register dest, Register scratch, SBit s, Condition c)

I see that for ma_and, ma_bic, … that the immediate value and the destination register are swapped.  My guess was that this was needed to create compiler errors while writting the patches, and avoid mixing cases where one instruction which had 2 register input and one immediate could be interpreted as an instruction with 1 input/output register and an immediate value.

This refactoring incidentally swapped the Imm32 and the destination register when there is no additional operands.  I would prefer if you can follow a single scheme here, either by swapping back the usage of ma_and, ma_bic, … or by converting ma_adc, ma_add, …

My understanding of the ARM backend, was that as_* function are read from right to left, while ma_* functions are read from left to right.  I think the easiest would be to continue to follow the scheme from left to right for ma_and, ma_bic, …

@@ +947,5 @@
>      // Begin the main loop.
>      bind(&head);
>      {
> +        // Extract the bottom bits.
> +        ma_and(Imm32(mask), tmp, scratch, scratch2);

(comment below)

@@ +954,3 @@
>          // Do a trial subtraction, this is the same operation as cmp, but we store
>          // the dest.
> +        ma_sub(dest, Imm32(mask), scratch, scratch2, SetCC);

The argument order is really confusing here, especially when compared to the ma_and just a few lines above.

@@ +3585,5 @@
>      int size = (sizeof(ResumeFromException) + 7) & ~7;
>  
> +    Imm8 size8(size);
> +    MOZ_ASSERT(!size8.invalid);
> +    as_sub(sp, sp, size8);

nit: No need to add a MOZ_ASSERT, as it would be done during the encoding of the instruction.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +160,4 @@
>      void ma_and(Register src1, Register src2, Register dest,
>                  SBit s = LeaveCC, Condition c = Always);
>  
> +    void ma_and(Register dest, Imm32 imm, Register scratch,

Changing the order of the arguments here is a no go.  Especially without a way to distinguish what is a scratch register, and what is an operand.  Same thing for ma_bic, ma_eor, ma_orr.

::: js/src/jit/arm/MoveEmitter-arm.cpp
@@ +308,5 @@
>          // Memory to memory move.
>          MOZ_ASSERT(from.isMemory());
> +        ScratchFloat32Scope scratchFloat32(masm);
> +        masm.ma_vldr(toAddress(from), VFPRegister(scratchFloat32).singleOverlay(), scratch);
> +        masm.ma_vstr(VFPRegister(scratchFloat32).singleOverlay(), toAddress(to), scratch);

existing nit: s/VFPRegister(scratchFloat32).singleOverlay()/scratchFloat32/

scratchFloat32 is already typed as a single, there is no need to take its single variant again.

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +175,5 @@
>      //
>      aasm->as_sub(r4, sp, O2RegImmShift(r1, LSL, 3));    // r4 = sp - argc*8
> +    {
> +        ScratchRegisterScope scratch(masm);
> +        masm.ma_and(Imm32(~(JitStackAlignment - 1)), r4, r4, scratch);

nit: as_bic(r4, r4, Imm8(…))

@@ +1184,5 @@
>      //      FrameDescriptor.size in scratch1
>      //      FrameDescriptor.type in scratch2
> +    {
> +        ScratchRegisterScope asmScratch(masm);
> +        masm.ma_and(Imm32((1 << FRAMETYPE_BITS) - 1), scratch1, scratch2, asmScratch);

follow-up: scratch[1-4] are really badly named, maybe naming them "temp" instead of scratch would help avoid the confusion here.

@@ +1233,5 @@
>          // scratch2 := StackPointer + Descriptor.size*1 + JitFrameLayout::Size();
>          masm.ma_add(StackPointer, scratch1, scratch2);
> +        {
> +            ScratchRegisterScope asmScratch(masm);
> +            masm.ma_add(scratch2, Imm32(JitFrameLayout::Size()), scratch2, asmScratch);

nit: JitFrameLayout::Size is unlikely to ever become larger than 256.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8793101 [details] [diff] [review]
patch v3

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

(see comment 11)

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8793101 [details] [diff] [review]
patch v3

Didn't find anything immediately wrong after a day of fuzzing, thanks!

Comment 14

[Christian Holler (:decoder)]

Comment on attachment 8793101 [details] [diff] [review]
patch v3

Feedback+ from me, no patch-specific failures left here :)

Comment 15

[Sean Stangl [:sstangl]]

Attachment: patch v4

Addresses nits.

Instead of creating a new type of Register, I opted to just require a Scope for the scratch register argument. This still guarantees that the destination argument won't be confused, but nevertheless allows regular registers to be used as scratch, as is allowed.

This patch should not require fuzzing, as it's pretty much the same as the last patch except for the function signatures.

Comment 16

[Sean Stangl [:sstangl]]

Comment on attachment 8795022 [details] [diff] [review]
patch v4

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?

Not very easily. It addresses the problem in an extremely generic way, which doesn't paint a bulls-eye.

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The comments in this bug do, but nothing in the patch does.

>Which older supported branches are affected by this flaw?

All.

>If not all supported branches, which bug introduced the flaw?

>Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I do not, but they could be created. They are likely very similar, since the code has not been changed in a while and the issue is long-standing. Low-risk.

>How likely is this patch to cause regressions; how much testing does it need?

It has already undergone fuzzing. It is unlikely to cause regressions, since the changes to the type signatures of the functions with the problem should have caught any errors of ScratchRegister misuse.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8795022 [details] [diff] [review]
patch v4

sec-approval+ for trunk.

Is this something that we should backport to other branches or just let ride the train from trunk?

Comment 18

[Sean Stangl [:sstangl]]

(In reply to Al Billings [:abillings] from comment #17)
> Is this something that we should backport to other branches or just let ride
> the train from trunk?

Let's see if there's a noticeable drop in the crash rate first. If there is, it's worth the time to backport it.

Comment 19

[Sean Stangl [:sstangl]]

Patch v4 applies cleanly to mozilla-inbound.

Comment 20

[Sean Stangl [:sstangl]]

Attachment: patch v5

Rebased to apply cleanly to m-i.

My ssh key for checking appears to have been revoked, so I can't check it in myself until that gets fixed.

Comment 21

[Sean Stangl [:sstangl]]

Hannes just landed Bug 1301400, which conflicts with this patch, so it can't be landed currently. (The git mirror I was using was a bit delayed.)

Comment 22

[Sean Stangl [:sstangl]]

Attachment: patch v6

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Sec-approval was given, landed on m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4875921ecf

Comment 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/83a86d52b0388f1eae91109ce393a12bd4a0dd9e for check_macroassembler_style.py bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=36858964&repo=mozilla-inbound

Comment 25

[Sean Stangl [:sstangl]]

Attachment: patch v7

Passes make check-masm.

Unfortunately, it is impossible to use a template specialization in the MacroAssembler without modifying the checking script.

Diff from previous version:

>diff --git a/js/src/jit/arm/MacroAssembler-arm-inl.h b/js/src/jit/arm/MacroAssembler-arm-inl.h
>index 677d79a..b869057 100644
>--- a/js/src/jit/arm/MacroAssembler-arm-inl.h
>+++ b/js/src/jit/arm/MacroAssembler-arm-inl.h
>@@ -1414,16 +1414,7 @@ template <typename T>
> inline CodeOffsetJump
> MacroAssembler::branchPtrWithPatch(Condition cond, Register lhs, T rhs, >RepatchLabel* label)
> {
>-    ma_cmp(lhs, rhs);
>-    return jumpWithPatch(label, cond);
>-}
>-
>-template <>
>-inline CodeOffsetJump
>-MacroAssembler::branchPtrWithPatch(Condition cond, Register lhs, ImmGCPtr rhs, >RepatchLabel* label)
>-{
>-    ScratchRegisterScope scratch(*this);
>-    ma_cmp(lhs, rhs, scratch);
>+    cmpPtr(lhs, rhs);
>     return jumpWithPatch(label, cond);
> }
> 
>@@ -1431,23 +1422,12 @@ template <typename T>
> inline CodeOffsetJump
> MacroAssembler::branchPtrWithPatch(Condition cond, Address lhs, T rhs, >RepatchLabel* label)
> {
>-    ScratchRegisterScope scratch(*this);
>     SecondScratchRegisterScope scratch2(*this);
>-
>-    ma_ldr(lhs, scratch, scratch2);
>-    ma_cmp(scratch, rhs);
>-    return jumpWithPatch(label, cond);
>-}
>-
>-template <>
>-inline CodeOffsetJump
>-MacroAssembler::branchPtrWithPatch(Condition cond, Address lhs, ImmGCPtr rhs, >RepatchLabel* label)
>-{
>-    ScratchRegisterScope scratch(*this);
>-    SecondScratchRegisterScope scratch2(*this);
>-
>-    ma_ldr(lhs, scratch, scratch2);
>-    ma_cmp(scratch, rhs, scratch2);
>+    {
>+        ScratchRegisterScope scratch(*this);
>+        ma_ldr(lhs, scratch2, scratch);
>+    }
>+    cmpPtr(scratch2, rhs);
>     return jumpWithPatch(label, cond);
> }

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fff275b1467016c8b312db9d596cbb21c379f2e

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fff275b1467