567762 - JM: JSOP_IFEQ branches are not properly patched (for forward branches).

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

I get stuck in an infinite loop on trace-test/tests/arguments/bug503772.js. It looks like jsl_Arguments is clobbering its return path and getting stuck, or something like that. I'll investigate in more detail tomorrow.

Comment 1

[Jacob Bramley [:jbramley]]

Attachment: Change the default branch target to work around an ARM Linux kernel bug.

Ok, this isn't the solution, but it's a step in the right direction. It appears that jsl_Arguments isn't the cause of the problem after all.

The jsl_Arguments is called from some (method-JIT-compiled) code that looks (something) like this:

ldr ip, =jsl_Arguments
blx JaegerStubVeneer
ldr     r10, [r11, #84]  ; Irrelevant
str     r10, [r11, #76]  ; Irrelevant
ldr     pc, [pc, #248]   ; Broken!

The call itself works fine. However, the literal-pool load into the PC is loading 0xffffffff. This is the value used temporarily before the branch target is known, and is what causes the crash.

Yes, GDB's backtrace makes it look like jsl_Arguments is the caller, but I think it's just confused.

----

That doesn't explain the infinite loop; you might expect to hit an abort at that address, but some digging around turned up a kernel bug: User-mode accesses to 0xc0000000 and above (i.e. kernel space) don't trigger the segmentation fault handler. Instead, the abort handler bails back out to user-mode, where the same (aborting) instruction is executed again.

There's already kernel patch going upstream to fix this, but for now I've written a patch for JM that simply uses a different default branch target so that the segmentation fault handler is invoked. This doesn't fix the bug, but it means that we can at least run trace-tests.

----

I've not yet worked out where the offending branch is emitted from, but there are only a few places where such code is generated.

Comment 2

[Jacob Bramley [:jbramley]]

The problem is in jsop_cond_jump, when a JSOP_IFEQ is handled. On both x86-64 and ARM, the default branch (for a constant false condition) is not redirected, and the contents of the block is executed.

To prove it, try using something like this:

function f(a) {
  // Create arguments on trace
  var z = arguments;
  // Make f need a call object
  if (false) {
    print("Shouldn't be here!");
    var p = function() { ++a; }
  }
}

Then, on x86-64:

/work/moz/jm.d$ ./js -mj /work/moz/jm/js/src/trace-test/tests/arguments/bug503772.js
Shouldn't be here!
Shouldn't be here!
Shouldn't be here!
Shouldn't be here!
Shouldn't be here!

The reason appears to be that the default branch target for x86-64 is the next instruction, so the default branch is a NOP, but on ARM it is an abort address. Well, it is after my previous patch, at least.

----

I have written a patch, but just discovered that it doesn't work, so I haven't attached it. (I naively using masm.label() etc and just ended up making the ARM version a NOP too.)

I'll write a proper fix for Monday.

Comment 3

[Jacob Bramley [:jbramley]]

Attachment: Add a test to catch failed branch patching on all platforms.

There are two significant things that I'm not sure about with this patch:

  * Should tests specifically for Jaeger Monkey be in trace-tests? (They don't do any harm for the other monkeys, so it seems as good a place as any.)

  * The test relies on the engine emitting JSOP_IFEQ with a constant argument. Sometimes it doesn't, but it seems that declaring a local variable forces it not to optimize the branch away. That might not last forever, so this isn't terribly robust, but in the absence of something like lirasm for JSOPs, I can't think of a decent alternative.

Also note that I can't test the case there JSOP_IFNE is emitted with a constant argument and a forward branch, because this code never seems to be emitted by the engine. (If a backward branch is used, as with "while" loops, the branch target is known and doesn't need to be patched.)

Comment 4

[Jacob Bramley [:jbramley]]

Attachment: Fix JSOP_IFEQ and JSOP_IFNE.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> Created an attachment (id=451247) [details]
> Add a test to catch failed branch patching on all platforms.
> 
> There are two significant things that I'm not sure about with this patch:
> 
>   * Should tests specifically for Jaeger Monkey be in trace-tests? (They don't
> do any harm for the other monkeys, so it seems as good a place as any.)

Existing basic JM tests are in trace-tests/tests/jaeger, so please put 'em there.

>   * The test relies on the engine emitting JSOP_IFEQ with a constant argument.
> Sometimes it doesn't, but it seems that declaring a local variable forces it
> not to optimize the branch away. That might not last forever, so this isn't
> terribly robust, but in the absence of something like lirasm for JSOPs, I can't
> think of a decent alternative.

It seems OK to me. One way to try make it a bit more robust would be to create some more test cases that are minor variations on the basic one.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 451248 [details] [diff] [review]
Fix JSOP_IFEQ and JSOP_IFNE.

Thanks a ton for tracking this down, Jacob.

Comment 7

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/

As recommended, I added several more test cases, for good measure. (Only the first actually fails before the fix is applied, but they all exercise related code.)

Comment 8

[Jacob Bramley [:jbramley]]

Of course, it helps if I post the link to the revision, rather than to the repository: http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/rev/bca1510d8912

Comment 9

[Jacob Bramley [:jbramley]]

This is no longer relevant, but it was technically fixed in the old Jaeger Monkey tree so I've marked it "FIXED".