542462 - JIT-ed code use non-scratch registers without saving and restoring them

Status: RESOLVED INVALID

(Core Graveyard :: Nanojit, defect)

Description

[Balazs Kelemen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
Build Identifier: hg-changeset: 1176:51a78a175b10

The latest nanojit generates fragments like this for me:
00b76d2e31   [prologue]
  00b76d2e31   push ebp
  00b76d2e32   mov ebp,esp
  00b76d2e34   [frag entry]
  00b76d2e34   sub esp,40
  ## compiling trunk 0x830bfe0
      jitFrame = iparam 0 ecx
  00b76d2e37   mov -12(ebp),ecx  <= spill jitFrame
  00b76d2e3a   mov edi,ecx
      callFrame = ld jitFrame[56]
  00b76d2e3c   mov esi,56(edi)
  00b76d2e3f   mov -8(ebp),esi  <= spill callFrame
      sti callFrame[-36] = imm(0)
  00b76d2e42   mov -36(esi),0
      ld1 = ld #0x830c428[232]
  00b76d2e49   mov   ebx,0(830c510)
  00b76d2e4f   mov -4(ebp),ebx  <= spill ld1

Actually, it has written ebx without storing it.
I do not know when the change happened that caused this, but one or two
months ago every fragment started with push instructions and ended with pops,
so my same use results in a fragment that started with:
  00b7659e18   push edi
  00b7659e19   push esi
  00b7659e1a   push ebx
  00b7659e1b   push ebp
  00b7659e1c   push ebp
and ended with:
00b7659fea   pop ebp
  00b7659feb   pop ebp
  00b7659fec   pop ebx
  00b7659fed   pop esi
  00b7659fee   pop edi

Reproducible: Always

Steps to Reproduce:
JIT-ting something and use verbose mode with assembly output.

Comment 1

[Edwin Smith]

callee-saved registers are handled by emitting LIR_param instructions for them, and saving the LIR* for those instructions in LirBuffer.savedRegs.  You can take a look at tamarin-redux or tracemonkey to see how this is done.

Comment 2

[Nicholas Nethercote [inactive]]

Balazs, thanks for the report.

First of all, if the callee-saved registers such as %ebx were being clobbered as you claim I would expect Firefox to crash extremely quickly.  And presumably it isn't crashing.  So something else must be going on.

The fragments generated by the current tip version of Tracemonkey all look like this:

  00f727f230   [prologue]
  00f727f230   push ebp
  00f727f231   mov ebp,esp
  00f727f233   [frag entry]
  00f727f233   sub esp,56
  ## compiling trunk 0x9fd6c2c /home/njn/moz/SunSpider/tests/sunspider-0.9.1/3d-
cube.js:318
      ebx = iparam 0 ebx
  00f727f236   mov -12(ebp),ebx  <= spill ebx
                            RR
      esi = iparam 1 esi
  00f727f239   mov -8(ebp),esi  <= spill esi
                            RR
      edi = iparam 2 edi
  00f727f23c   mov -4(ebp),edi  <= spill edi
                            RR

ie. the first thing they do is save the callee-save registers.  (I think that's what Edwin was talking about in comment 1.)  But the code that does this isn't in the 1.9.1 branch, which Firefox 3.5 is based on.

So those registers must be being saved some other way in 1.9.1.  Turns out it's in Assembler::genPrologue():  http://hg.mozilla.org/releases/mozilla-1.9.1/file/e7d399d3d636/js/src/nanojit/Nativei386.cpp#l111.

Now it seems that although code that saves those registers is generated, for some reason nothing gets printed for them in the verbose output.  PUSHr() is defined here: http://hg.mozilla.org/releases/mozilla-1.9.1/file/e7d399d3d636/js/src/nanojit/Nativei386.h#l508.  The printing ends up happening in Assembler::output() at http://hg.mozilla.org/releases/mozilla-1.9.1/file/e7d399d3d636/js/src/nanojit/Assembler.cpp#l1895.  I strongly suspect that the '_frago->core()->console << s << "\n";' case is being executed, because we're in the block's prologue, and somehow the output is being lost.

So I think you've found a bug in the verbose output printing.  But it's already been fixed on the 1.9.2 branch, partly because the param saving is part of the LIR, as the code example above shows, and partly because Assembler::output() et al have been cleaned up (bug 512181) and the "_frago->core()->console' code is gone.

So I'm downgrading the severity of this from 'critical' to 'minor', and nominating for a WONTFIX.  Edwin, Gal, you ok with that?

Comment 3

[Andreas Gal :gal]

jitFrame and callFrame don't look like code we generate. As Ed pointed out you are missing the param placeholders to save non-volatile registers. This is what the code looks like in TM:

    for (int i = 0; i < NumSavedRegs; ++i)
        lir->insParam(i, 1);
#ifdef DEBUG
    for (int i = 0; i < NumSavedRegs; ++i)
        addName(lirbuf->savedRegs[i], regNames[Assembler::savedRegs[i]]);
#endif

Feel free to re-open if we misunderstood you but I am pretty sure this is not a bug in our code.

Comment 4

[Balazs Kelemen]

I think have understood the concept - code generation needs to explicitly save non-volatile registers. Thank you for your help.