1138141 - Assertion failure: padding % sizeof(uintptr_t) == 0, at jit/IonCaches.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

__defineGetter__("x", function() {})
for (var j = 0; j < 2; j++) {
    try {
        Math.fround(Math.fround())(x)
    } catch (e) {}
}

asserts js debug shell on m-c changeset b94bcbc389e8 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: padding % sizeof(uintptr_t) == 0, at jit/IonCaches.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r b94bcbc389e8

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150226030722" and the hash "38382572cf6f".
The "bad" changeset has the timestamp "20150226031912" and the hash "71a08ff0d27c".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=38382572cf6f&tochange=71a08ff0d27c

Nicolas, is bug 1112164 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x115768, 0x00000001005585a5 js-dbg-64-dm-nsprBuild-darwin-b94bcbc389e8`EmitGetterCall(cx=<unavailable>, masm=<unavailable>, attacher=<unavailable>, obj=<unavailable>, holder=<unavailable>, shape=<unavailable>, liveRegs=<unavailable>, object=<unavailable>, output=<unavailable>, returnAddr=<unavailable>) + 3381 at IonCaches.cpp:1066, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001005585a5 js-dbg-64-dm-nsprBuild-darwin-b94bcbc389e8`EmitGetterCall(cx=<unavailable>, masm=<unavailable>, attacher=<unavailable>, obj=<unavailable>, holder=<unavailable>, shape=<unavailable>, liveRegs=<unavailable>, object=<unavailable>, output=<unavailable>, returnAddr=<unavailable>) + 3381 at IonCaches.cpp:1066
    frame #1: 0x00000001005542ed js-dbg-64-dm-nsprBuild-darwin-b94bcbc389e8`GenerateCallGetter(failures=0x00007fff5fbfdd0c, cx=<unavailable>, ion=<unavailable>, masm=<unavailable>, attacher=<unavailable>, obj=<unavailable>, name=<unavailable>, holder=<unavailable>, shape=<unavailable>, liveRegs=<unavailable>, object=<unavailable>, output=<unavailable>, returnAddr=<unavailable>) + 541 at IonCaches.cpp:1144
    frame #2: 0x0000000100567fc0 js-dbg-64-dm-nsprBuild-darwin-b94bcbc389e8`js::jit::NameIC::attachCallGetter(this=0x0000000102850350, cx=0x0000000101c158f0, ion=0x0000000102850200, returnAddr=<unavailable>, outerScript=<unavailable>, scopeChain=<unavailable>, obj=<unavailable>, holder=<unavailable>, shape=<unavailable>) + 400 at IonCaches.cpp:4363
    frame #3: 0x000000010056867e js-dbg-64-dm-nsprBuild-darwin-b94bcbc389e8`js::jit::NameIC::update(cx=0x0000000101c158f0, cacheIndex=<unavailable>, outerScript=<unavailable>, scopeChain=<unavailable>, vp=<unavailable>) + 1294 at IonCaches.cpp:4428
    frame #4: 0x00000001048eea97
(lldb)

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 till at least Mar 2 from comment #0)
> Nicolas, is bug 1112164 a likely regressor?

Yes, it is likely that we expected PushRegsInMask to only push double-size words on the stack, and with this set of patches PushRegsInMask is now capable of pushing float register as 32 bits values instead of 64 bits.

I will look at it this week.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: x64: Add some padding after pushing an odd number of float32 registers.

In practice the assertion which is in the IonCache is not needed as the code
is capable of reserving the padding even if it is not a multiple of
uintptr_t.  On the other hand, keeping the Push statements aligned on the
word size of the architecture is still good, especially since pushRegsInMask
is used before any out-of-line call.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8572607 [details] [diff] [review]
x64: Add some padding after pushing an odd number of float32 registers.

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

Alright.

::: js/src/jit/shared/MacroAssembler-x86-shared.cpp
@@ +56,5 @@
>          storeUnalignedInt32x4(*iter, Address(StackPointer, diffF));
>      }
>      MOZ_ASSERT(numSimd == 0);
> +    // x64 padding to keep the stack aligned on uintptr_t
> +    diffF -= diffF % sizeof(uintptr_t);

I guess this will be useful for arm64 as well.

Can you mention in the comment that this shall be kept in sync with GetPushBytesInSize, please?

@@ +103,5 @@
>      }
>      freeStack(reservedF);
>      MOZ_ASSERT(numDouble == 0);
> +    // x64 padding to keep the stack aligned on uintptr_t
> +    diffF -= diffF % sizeof(uintptr_t);

ditto

::: js/src/jit/x64/Assembler-x64.cpp
@@ +333,5 @@
>      SetType set64b = doubleSet & ~set128b;
>      SetType set32b = singleSet & ~set64b  & ~set128b;
>  
>      static_assert(Codes::AllPhysMask <= 0xffff, "We can safely use CountPopulation32");
> +    uint32_t nb32b = mozilla::CountPopulation32(set32b);

nit: this name doesn't ring a bell, even if I guess that it is an acronym for "number 32 bits". How about naming countFloat32, to keep it simple?

@@ +338,3 @@
>      return mozilla::CountPopulation32(set128b) * (4 * sizeof(int32_t))
>          + mozilla::CountPopulation32(set64b) * sizeof(double)
> +        + (nb32b + (/* padding = */ nb32b & 1)) * sizeof(float);

I'd rather do the padding above the return statement with a proper comment, and use the resulting padded countFloat32 here:

  // Ensure that we keep 8 bytes stack alignment for Ion caches. Keep this in sync with PushRegsInMask/PopRegsInMaskIgnore
  countFloat32 += countFloat32 & 1;
  return ...

Comment 5

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1826e5fca6bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/649267c62ddd

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/649267c62ddd