Bug 60941 - [4.7/4.8/4.9/4.10 regression] miscompilation of firefox javascript interpreter
Summary: [4.7/4.8/4.9/4.10 regression] miscompilation of firefox javascript interpreter
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.8.3
: P2 normal
Target Milestone: 4.7.4
Assignee: Eric Botcazou
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2014-04-23 14:26 UTC by Martin Husemann
Modified: 2014-04-25 10:50 UTC (History)
0 users

See Also:
Host:
Target: sparc64-*-*
Build:
Known to work: 4.5.4
Known to fail: 4.10.0, 4.7.4, 4.8.3, 4.9.0
Last reconfirmed: 2014-04-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Husemann 2014-04-23 14:26:22 UTC
System: NetBSD/sparc64; NetBSD in-tree version of gcc: 
> gcc -v
Using built-in specs.
COLLECT_GCC=gcc
Target: sparc64--netbsd
Configured with: /usr/src6/tools/gcc/../../external/gpl3/gcc/dist/configure --target=sparc64--netbsd --enable-long-long --enable-threads --with-bugurl=http://www.NetBSD.org/Misc/send-pr.html --with-pkgversion='NetBSD nb1 20120916' --with-system-zlib --enable-__cxa_atexit --with-mpc-lib=/var/obj/mknative/sparc64/usr/src6/external/lgpl3/mpc/lib/libmpc --with-mpfr-lib=/var/obj/mknative/sparc64/usr/src6/external/lgpl3/mpfr/lib/libmpfr --with-gmp-lib=/var/obj/mknative/sparc64/usr/src6/external/lgpl3/gmp/lib/libgmp --with-mpc-include=/usr/src6/external/lgpl3/mpc/dist/src --with-mpfr-include=/usr/src6/external/lgpl3/mpfr/dist/src --with-gmp-include=/usr/src6/external/lgpl3/gmp/lib/libgmp/arch/sparc64 --enable-tls --disable-multilib --disable-symvers --disable-libstdcxx-pch --build=x86_64-unknown-netbsd6.0. --host=sparc64--netbsd --with-sysroot=/var/obj/mknative/sparc64/usr/src6/destdir.sparc64
Thread model: posix
gcc version 4.8.3 (NetBSD nb2 20140304) 


When building a firefox 28 with gcc 4.8 and at least -O2 the javascript interpreter misbehaves and causes firefox to crash. An example URL that triggers this is: https://github.com/mconf/ffmpeg/blob/master/doc/optimization.txt

The bug itself is a bit subtle - hope I get it clear:

The firefox javascript interpreter has a very special way to represent all kinds of values in a single 64 bit value (which makes it impossible to represent certain double NaN values and some pointer values - however, this is all "properly" taken care of). The definition of the Value type is:

typedef union jsval_layout
{
    uint64_t asBits;
    struct {
        JSValueTag         tag : 17;
        uint64_t           payload47 : 47;
    } debugView;
    struct {
        uint32_t           padding;
        union {
            int32_t        i32;
            uint32_t       u32;
            JSWhyMagic     why;
        } payload;
    } s;
    double asDouble;
    void *asPtr;
    size_t asWord;
    uintptr_t asUIntPtr;
} JSVAL_ALIGNMENT jsval_layout;

There is a helper macro that "constructs" new Values from 32bit ints:

static inline JS_VALUE_CONSTEXPR jsval_layout
INT32_TO_JSVAL_IMPL(int32_t i32)
{
    JS_RETURN_LAYOUT_FROM_BITS(((uint64_t)(uint32_t)i32) | JSVAL_SHIFTED_TAG_INT32);
}

and JS_RETURN_LAYOUT_FROM_BITS is (depending on ifdefs) in this case defined as:

#  define JS_RETURN_LAYOUT_FROM_BITS(BITS) \
    return (jsval_layout) { .asBits = (BITS) }

Now ignoring a few details, the values with tag 0xfff88 in the high order bits are int32 values, and the low order bits just contain the value.

With some printf instrumentation, right before the crash happens, the interpreter stack looks like:

end of JSOP_INT8 top of stack: fff8800000000019
end of JSOP_URSH top of stack: fff8800000000028
end of JSOP_BITXOR top of stack: fff88000014e18ab
end of JSOP_GETLOCAL top of stack: fff88000510e527f
end of JSOP_INT8 top of stack: fff880000000001a

now you see 0x510e527f and 0x1a as the topmost values, both int32. Next a logical shift left is done with these two.

The assembler code generated loads the args into %g2 and %g3 and does a sll:

(gdb) p/x $g3
$1 = 0x510e527f
(gdb) p/x $g2
$2 = 0x1a
 
   0xfffffffffe4d97e0     sll  %g3, %g2, %g2

now sll leaves the high 32 bits in undefined state, so after the sll we get 0x1443949fc000000 instead of 0xfc000000 in %g2. Unfortunately the code now ors the correct tag bits into this and stores it directly as a 64bit value:

=> 0xfffffffffe4d97e4     ldx  [ %l7 + %o0 ], %o0 
   0xfffffffffe4d97e8     add  %g1, -8, %g3                                              
   0xfffffffffe4d97ec     or  %g2, %g4, g2                                              
   0xfffffffffe4d97f0     stx  %g3, [ %fp + 0x727 ] 

