rustc 1.14.0 beta producing neon instructions on armv7

[rillian]

We have some SIGILL crashes from Firefox on Tegra 2 devices, apparently in Rust code. See https://bugzilla.mozilla.org/show_bug.cgi?id=1323773 or https://crash-stats.mozilla.com/report/index/c0d43287-b39c-422c-8609-d63c52161215 for a specific example. These are ARM Cortex-A9 SoC without the neon extension.

I didn't find the function in question in our official apk, but when I build the mp4parse_capi crate with rust 1.14.0-beta.3, I see vld and vst instructions in the rlib disassembly for the mp4parse_new() function.

I believe from recent commits that armv7 it intended to support non-neon devices like the Tegra 2 with this target triple. Perhaps #36933 was insufficient?

[rillian]

The stack trace suggests this might be an inlined Box::new(). I'm trying to produce a smaller test-case.

cc @alexcrichton

[rillian]

Here's a relatively small test-case:

//! Reduced neon test case from the mp4parse_capi crate.

pub struct Parser {
    state: State,
}

#[derive(Clone)]
pub struct State {
    // This struct must have at least two members to trigger
    // NEON code generation.
    pub u: u32,
    pub v: u32,
}

pub unsafe fn parser_new(state: *const State) -> *mut Parser {
    let parser = Box::new(Parser {
        state: (*state).clone(),
    });
    Box::into_raw(parser)
}

All of rustc 1.13.0-stable, 1.14.0-beta.3, and 1.15.0-nightly generate a vldr; vstr sequence in State as core::clone::Clone. Now I think perhaps I've misunderstood something!

[alexcrichton]

@rillian hm unfortunately I can't seem to reproduce with your test case. Could you gist the commands you're using to reproduce? Also, what target are you using for this?

[alexcrichton]

Er, and to be concrete to what I'm seeing:

$ cat foo.rs

pub struct Parser {
    state: State,
}

#[derive(Clone)]
pub struct State {
    // This struct must have at least two members to trigger
    // NEON code generation.
    pub u: u32,
    pub v: u32,
}

#[no_mangle]
pub unsafe fn parser_new(state: *const State) -> *mut Parser {
    let parser = Box::new(Parser {
        state: (*state).clone(),
    });
    Box::into_raw(parser)

}

$ rustc +nightly -C opt-level=3 --target armv7-unknown-linux-gnueabihf foo.rs --crate-type lib --emit asm && cat foo.s
warning: field is never used: `state`, #[warn(dead_code)] on by default
 --> foo.rs:3:5
  |
3 |     state: State,
  |     ^^^^^^^^^^^^

	.text
	.syntax unified
	.eabi_attribute	67, "2.09"
	.eabi_attribute	6, 10
	.eabi_attribute	7, 65
	.eabi_attribute	8, 1
	.eabi_attribute	9, 2
	.fpu	vfpv3-d16
	.eabi_attribute	15, 1
	.eabi_attribute	16, 1
	.eabi_attribute	17, 2
	.eabi_attribute	20, 1
	.eabi_attribute	21, 1
	.eabi_attribute	23, 3
	.eabi_attribute	34, 1
	.eabi_attribute	24, 1
	.eabi_attribute	25, 1
	.eabi_attribute	28, 1
	.eabi_attribute	38, 1
	.eabi_attribute	14, 0
	.file	"foo.cgu-0.rs"
	.section	.text.parser_new,"ax",%progbits
	.globl	parser_new
	.p2align	2
	.type	parser_new,%function
parser_new:
	.fnstart
	.save	{r4, r5, r11, lr}
	push	{r4, r5, r11, lr}
	ldm	r0, {r4, r5}
	mov	r0, #8
	mov	r1, #4
	bl	__rust_allocate
	cmp	r0, #0
	stmne	r0, {r4, r5}
	popne	{r4, r5, r11, pc}
	bl	_ZN5alloc3oom3oom17h9abadf64e5736b05E
.Lfunc_end0:
	.size	parser_new, .Lfunc_end0-parser_new
	.fnend


	.section	".note.GNU-stack","",%progbits
	.eabi_attribute	30, 2

[rillian]

Sorry, I meant to include the command line. I was using:

$ $ rustup run nightly rustc --emit asm --crate-type=rlib --target armv7-linux-androideabi foo.rs && fgrep vld foo.s
warning: field is never used: `state`, #[warn(dead_code)] on by default
 --> foo.rs:4:5
  |
4 |     state: State,
  |     ^^^^^^^^^^^^

	vldr	d0, [r11, #-24]

[rillian]

Adding -O makes the vldr disappear.

[alexcrichton]

@rillian oh for Android it's slightly different. #36933 only touched the Linux targets, not the Android ones.

For some reason I thought we still enabled NEON by default on armv7 Android b/c Google said it was required, but that's not the case apparently. In light of that it does indeed seem that we're missing a -neon in the target feature set of android.

Note that you can confirm NEON is enabled through:

$ rustc +nightly --print cfg --target armv7-linux-androideabi
debug_assertions                                      
target_arch="arm"                                     
target_endian="little"                                
target_env=""                                         
target_family="unix"                                  
target_feature="neon"                                 
target_feature="vfp2"                                 
target_feature="vfp3"                                 
target_has_atomic="16"                                
target_has_atomic="32"                                
target_has_atomic="64"                                
target_has_atomic="8"                                 
target_has_atomic="ptr"                               
target_os="android"                                   
target_pointer_width="32"                             
target_vendor="unknown"                               
unix                                                  

@rillian want to send a PR to disable neon by default on armv7 Android?

[rillian]

Aha! Thanks for explaining. I completely missed the android/gnu difference. I'll send a PR.