520718 - nanojit: i386 backend accesses the stack below SP

Status: RESOLVED DUPLICATE of bug 590553

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Running lirasm --random I see lots of these:

==28514== Invalid read of size 8
==28514==    at 0x4CD6EB3: ???
==28514==    by 0x805A541: main (lirasm.cpp:1553)
==28514==  Address 0xfeb767b8 is just below the stack ptr.  To suppress, use: --workaround-gcc296-bugs=yes

AIUI Valgrind complains about this because the x86 ABI doesn't guarantee that addresses below SP (in the sense "addresses lower than SP") are accessible.  Eg. the kernel could trash them at any time.

The code responsible is in asm_u2f():

  const int disp = -8;
  const Register base = SP;
  Register gr = findRegFor(ins->oprnd1(), GpRegs);
  NanoAssert(rr == FST0);
  FILDQ(disp, base);
  STi(base, disp+4, 0);   // high 32 bits = 0
  ST(base, disp, gr);     // low 32 bits = unsigned value

The generated code looks like this:

    u2f13 = u2f ld5
        mov -8(esp),ebx
        mov -4(esp),0
        fildq -8(esp)

AFAICT the eight bytes just below ESP are being used as a very short-term scratch space.  This also avoids using a register for the base pointer.  It seems unlikely to cause any problems in practice but is probably worth avoiding (by allocating memory on the stack with findMemFor()) just to keep things Valgrind-clean.

Comment 1

[Andreas Gal :gal]

My ESP-relative addressing patch fixes this as a side effect. I will try to get back to that this week.

Comment 2

[Julian Seward [:jseward]]

(In reply to comment #0)
>     u2f13 = u2f ld5
>         mov -8(esp),ebx
>         mov -4(esp),0
>         fildq -8(esp)
> 
> It seems unlikely to cause any problems in practice

Random unreproducible crashes/failures, especially in cases where 
many signals are delivered, eg, in SIGPROF-based profiling setups.

Comment 3

[Nicholas Nethercote [inactive]]

I still see this, with 'valgrind --smc-check=all lirasm --random 10000'.

Comment 4

[Julian Seward [:jseward]]

The least-worst safe replacement sequence I can think of is

  pushl  $0
  pushl  %ebx
  fildq  0(%esp)
  addl   $8,%esp

Additional comment re both sequences: this kind of thing looks
like it's asking for trouble w.r.t. store-forwarding stalls.
Indeed, the "Intel 64 and IA-32 Architectures Optimization 
Reference Manual" version dated Nov 2007, lists this as
Example 3-33 on page 3-54:

  mov  mem, eax
  mov  mem+4, ebx
  fld mem              ; load qword at address "MEM", stalls

Comment 5

[Andreas Gal :gal]

fld can take a memory address as argument, right? Why don't we use that form? Just put the constant into memory and load it. That should avoid the stall.

Comment 6

[Julian Seward [:jseward]]

(In reply to comment #5)
> Just put the constant into memory and load it. That should avoid the stall.

I don't understand.  The sequence appears to be intended to take the
value in %ebx, convert it unsignedly into a 80-bit float, and park it
in %st0.  I don't see how to do that without doing a store of %ebx, at
least, shortly before the fildq.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> Additional comment re both sequences: this kind of thing looks
> like it's asking for trouble w.r.t. store-forwarding stalls.

This code only occurs on non-SSE2 machines so avoiding stalls doesn't seem that important... worth doing if it's easy, but otherwise not.