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

Wasm code failing regression probably related to update to LLVM 9 trunk #63918

Closed
gui1117 opened this issue Aug 26, 2019 · 6 comments · Fixed by #64317
Closed

Wasm code failing regression probably related to update to LLVM 9 trunk #63918

gui1117 opened this issue Aug 26, 2019 · 6 comments · Fixed by #64317
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gui1117
Copy link
Contributor

gui1117 commented Aug 26, 2019

Summary:

The following piece of code fail when compiled to wasm32-unknown-unknown since nightly-2019-07-18. This nightly introduces https://github.com/rust-lang/rust/pull/62592/files so probably related.

Regression test:

pub fn get() -> u128 {
	let d = [0, 0, 32, 59, 157, 181, 5, 111, 0, 0, 0, 0, 0, 0, 0, 0];
	decode_impl(&mut &d[..]).unwrap()
}

#[no_mangle]
fn main() {
	let c: u128 = get();
	assert_eq!(c, 8000000000000000000u128);
}

#[inline(never)]
fn decode_impl(input: &mut &[u8]) -> Result<u128, ()> {
	let mut buf = [0u8; 16];
	read(input, &mut buf)?;
	Ok(<u128>::from_le_bytes(buf))
}

#[inline(never)]
fn read(input: &mut &[u8], into: &mut [u8]) -> Result<(), ()> {
	if into.len() != input.len() {
		return Err(());
	}
	into.copy_from_slice(&input[..]);
	Ok(())
}

with configuration:

[lib]
name = "code"
crate-type = [
	"cdylib",
	"rlib",
]

[profile.release]
opt-level = "z"

Reproduce:

see the repo: https://github.com/thiolliere/wasm-llvm9-issue it contains:

  • javascript file t.js used to execute test with node.
  • make.sh file to execute the test on rust version 2019-07-17 and 2019-07-18.

Notes:

cc @pepyakin

@jonas-schievink jonas-schievink added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-nominated I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-wasm Target: WASM (WebAssembly), http://webassembly.org/ regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 26, 2019
@pepyakin
Copy link
Contributor

pepyakin commented Aug 26, 2019

I changed an example a bit, minimized it from the size of wasm binary prespective and used some c-reduce (which hasn't yield much improvements).

#![no_std]
use core::arch::wasm32::unreachable;
#[panic_handler]
fn panic_handler(_: &core::panic::PanicInfo) -> ! {
    unsafe { unreachable() }
}
#[no_mangle]
fn main() {
    let b = c();
    if b != 8000000000000000000u128 {
        unsafe { unreachable() }
    }
}
pub fn c() -> u128 {
    let d = [0, 0, 32, 59, 157, 181, 5, 111, 0, 0, 0, 0, 0, 0, 0, 0];
    match e(&mut &d[..]) {
        Ok(f) => f,
        _ => unsafe { unreachable() },
    }
}
#[inline(never)]
fn e(g: &&[u8]) -> Result<u128, ()> {
    let mut buf = [0; 16];
    h(g, &mut buf).ok_or_else(|| ())?;
    Ok(u128::from_le_bytes(buf))
}
fn h(g: &&[u8], i: &mut [u8]) -> Option<()> {
    if i.len() != g.len() {
        return None;
    }
    i.copy_from_slice(g);
    Some(())
}

compiling with

rustc --crate-name code lib.rs --crate-type cdylib --crate-type rlib -C opt-level=z -C codegen-units=1 --target=wasm32-unknown-unknown

there is no notable difference in LLVM IR, only some identifiers and added imm arg in e.g. declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) #4 which I believe is not related.

The miscompilation seems to happen around c function, specifically when copying low and high parts of the u128 by the out RVO ptr (gist):

;; local 1 = SP
;; local.2 = i64.load(sp + 16)
      get_local 1
      i32.const 16
      i32.add
      i64.load
      set_local 2
;; *out.0 = i64.load(sp + 8)
      get_local 0
      get_local 1
      i64.load offset=8
      i64.store
;; *(out.0+8) = local.2
      get_local 0
      get_local 2
      i64.store offset=8

in miscompiled version we just do this (gist):

;; *out.0 = local.2
    get_local 0
    get_local 2
    i64.store

which doesn't make a lot of sense since local.2 should hold i64 of descriminator of Result returned by e.

All this makes me think (although I am of course not sure) that this is a LLVM IR to wasm32 lowering bug, so cc @sunfishcode

@sunfishcode
Copy link
Member

I haven't investigated this in detail yet, but in your analysis, in the reduced version, c returns a plain u128, not a Result, so it makes sense that it's not storing the discriminator.

@pepyakin
Copy link
Contributor

pepyakin commented Aug 27, 2019

What I meant is that the descriminator of the Result returned by e (which is u64, AFAIR) is stored in first half of the out ptr *u128 (which is the return value of c as you noted)

Or in other words, it attempts to squeeze the descriminator of Result into u128.

@alexcrichton
Copy link
Member

I think I've reduced this a bit further:

; ModuleID = 'foo.ll'
source_filename = "foo.ll"
target triple = "wasm32-unknown-unknown"

@wut = private constant i128 8000000000000000000

define i128 @get() {
start:
  %0 = tail call { i64, i128 } @decode(i8* bitcast (i128* @wut to i8*))
  %1 = extractvalue { i64, i128 } %0, 1
  ret i128 %1
}

declare hidden { i64, i128 } @decode(i8* nocapture readonly)

generates:

  (func (;1;) (type 0) (param i32)
    (local i32)
    global.get 0
    i32.const 32
    i32.sub
    local.tee 1
    global.set 0
    local.get 1
    i32.const 8
    i32.add
    i32.const 0
    call 0
    local.get 0
    local.get 1
    i64.load offset=8
    i64.store
    local.get 1
    i32.const 32
    i32.add
    global.set 0)

and it looks like the upper or lower bits (dunno which, but at least one) isn't being copied over

I've opened an upstream bug at https://bugs.llvm.org/show_bug.cgi?id=43132

@nikomatsakis
Copy link
Contributor

Visiting for compiler triage meeting. I'm marking this as a "P-medium" bug since it is specific to WASM and also due to an LLVM bug. I'm not sure if we plan to fix this on our side in any way?

@nikomatsakis nikomatsakis added the P-medium Medium priority label Aug 29, 2019
@sunfishcode
Copy link
Member

The reduced testcase is now fixed in r370430.

@nikic nikic added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 31, 2019
@bors bors closed this as completed in 7b0d134 Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants