957504 - crash in js::jit::DoTypeMonitorFallback on hardfp arm linux

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Oleg Romashin (:romaxa)]

in recent upstream I've found weird crash:
Backtrace:
#0  0x47172740 in js::jit::DoTypeMonitorFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) () from dist/bin/libxul.so
#1  0x420af834 in ?? ()
#2  0x420af834 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)


on Mer SDK target:
gcc -v
Using built-in specs.
COLLECT_GCC=/opt/cross/bin/armv7hl-meego-linux-gnueabi-gcc
COLLECT_LTO_WRAPPER=/opt/cross/libexec/gcc/armv7hl-meego-linux-gnueabi/4.6.4/lto-wrapper
Target: armv7hl-meego-linux-gnueabi
Configured with: ../configure --prefix=/opt/cross --mandir=/opt/cross/share/man --infodir=/opt/cross/share/info --disable-bootstrap --with-bugurl=http://bugs.merproject.org/ --build=i486-meego-linux --host=i486-meego-linux --target=armv7hl-meego-linux-gnueabi --with-sysroot=/opt/cross/armv7hl-meego-linux-gnueabi/sys-root --disable-multilib --disable-multilib --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-lto --enable-linker-build-id --enable-languages=c,c++,objc,obj-c++ --enable-threads=posix --enable-shared --disable-libgcj --with-float=hard --with-fpu=vfpv3-d16 --with-arch=armv7-a --build=i486-meego-linux
Thread model: posix
gcc version 4.6.4 20130102 (Mer 4.6.4-1) (Linaro GCC 4.6-2013.01) 
ARCH: armv7l GNU/Linux

First bad commit was 
https://hg.mozilla.org/mozilla-central/rev/d3cb4aa974a7
Bug 949668 - SpiderMonkey: Add a MoveOp::FLOAT32 r=jandem

Comment 1

[Douglas Crosher [:dougc]]

From romaxa on IRC: 'even if I apply patch from 956606, armv7hf build crashes on startup.'

So this looks like an easy bug to reproduce.

Comment 2

[Marty Rosenberg [:mjrosenb]]

Attachment: fixCorrectArg-r1.patch

My lack of familiarity with BC made finding this a bit slower than it should have been.  I'm also not entirely clear why this only shows up in HF builds in BC, since it should be everywhere.  The issue is if we have arguments that should go in r0, and r1, and the value that should be in r0 is already in r0, then subsequent arguments are moved up, and r0 gets overwritten. (and all other arguments are similarly wrong).

Comment 3

[Douglas Crosher [:dougc]]

Comment on attachment 8369884 [details] [diff] [review]
fixCorrectArg-r1.patch

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

Good find.

This is only in the hardfp specific code which would explain why only these builds were affected.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3524,4 @@
>      if (!enoughMemory_)
>          return;
>      switch (type) {
>        case MoveOp::FLOAT32:

Might it be appropriate to add a comment and assertion to catch attempts to pass move than one float32 argument as this would not be packed correctly and is not currently supported?

@@ +3533,5 @@
> +                if (type == MoveOp::FLOAT32)
> +                    passedArgTypes_ = (passedArgTypes_ << ArgType_Shift) | ArgType_Float32;
> +                else
> +                    passedArgTypes_ = (passedArgTypes_ << ArgType_Shift) | ArgType_Double;
> +                break;

There is both a 'break' and return here. The break will fall through to add a move, whereas the return would have skipped this. Might the move be redundant?  Does the move resolver optimize away a redundant move?

Comment 4

[Oleg Romashin (:romaxa)]

it does not crash with default just startup now, but it crashes on loading some page like linux.org.ru

with stdout message
Program received signal SIGBUS, Bus error.

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8369884 [details] [diff] [review]
fixCorrectArg-r1.patch

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

I don't have anything to add beyond Douglas' comments.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Suggested changes

Here's a suggestion.  Remove the 'break', and add an assertion.  I have not spotted any ABI calls with float32 arguments other than a single float32 argument?

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 8372932 [details] [diff] [review]
Suggested changes

With this patch it seems to work on all pages fine

Comment 8

[Douglas Crosher [:dougc]]

Attachment: Suggested changes

Can also assert if attempting to add more float arguments when there is already one float32 argument.

Comment 9

[Marty Rosenberg [:mjrosenb]]

Attachment: hf_compile-r0.patch

I think Doug's patch only checks the previous argument to see if it was a float32.  I think in general, if we ever mix the arguments, things will be out of whack, and we should just avoid using float32 with anything else at all, even another float32.

Comment 10

[Douglas Crosher [:dougc]]

Comment on attachment 8373120 [details] [diff] [review]
hf_compile-r0.patch

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

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3533,3 @@
>          FloatRegister fr;
>          if (GetFloatArgReg(usedIntSlots_, usedFloatSlots_, &fr)) {
>              if (from.isFloatReg() && from.floatReg() == fr) {

The assertion test looks good, better than my suggestion as it correctly allows preceding double arguments in registers.  For example, fn(double x, float y) should be compiled ok.

nit: Might it be appropriate to move the test into the 'GetFloatArgReg' success branch because it is only float32's passed in registers that cause problems?   This is not currently necessary because the only relevant case is a single float32 argument.

nit: Could you consider adding some source comments so cold readers know this is an implementation limitation and not an ABI limitation, and perhaps even note a bug number. Will you open a new bug for supporting multiple float32 arguments?

nit: The indentation is wrong.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8373120 [details] [diff] [review]
hf_compile-r0.patch

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

LGTM, with Douglas' nits addressed. Also, please make the patch description a little more descriptive :-).

Comment 12

[Marty Rosenberg [:mjrosenb]]

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/280aa953c868

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/280aa953c868

Comment 15

[Marty Rosenberg [:mjrosenb]]

Attachment: fixFloat32-r0.patch

hit everything with a sledgehammer until float32s work more or less correctly (only with lsra)  There are some namechanges that shouldn't go in, and were just there so that I'd get compile failures rather than mysterious behavior

Oh, and only arm has been ported to the new code,  A couple of shim functions need to be implemented for x86 + x64.

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8392167 [details] [diff] [review]
fixFloat32-r0.patch

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

::: js/src/jit/AsmJS.cpp
@@ +5953,5 @@
>  #if defined(JS_CODEGEN_ARM)
>  // The ARM system ABI also includes d15 in the non volatile float registers.
>  static const RegisterSet NonVolatileRegs =
>      RegisterSet(GeneralRegisterSet(Registers::NonVolatileMask),
> +                    FloatRegisterSet(FloatRegisters::NonVolatileDoubleMask | (1ull << FloatRegisters::d15 + 32)));

This is hardly readable, also isn't this supposed to be d16 ?

::: js/src/jit/CodeGenerator.cpp
@@ +1397,5 @@
> +          case LDefinition::FLOAT32: moveType = MoveOp::FLOAT32;
> +#ifdef JS_CPU_ARM
> +            if (from->isFloatReg()) {
> +                tmpfrom = LFloatReg(from->toFloatReg()->reg().singleOverlay());
> +                from = &tmpfrom;

This definitely sounds like a hack, the fact that we are using the register as a single should already be registered in the LAllocation by the register allocator.

::: js/src/jit/arm/Architecture-arm.h
@@ +248,5 @@
> +    static const uint32_t TotalDouble = 32;
> +    static const uint32_t TotalSingle = 32;
> +    static const uint32_t Allocatable = 30;
> +    // there are only 32 places that we can put values.
> +    static const uint32_t TotalPhys = 32;

This is extremely weird.  If we want to make this one the physical limit then I think the best would be to have it in bytes.

@@ +357,5 @@
> +    bool isInvalid() const;
> +    bool isMissing() const;
> +
> +    VFPRegister doubleOverlay(int which = 0) const;
> +    VFPRegister singleOverlay(int which = 0) const;

This sounds like a terrible interface, because the caller decides which single register it want to see.  So, from this I wonder how we can manage to allocate single register independently in the register allocator, and this does not sounds right.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +585,5 @@
> +    if (getenv("INST_DUMP")) {
> +        char buf[4096];
> +        sprintf(buf, "gdb /proc/%d/exe %d -batch -ex 'set pagination off' -ex 'set arm force-mode arm' -ex 'x/%di %p' -ex 'set arm force-mode auto'", getpid(), getpid(), m_buffer.size() / 4, buffer);
> +        system(buf);
> +    }

Nice trick, can you document that in the Hacking tips page.

::: js/src/jit/arm/Assembler-arm.h
@@ +95,5 @@
>  static MOZ_CONSTEXPR_VAR Register ReturnReg = r0;
> +static MOZ_CONSTEXPR_VAR FloatRegister ReturnDoubleReg = { FloatRegisters::d0, VFPRegister::Double };
> +static MOZ_CONSTEXPR_VAR FloatRegister ReturnFloat32Reg = { FloatRegisters::d0, VFPRegister::Single };
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchFloat32Reg = { FloatRegisters::d30, VFPRegister::Single };
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchDoubleReg = { FloatRegisters::d15, VFPRegister::Double };

ScratchFloat32Reg = { d30, Single }

This is weird, might not have an enumerated code for s30?  Also s30 alias d15, so we would need to pay attention in the macro assembler as we would have either 2 scratch float or a scratch double.

::: js/src/jit/x86/Assembler-x86.h
@@ +43,5 @@
>  static MOZ_CONSTEXPR_VAR Register StackPointer = esp;
>  static MOZ_CONSTEXPR_VAR Register FramePointer = ebp;
>  static MOZ_CONSTEXPR_VAR Register ReturnReg = eax;
> +static MOZ_CONSTEXPR_VAR FloatRegister ReturnFloat32Reg = xmm0;
> +static MOZ_CONSTEXPR_VAR FloatRegister ReturnDoubleReg = xmm0;

I would have expected to see a Float32Register and a DoubleRegister, is that intended for the future?

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8392167 [details] [diff] [review]
fixFloat32-r0.patch

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

Nice work! I began to note some nits but as this is a WIP, it's not really worth it.
I trust your work for everything related to the linear scan regalloc, as I don't have any knowledge about it.
My only non-nit concerns mainly concern Architecture-arm.h, see comments below.

::: js/src/jit/CodeGenerator.cpp
@@ +1329,5 @@
>      int32_t stack_offset = StackOffsetOfPassedArg(argslot);
>      Address dest(StackPointer, stack_offset);
> +    Label l;
> +    if (arg->isFloatReg() && lir->id() == 102 && ToFloatRegister(arg).code() == 33) {
> +        fprintf(stderr, "found him!\n");

yummy.

::: js/src/jit/LIR-Common.h
@@ +5954,5 @@
>      }
>  };
>  
> +#ifdef JS_CODEGEN_ARM
> +class LAssertRangeF : public LInstructionHelper<0, 1, 2>

Should be platform specific, then.

::: js/src/jit/Lowering.cpp
@@ +2982,5 @@
>          lir = new(alloc()) LAssertRangeD(useRegister(input), tempDouble());
>          break;
>  
>        case MIRType_Float32:
> +#ifdef JS_CODEGEN_ARM

These ifdefs tend to justify that such methods should go in the platform specific Lowering-* files.

::: js/src/jit/MoveResolver.h
@@ +95,5 @@
> +            return false;
> +        if (kind_ == FLOAT_REG) {
> +            return floatReg().aliases(other.floatReg());
> +        } else if (code_ != other.code_)
> +            return false;

nit: no else after return

@@ +207,5 @@
>      // Moves that are definitely unblocked (constants to registers). These are
>      // emitted last.
>      js::Vector<MoveOp, 16, SystemAllocPolicy> orderedMoves_;
>      bool hasCycles_;
> +    int numCycles_;

seems unused

::: js/src/jit/RegisterSets.h
@@ +101,5 @@
> +    }
> +    bool aliases(const AnyRegister &other) const {
> +        if (isFloat() && other.isFloat()) {
> +            return fpu().aliases(other.fpu());
> +        } else if(!isFloat() && !other.isFloat()) {

nit: else after return

::: js/src/jit/arm/Architecture-arm.cpp
@@ +293,5 @@
> +getSizeInBytes(const FloatRegisterSet &s)
> +{
> +    uint64_t bits = s.bits();
> +    uint32_t ret = mozilla::CountPopulation32(bits&0xffffffff) * sizeof(float);
> +    ret +=  mozilla::CountPopulation32(bits >> 32) * sizeof(double);

Nit: two spaces after +=

@@ +323,5 @@
> +    FloatRegisterSet ss = reduceSetForPush(s);
> +    uint64_t bits = ss.bits();
> +    uint32_t ret = mozilla::CountPopulation32(bits&0xffffffff) * sizeof(float);
> +    ret +=  mozilla::CountPopulation32(bits >> 32) * sizeof(double);
> +    return ret;

You could reuse getSizeInBytes here?

::: js/src/jit/arm/Architecture-arm.h
@@ +214,5 @@
> +        s27,
> +        s28,
> +        s29,
> +        s30,
> +        s31

Should this be ending by invalid_freg too?

@@ +255,3 @@
>  
>      // d15 is the ScratchFloatReg.
> +    // s31 and s32 alias d15.

There is no s32.

@@ +278,5 @@
> +          (1 << s26) |
> +          (1 << s27) |
> +          (1 << s28) |
> +          (1 << s29) |
> +          (1 << s30)));

Correct me if I'm wrong, but it seems that dX is aliased by s(2X) and s(2X+1), so d14 is aliased by s28 and s29, but s30 shouldn't be in this list, right?

@@ +288,2 @@
>  
>      // d15 is the ARM scratch float register.

Might be better to avoid "float" as a generic name grouping float32 and double, now. What do you think?

@@ +288,5 @@
>  
>      // d15 is the ARM scratch float register.
> +    static const uint64_t NonAllocatableMask = ((1ll << d15) << 32) |
> +                                                (1ll << s30) |
> +                                                (1ll << s31);

That seems to confirm my suspicion above re: d15 being aliased by s30 and s31.

@@ +351,5 @@
> +    bool isFloat() const { return (kind == Double) || (kind == Single); }
> +    bool isInt() const { return (kind == UInt) || (kind == Int); }
> +    bool isSInt() const { return kind == Int; }
> +    bool isUInt() const { return kind == UInt; }
> +    bool equiv(VFPRegister other) const { return other.kind == kind; }

The name kind of sounds too generic. How about {is || has}Same{Kind || Type}?

@@ +375,5 @@
> +
> +        VFPRegIndexSplit (uint32_t block_, uint32_t bit_)
> +          : block(block_), bit(bit_)
> +        {
> +            JS_ASSERT (block == block_);

nit: whitespace

@@ +397,5 @@
> +        return VFPRegister(code, RegType(kind));
> +    }
> +    bool volatile_() const {
> +        if (isDouble())
> +            return !!((1 << (code_ >> 1)) & FloatRegisters::VolatileMask);

By construction, shouldn't it just be ((1 << code_) << 32) & FloatRegisters::VolatileMask? Trying by hand, it seems to give the bad result for d8, if d8 == 9. I must be missing something here.

@@ +398,5 @@
> +    }
> +    bool volatile_() const {
> +        if (isDouble())
> +            return !!((1 << (code_ >> 1)) & FloatRegisters::VolatileMask);
> +        else

nit: no else after return

@@ +408,5 @@
> +        return FloatRegisters::GetSingleName(code_);
> +    }
> +    bool operator != (const VFPRegister &other) const {
> +        // if the kinds are different, the registers must be different
> +        // right?

One would hope that operator!= returns !operator==, which seems about the same here, modulo the assertions.

@@ +414,5 @@
> +    }
> +    bool aliases(const VFPRegister &other) {
> +        if (kind == other.kind) {
> +            return code_ == other.code_;
> +        } else {

nit: else after return

::: js/src/jit/arm/Assembler-arm.cpp
@@ +51,2 @@
>          if (floatRegIndex_ == NumFloatArgRegs) {
>              static const int align = sizeof(double) - 1;

sizeof(float)?

@@ +51,5 @@
>          if (floatRegIndex_ == NumFloatArgRegs) {
>              static const int align = sizeof(double) - 1;
>              stackOffset_ = (stackOffset_ + align) & ~align;
>              current_ = ABIArg(stackOffset_);
>              stackOffset_ += sizeof(uint64_t);

sizeof(uint32_t)?

@@ +585,5 @@
> +    if (getenv("INST_DUMP")) {
> +        char buf[4096];
> +        sprintf(buf, "gdb /proc/%d/exe %d -batch -ex 'set pagination off' -ex 'set arm force-mode arm' -ex 'x/%di %p' -ex 'set arm force-mode auto'", getpid(), getpid(), m_buffer.size() / 4, buffer);
> +        system(buf);
> +    }

wow.

@@ +1211,3 @@
>  {
>      JS_ASSERT(!_isInvalid);
> +    JS_ASSERT(which == 0);

Maybe add a comment to refer to the Architecture.h comment in which we say we don't support higher parts of double regs?

@@ +1371,5 @@
>  Assembler::as_alu(Register dest, Register src1, Operand2 op2,
>                    ALUOp op, SetCond_ sc, Condition c, Instruction *instdest)
>  {
> +    //if (dest == r6 && op2.isO2Reg() && op2.toOp2Reg().checkRM(lr) && op == op_mov)
> +    //        dbg_break();

wow, this kind of statements makes me think it must have been some hell of a debug.

::: js/src/jit/arm/Assembler-arm.h
@@ +1652,5 @@
> +        // vdtm can only transfer 16 registers at once.  If we need to transfer more,
> +        // then either hoops are necessary, or we need to be updating the register
> +        JS_ASSERT_IF(len > 16, dtmUpdate == WriteBack);
> +        
> +        int adjustLow = dtmLoadStore == IsStore ? 0 : 1;

Could you please add parenthesis? I am not even sure whether it's doing (dtmLoadStore == IsStore) ? 0 : 1 or dtmLoadStore == (IsStore ? 0 : 1). From context, it seems to be doing the former, but explicit is better than implicit.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +108,5 @@
>  MacroAssemblerARM::convertDoubleToInt32(const FloatRegister &src, const Register &dest,
>                                          Label *fail, bool negativeZeroCheck)
>  {
>      // convert the floating point value to an integer, if it did not fit,
>      //     then when we convert it *back* to  a float, it will have a

s/float/double

@@ +161,5 @@
>  
>  void
>  MacroAssemblerARM::convertFloat32ToDouble(const FloatRegister &src, const FloatRegister &dest) {
> +    JS_ASSERT(dest.isDouble());
> +    JS_ASSERT(src.isSingle());

These asserts are very handy.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +532,5 @@
> +            scratch = ScratchDoubleReg;
> +        else
> +            scratch = ScratchFloat32Reg;
> +#else
> +        scratch = ScratchFloat32Reg;

This will work as ScratchFloat32Reg == ScratchDoubleReg, but that seems better to have ScratchDoubleReg here.

@@ +771,5 @@
>      saveVolatile(dest);
> +#ifdef JS_CODEGEN_ARM
> +    if (ool->needFloat32Conversion()) {
> +        masm.convertFloat32ToDouble(src, ScratchDoubleReg);
> +        src = ScratchDoubleReg;

I think ool->src() could be clobbered by the call below, right? That sounds like the reason why we had to push the src value in the first place.

::: js/src/jit/x64/Architecture-x64.h
@@ +181,5 @@
> +    bool volatile_() const {
> +        return !!((1 << code()) & FloatRegisters::VolatileMask);
> +    }
> +    bool aliases(FloatRegister const &other) const;
> +};

Seems like this could be shared between x86 and x64 architectures, as it's the exact same code.  Maybe worth it to have an Architecture-x86-shared.h?

Comment 18

[haitao]

Comment on attachment 8392167 [details] [diff] [review]
fixFloat32-r0.patch

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

::: js/src/jit/LinearScan.cpp
@@ +1005,5 @@
>              AnyRegister reg = alloc->toRegister();
> +            for (int a = 0; a < reg.numAliased(); a++) {
> +                IonSpew(IonSpew_RegAlloc, "   Register %s not free", reg.aliased(a).name());
> +                freeUntilPos[reg.aliased(a).code()] = CodePosition::MIN;
> +            }

I would suggest we introduce a RegisterKind for each Interval (virtual register) for Float32 and Float64. The physical registers of the different kind could be aliased, but the virtual register could only be assigned to the required kind.
"
     for (IntervalIterator i(active.begin()); i != active.end(); i++) {
         LAllocation *alloc = i->getAllocation();
         if (alloc->isRegister() && alloc->toRegister().overlapsWith(kind)) {
             for (AnyRegisterIterator iter(reg.overlappingSet(kind)); iter.more(); iter++) {
                 AnyRegister overlapped = *iter;
"

what do you think?

@@ +1133,5 @@
> +                } else if (nextUsePos[reg.code()] != CodePosition::MIN) {
> +                    nextUsePos[reg.code()] = i->nextUsePosAfter(current->start());
> +                    IonSpew(IonSpew_RegAlloc, "   Register %s next used %u", reg.name(),
> +                            nextUsePos[reg.code()].pos());
> +                }

We could always spill a bigger virtual register (128-bit than 64-bit, 64-bit than 32-bit) when register kind are aliased. Do we want to spill smaller ones for the big? For example, when the current requires a Float64, do we want to select two/one float32 registers to spill? Sometimes we could only spill one as the other half of the float32 might be available (not in the active set).

Comment 19

[Marty Rosenberg [:mjrosenb]]

Attachment: fixFloat32-r1.patch

now works with backtracking, *mostly* works with the stupid allocator.  Next up fixing nits + removing debug code.