(the 0xfff88 tag bits had been precomputed in %g4 earlier). This leads to broken jsvalue_layout data, as the result now looks like a pointer to a javascript object to the interpreter, and when trying to call methods of that, the crash happens.


The code in the interpreter loop (slightly unriddled a few macros) looks correct to me:

CASE(JSOP_LSH)
    {
        int32_t i, j;
        if (!ToInt32(cx, REGS.stackHandleAt(-2), &i))
            goto error;
        if (!ToInt32(cx, REGS.stackHandleAt(-1), &j))
            goto error;
        i = i << (j & 31);
        REGS.sp--;
        REGS.sp[-1].setInt32(i);
    }
END_CASE(JSOP_LSH)  

The setInt32() function is the accessor mentioned earlier:

    void setInt32(int32_t i) {
        data = INT32_TO_JSVAL_IMPL(i);
    }

and "data" is a union jsval_layout. INT32_TO_JSVAL_IMPL is shown above.

The files this can be found in the firefox source are:

mozilla-release/js/public/Value.h (macros, inlines, unions jsval_layout)
mozilla-release/js/src/vm/Interpreter.cpp (main interpreter loop).

The critical thing should be the ((uint64_t)(uint32_t)i32 part when i32 is the result of a "sll" - this should require an explicit extension to 64bit, shouldn't it?
Comment 1 Martin Husemann 2014-04-23 14:39:33 UTC
Here is a small test program:

---8<---
#include <stdio.h>
#include <stdlib.h>


int main(int argc, char **argv)
{
        unsigned long v[2], *p;
        int a, b;

        for (int i = 0; i < 2 && i < argc; i++) {
                v[i] = atol(argv[i+1]) | 0xfff8800000000000;
                printf("i = %d, got: %lx\n", i, v[i]);
        }

        p = &v[1];
        a = *p-- & 0x0ffffffff;
        b = *p & 0x0ffffffff;
        printf("a = %08x b= %08x\n", a, b);
        b = a << (b & 31);
        *p = ((uint64_t)(uint32_t)b) | 0xfff8800000000000;
        printf("res: %08lx\n", *p);

        return 0;
}
--->8---

compile with  c++ -Wall -O2 test.c and run as:

> ./a.out 26 1359893119
i = 0, got: fff880000000001a
i = 1, got: fff88000510e527f
a = 510e527f b= 0000001a
res: fffcb949fc000000

I would have expected res: 0xfff88000fc000000 (and on amd64 I get exactly that)
Comment 2 Eric Botcazou 2014-04-23 22:39:32 UTC
I can reproduce the bug on Solaris with the following testcase:

extern void abort ();

static void __attribute__((noinline))
set (unsigned long *l)
{
  *l = 31;
}

int main (void)
{
  unsigned long l;
  int i;

  set (&l);
  i = (int) l;
  l = (unsigned long)(unsigned int)(2 << i);
  if (l != 0)
    abort ();
  return 0;
}
Comment 3 Eric Botcazou 2014-04-23 22:40:38 UTC
Investigating.
Comment 4 Eric Botcazou 2014-04-25 10:39:46 UTC
Author: ebotcazou
Date: Fri Apr 25 10:39:14 2014
New Revision: 209790

URL: http://gcc.gnu.org/viewcvs?rev=209790&root=gcc&view=rev
Log:
	PR target/60941
	* config/sparc/sparc.md (ashlsi3_extend): Delete.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sparc/sparc.md
    trunk/gcc/testsuite/ChangeLog
Comment 5 Eric Botcazou 2014-04-25 10:44:00 UTC
Author: ebotcazou
Date: Fri Apr 25 10:43:28 2014
New Revision: 209791

URL: http://gcc.gnu.org/viewcvs?rev=209791&root=gcc&view=rev
Log:
	PR target/60941
	* config/sparc/sparc.md (ashlsi3_extend): Delete.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
      - copied unchanged from r209790, trunk/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/sparc/sparc.md
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog
Comment 6 Eric Botcazou 2014-04-25 10:46:49 UTC
Author: ebotcazou
Date: Fri Apr 25 10:46:18 2014
New Revision: 209792

URL: http://gcc.gnu.org/viewcvs?rev=209792&root=gcc&view=rev
Log:
	PR target/60941
	* config/sparc/sparc.md (ashlsi3_extend): Delete.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
      - copied unchanged from r209791, trunk/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/sparc/sparc.md
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
Comment 7 Eric Botcazou 2014-04-25 10:48:16 UTC
Author: ebotcazou
Date: Fri Apr 25 10:47:44 2014
New Revision: 209793

URL: http://gcc.gnu.org/viewcvs?rev=209793&root=gcc&view=rev
Log:
	PR target/60941
	* config/sparc/sparc.md (ashlsi3_extend): Delete.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
      - copied unchanged from r209791, trunk/gcc/testsuite/gcc.c-torture/execute/20140425-1.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/sparc/sparc.md
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
Comment 8 Eric Botcazou 2014-04-25 10:50:19 UTC
Thanks for reporting the problem and distilling a reduced testcase.