596433 - Method JIT inline assembly has illegal instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Stover (:stechz)]

It is not thumb encoded and uses bl instruction to jump to thumb code instead of blx instruction. This crashes with invalid operation.

We tried a fix that uses "blx" but for some reason the generated code was still using "bl" on my build, even clobbered (maybe this is my own build machine weirdness?). We managed to finally fix it by encoding the inline assembly with thumb like the rest of the code.

Patch is coming.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: WIP fix

Comment 2

[Benjamin Stover (:stechz)]

This still needs to check if we are compiling for thumb. I tried MOZ_THUMB2 but it didn't work, so I need to change the Makefile probably.

Comment 3

[Benjamin Stover (:stechz)]

By the way, I copied the headers that GCC was generating for its functions (align 2, thumb, thumb_func). The function footers also define the size of the function, but I left that alone.

I should probably mention I don't really know what I'm doing. :)

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Only use thumb encoding for thumb builds

Comment 5

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

Comment on attachment 475622 [details] [diff] [review]
Only use thumb encoding for thumb builds

r=me, please make sure to land on tracemonkey too. Jacob, could you still look this over?

Comment 6

[Jacob Bramley [:jbramley]]

Comment on attachment 475622 [details] [diff] [review]
Only use thumb encoding for thumb builds

It looks functionally correct, but I've made a couple of comments:

>+#ifdef MOZ_THUMB2
>+#define FUNCTION_HEADER_EXTRA \
>+  ".align 2\n" \
>+  ".thumb\n" \
>+  ".thumb_func\n"
>+#else
>+#define FUNCTION_HEADER_EXTRA
>+#endif

".align 3" (or ".balign 8") might be better here for hot functions,
regardless of whether the target is ARM or Thumb.

> "   mov     r0, sp"                             "\n"
>-"   bl  " SYMBOL_STRING_VMFRAME(SetVMFrameRegs)   "\n"
>+"   blx  " SYMBOL_STRING_VMFRAME(SetVMFrameRegs)   "\n"

A "bl" should have worked here. A "blx <label>" instruction _always_
swaps between Thumb and ARM state. A "bl <label>" instruction never
does. However, the assembler is supposed to select between the two, so
the distinction is only really important to the JIT back-end.

(The change is harmless, however. When I last looked, "blx" also gets
translated to "bl" when required.)

Comment 7

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

Aha - Thanks Jacob. That explains why Ben and I were seeing our "blx" display as "bl" in the debugger.

Comment 8

[Benjamin Stover (:stechz)]

> ".align 3" (or ".balign 8") might be better here for hot functions,
> regardless of whether the target is ARM or Thumb.

GCC is doing align 2, so if align 3 is better perhaps there's a compiler option we could use? Why is align 3 better?

Good to know the assembler changes the instruction based on how the instructions are encoded. I thought I was going crazy when I was inspecting the disassembled code in gdb.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ec64525c175d

Comment 10

[walter sheets]

Hi guys.  I'm trying to bisect a bug that appeared in the linux 32-bit nightly builds around Sept 10 and still hasn't been fixed in trunk.

My problem is that this illegal instruction crash is preventing me from doing my debugging.

Of course I want to apply your patch from comment 9, so I went to apply it, and patch claims that the patch has already been applied.  Huh?

Reading thru configure.in and MethodJIT.cpp, patch appears to be right - the changes from 54171 are already there, even though I've checked out and built 54121, and I'm still getting the illegal instruction crash.

Maybe (probably) I don't understand how hg works.  Why should my checked-out 54121 already have your changes from 54171?  I don't think I'm mistaken about which version I'm working on at the moment:

$hg sum
parent: 54121:cdb90b48f19f 
 Removal of extra ws from notification code.

Any clues would be most welcome.  Thanks!

Comment 11

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

(In reply to comment #10)

This bug is for ARM CPUs - if you're seeing this on x86, please file a new bug. It could be related to bug 596457.