Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array as asm input generates wrong code #13366

Closed
kmcallister opened this issue Apr 7, 2014 · 5 comments
Closed

Array as asm input generates wrong code #13366

kmcallister opened this issue Apr 7, 2014 · 5 comments
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@kmcallister
Copy link
Contributor

#[feature(asm)];

#[inline(never)]
unsafe fn print_first_half(arr: [u8, ..16]) {
    let mut out: u64;
    asm!("movups $1, %xmm0
          pextrq $$0, %xmm0, $0"
          : "=r"(out) : "m"(arr) : "xmm0");
    println!("{:?}", out);
}

fn main() {
    let arr: [u8, ..16] = [0, ..16];
    unsafe { print_first_half(arr); }
}
$ rustc -v
rustc 0.10-pre (68a4f7d 2014-02-24 12:42:02 -0800)
host: x86_64-unknown-linux-gnu

$ rustc -O foo.rs && ./foo
140489369304528u64

This should be 0u64; try replacing the movups with xorps %xmm0, %xmm0. Here's the generated code:

$ objdump -d foo
…
  4069f9:       48 8b 07                mov    (%rdi),%rax
  4069fc:       48 8b 4f 08             mov    0x8(%rdi),%rcx
  406a00:       48 89 4c 24 48          mov    %rcx,0x48(%rsp)
  406a05:       48 89 44 24 40          mov    %rax,0x40(%rsp)
  406a0a:       48 8d 44 24 40          lea    0x40(%rsp),%rax
  406a0f:       48 89 44 24 08          mov    %rax,0x8(%rsp)
  406a14:       0f 10 44 24 08          movups 0x8(%rsp),%xmm0
  406a19:       66 48 0f 3a 16 c0 00    pextrq $0x0,%xmm0,%rax
…

So it copies the array to 0x40(%rsp) (in two 64-bit pieces), then puts that address at 0x8(%rsp), and movups loads 16 bytes from there rather than from the array itself.

In GCC, I would do

void f(char *arr) {
    asm("movups %0, %%xmm0" :: "m"(*arr));
}

which gcc -O3 turns into the optimal

   0:   0f 10 07                movups (%rdi),%xmm0

Attempting to do the same in Rust

asm!("movups $0, %xmm0" :: "m"(*(arr.as_ptr())) : "xmm0");

produces even wronger code

  4069f9:       8a 07                   mov    (%rdi),%al
  4069fb:       88 04 24                mov    %al,(%rsp)
  4069fe:       0f 10 04 24             movups (%rsp),%xmm0

Workarounds:

  1. When the array is a static with a name, just name it within the asm!. See pub static disappears if only used from asm #13365.

asm!("movups ($0), %xmm0" : : "r"(arr.as_ptr()) : "xmm0");

which generates optimal code in this case, because the array is already pointed to by %rdi, but in general may clobber a register and emit a load when neither is necessary.

@kmcallister
Copy link
Contributor Author

I spent a while today poking at this. trans::asm calls callee::trans_arg_datum, which is the same function used to translate ordinary function arguments. So we end up passing a pointer for structs and arrays bigger than a machine word.

I'm not sure exactly how to fix this, but naive solutions (such as always loading from lvalues and by-ref rvalues) produced incorrect code. I think we want to keep them by-ref and change the m constraint to *m. At any rate that's what clang does; see CodeGenFunction::EmitAsmInputLValue.

@steveklabnik
Copy link
Member

Triage: no update that I'm aware of.

bors added a commit that referenced this issue Nov 4, 2015
The "m" memory constraint in inline assembly is broken (generates incorrect code or triggers LLVM asserts) and should not be used. Instead, indirect memory operands should be used with "\*m", "=\*m" and "+\*m".

Clang does this transparently by transforming "m" constraints into "\*m" indirect constraints, but for now just being able to use "\*m" directly is enough since asm! isn't stable.

While "\*m" works fine as an input operand, "=\*m" and "+\*m" need to be specified as input operands because they take a pointer value as an input. This PR relaxes the constraint checker to allow constraints starting with "=" or "+" if the constraint string contains a "\*", which indicates an indirect operand.

This (indirectly) fixes these issues: #29382, #16383 and #13366. The code will need to be changed to use "\*m" instead of "m".
@mrhota
Copy link
Contributor

mrhota commented Mar 1, 2017

@kmcallister @steveklabnik I tested the OP code (slightly updated) as follows:

#![feature(asm)]

#[inline(never)]
unsafe fn print_first_half(arr: [u8; 16]) {
    let out: u64;
    asm!("movups $1, %xmm0
          pextrq $$0, %xmm0, $0"
          : "=r"(out) : "m"(arr) : "xmm0");
    println!("{:?}", out);
}

fn main() {
    let arr: [u8; 16] = [0; 16];
    unsafe { print_first_half(arr); }
}

and we get an ICE on nightly when emitting ASM or LLVM IR. Generating MIR does not ICE.

See code at https://is.gd/MQGiQN

ICE report is here: #40187

@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. and removed I-wrong labels Jul 20, 2017
@steveklabnik
Copy link
Member

Triage: ICE not fixed

@Centril Centril added the requires-nightly This issue requires a nightly compiler in some way. label Oct 25, 2019
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2020
@Amanieu
Copy link
Member

Amanieu commented May 22, 2020

This issue does not apply to the new asm! (RFC 2850) which has stricter checks on the types that can be used as asm operands.

The legacy llvm_asm! is deprecated and is no longer maintained.

@Amanieu Amanieu closed this as completed May 22, 2020
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Oct 11, 2022
internal: ⬆️ xflags

The main change here should be that flags are not inhereted, so

   $ rust-analyzer analysis-stats . -v -v

would do what it should do

We also no longer Don\'t
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 3, 2024
fix: Specifying reason in expect(clippy::needless_return) no longer triggers false positive

fixes rust-lang#13366
changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants