536153 - NativeARM.cpp: Incorrect use ALUr_shi to generate compare insns

Status: RESOLVED WONTFIX

(Core Graveyard :: Nanojit, defect)

Comment 1

[Julian Seward [:jseward]]

Ach, the perils of pressing Enter at the wrong time.  Details are:

NJ emits the following for signed-multiply and overflow check on ARM:

      mul2 = mul ld1, sizeof(JSTraceType)
  0004d73dc8   smull r5, ip, r4, r10
  0004d73dcc   cmp  ip, r5, asr #31

The compare looks legit, but it is assembled in such a way that a
should-be-zero field in the instruction (bits 15:12) is not zero.
This appears to be accepted by Cortex-A8 (although perhaps it is
interpreted as something else?).  However, the instruction decoder for
Valgrind on ARM checks said field and fails to recognise the
instruction.  At a minimum we can say that NJ is not ARM ARM compliant
here.

binutils assembles "cmp ip, r5, asr #31" as E15C0FC5,
                          but Nanojit emits E15CCFC5.

Comment 2

[Julian Seward [:jseward]]

Attachment: fixes the bogus CMPs and one related bogus MOV

Comment 3

[Jacob Bramley [:jbramley]]

Comment on attachment 418643 [details] [diff] [review]
fixes the bogus CMPs and one related bogus MOV

Yep looks good. I like the assertions!

>+                ALUr_shi(AL, cmp, 1, 0/*SBZ*/, IP, rr, ASR_imm, 31);

Only one comment: Is it worth having "SBZ" constant somewhere so we can tidy up the mid-line comments in the ALUr_shi invocations? Unfortunately, it clashes with r0, otherwise I'd recommend adding a "null register" constant to the Register enum. It's probably something for another patch.

Comment 4

[Nicholas Nethercote [inactive]]

I pulled SBZ out into a local const variable and landed:

http://hg.mozilla.org/projects/nanojit-central/rev/f4ece4c13545

Comment 5

[Nicholas Nethercote [inactive]]

http:///hg.mozilla.org/tracemonkey/rev/be0d7c213c90

Comment 6

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/3683

Comment 7

[Nicholas Nethercote [inactive]]

Nanojit has been dead for several years. Its Bugzilla component has been moved to the graveyard (bug 984276).

I checked all the open bugs. They're all uninteresting, so I'm WONTFIXing them all. Apologies for the bugspam.