569691 - mops.abc_ test fails linux-arm with "Illegal Instruction"

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Chris Peyer]

The mops.abc test (test/acceptance/mops/mops.abc_) fails on linux-arm with "Illegal Instruction" and an exitcode of 132.

I'm pretty sure that this is the changeset that introduced the issue (isolated the change to between r4617-21, other changes don't seem relevant):

http://hg.mozilla.org/tamarin-redux/rev/4618 : LHansen : Fix 557286 - Clean up mess around floating point layouts

Comment 1

[Chris Peyer]

testconfig.txt updated to skip mops test until bug is resolved: http://hg.mozilla.org/tamarin-redux/rev/baf59c1c0d5e

Comment 2

[Lars T Hansen]

Could be.  What's the hardware - beagleboard?

Comment 3

[Brent Baker]

(In reply to comment #2)
> Could be.  What's the hardware - beagleboard?

Yes this failure is happening on a beagleboard. I can take a couple of minutes and bring a freescale board online and double check the failure on that hardware.

Comment 4

[Lars T Hansen]

If you have the time.

I have a beagleboard here, so at least I can debug when I get around to investigating.

Comment 5

[Brent Baker]

Same error happens on the freescale board:

Program received signal SIGILL, Illegal instruction.
avmplus::mop_sf32 (addr=0x403bf011, value=13.34)
    at /home/build/buildbot/tamarin-redux/linux/repo/core/instr.cpp:614
614	}

Comment 6

[Rick Reitmaier]

Do we know what floating-point flags were passed to the compiler and what is the offensive instruction ?   At a gdb prompt 'disass avmplus::mop_sf32' might give us a clue.

Comment 7

[Brent Baker]

Built using x-platform configure script with these switches " --enable-arm-neon --target=arm-linux" which defines the following:

 -mfloat-abi=softfp -mfpu=neon -march=armv7-a

(gdb) disass avmplus::mop_sf32 
Dump of assembler code for function _ZN7avmplus8mop_sf32EPvd:
0x00112cd0 <_ZN7avmplus8mop_sf32EPvd+0>:	vmov	d16, r2, r3
0x00112cd4 <_ZN7avmplus8mop_sf32EPvd+4>:	vcvt.f32.f64	s15, d16
0x00112cd8 <_ZN7avmplus8mop_sf32EPvd+8>:	vstr	s15, [r0]
0x00112cdc <_ZN7avmplus8mop_sf32EPvd+12>:	bx	lr
End of assembler dump.

Comment 8

[Lars T Hansen]

Hm, that changeset did not touch mop_sf32, it only affected 64-bit floating point values.

Comment 9

[Lars T Hansen]

I take that back, it added this optimization in a couple of places:

#if defined(VMCFG_UNALIGNED_INT_ACCESS) && defined(AVMPLUS_LITTLE_ENDIAN)
    *(uint32_t*)u = b;
#else

That's an interesting problem.  Clearly u is a uint32_t pointer, and 'b' is a member of a union whose type is uint32_t, but the other member of the union is a float, into which we've just stored a float value.  What's happening here is that the compiler is optimizing away the change in type that was implied by that trick, and storing directly from the floating point register using a floating point store.  Hence it's not correct (here) to be checking VMCFG_UNALIGNED_INT_ACCESS.

This could be considered a compiler bug, of course, but it'll be an uphill battle to argue that.

It seems to me that either (a) we must be checking both VMCFG_UNALIGNED_INT_ACCESS and VMCFG_UNALIGNED_FP_ACCESS in these situations or (b) we must do away with the distinction and have a single define for both of those that is true only if both cases are true or (c) we must back out this optimization.

Comment 10

[Lars T Hansen]

I can't repro the problem here.

I'm building on the beagleboard itself using gcc 4.3.3-5ubuntu4 (yay 50 minute builds).  I needed to fix a bunch of casts in the shell directory so I suspect you may have been building with an older version, or these would have been fixed already.

I need details on compiler version, build environment, and switches to configure.py - notably whether it was release or debug, debugger or not.

Comment 11

[Lars T Hansen]

Attachment: Candidate patch

This is the patch I would be testing, if I could repro the problem.  Might as well review it preemptively.

Comment 12

[Lars T Hansen]

Ah, some info above about config switches -- will retry.

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Isn't the #if that you're modifying in the context of an #else of an #if that looks like:
  #if defined(VMCFG_UNALIGNED_FP_ACCESS) && defined(AVMPLUS_LITTLE_ENDIAN)
and therefore your modification is making the code in question generally unreachable?

Comment 14

[Lars T Hansen]

Managed to repro after building with correct switches.  Verified that the offered patch fixes the problem in that setting.  Issues around whether the VMCFG_UNALIGNED_*_ACCESS remain but can be the subject of a different bug.

Comment 15

[Lars T Hansen]

(In reply to comment #13)
> Isn't the #if that you're modifying in the context of an #else of an #if that
> looks like:
>   #if defined(VMCFG_UNALIGNED_FP_ACCESS) && defined(AVMPLUS_LITTLE_ENDIAN)
> and therefore your modification is making the code in question generally
> unreachable?

You're right.  The entire case can just disappear, and I should just leave a comment stating why it's not there.

Comment 16

[Lars T Hansen]

Attachment: Patch

Comment 17

[Steven Johnson]

IIRC, ARMv7 chips allow unaligned int load/store, but not unaligned VFP load/store -- this bit us once before, and was the cause of splitting the FP and INT access macros in twain (I don't have the bug handy). So really this is an aliasing bug, why not change the ifdef'ed code to

    *(float*)u = a;	

?

Comment 18

[Lars T Hansen]

(In reply to comment #17)
> IIRC, ARMv7 chips allow unaligned int load/store, but not unaligned VFP
> load/store -- this bit us once before, and was the cause of splitting the FP
> and INT access macros in twain (I don't have the bug handy). So really this is
> an aliasing bug, why not change the ifdef'ed code to
> 
>     *(float*)u = a;    
> 
> ?

I think that's what the initial case in the method does (not shown in the patch).

Comment 19

[Lars T Hansen]

tamarin-redux changeset:   4766:813358258f11.  Also changed the mop_lf32 case, which did not fail in testing but could easily have the same problem.  Did not change the sf64/lf64 cases even though on some platforms, with enough optimization, a similar problem could occur.  (If it does occur then the problem will be caught in testing anyway.)