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

reproducable wasm32 bug #85580

Closed
gilescope opened this issue May 22, 2021 · 7 comments · Fixed by #88603
Closed

reproducable wasm32 bug #85580

gilescope opened this issue May 22, 2021 · 7 comments · Fixed by #88603
Assignees
Labels
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-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gilescope
Copy link
Contributor

I'm really sorry if this isn't a compiler bug, but it smells like one. I've managed to bag it into a few lines of code. Change or reorder anything and the test will pass.

If you run cargo test this repo it will pass:

https:/gilescope/wasm-repro1.git

If you add in --target=wasm32-unknown-unknown it will fail. ( You will need to cargo install wasm-bindgen-cli)
It seems to correctly figure out that the condion is false but then take the true branch of the if anyway. There's no unsafe in the code so there should be no UB....

#[wasm_bindgen_test]
#[test]
fn test() {
    let s: &[u8] = &[1];
    let masked = s[0];
    // The below cond should evaluate to false but,
    // what seems to happen is even though it evaluates to false,
    // the if takes the wrong branch!!!!
    let cond = s[0] as u32 == 0; //should evaluate to false.

    // Comment out line below to pass the test on --target=wasm32-unknown-unknown ????
    let m1 = masked << 0;

    assert_eq!(Err(()), if cond {
        Ok(3333)
    } else {
        //Should go here.
        Err(())
    });
}

Why for instance does << 0 have to be there for the bug to manifest itself? I am clueless...

This is with the latest nightly ( rustc 1.54.0-nightly (5dc8789 2021-05-21) ), but have confirmed it fails on beta too.

I would love to know what is going on - it seems very wierd behaviour.

@gilescope gilescope added the C-bug Category: This is a bug. label May 22, 2021
@jonas-schievink jonas-schievink added 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/ labels May 22, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 22, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 25, 2021
@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 25, 2021
@RalfJung
Copy link
Member

Smells like an LLVM miscompilation issue? Since it is target-specific, a MIR issue seems unlikely. Cc @rust-lang/wg-llvm

@apiraino
Copy link
Contributor

apiraino commented Aug 26, 2021

I'm afraid the ping didn't work

@rustbot ping @rust-lang/wg-llvm

cc: @cuviper @nikic @nagisa

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2021

Error: This team (@rust-lang/wg-llvm) cannot be pinged via this command; it may need to be added to triagebot.toml on the master branch.

Please let @rust-lang/release know if you're having trouble with this bot.

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2021

Error: This team (@rust-lang/wg-llvm~) cannot be pinged via this command; it may need to be added to triagebot.toml on the master branch.

Please let @rust-lang/release know if you're having trouble with this bot.

@nikic
Copy link
Contributor

nikic commented Aug 26, 2021

target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128-ni:1:10:20"
target triple = "wasm32-unknown-unknown"

define i32 @test(i8* %p, i8* %p2) {
  %v = load i8, i8* %p
  %v.ext = zext i8 %v to i32
  %cond = icmp eq i32 %v.ext, 0
  %shl = shl i8 %v, 0
  store i8 %shl, i8* %p2
  br label %bb2

bb2:
  br i1 %cond, label %bb3, label %bb4

bb4:
  ret i32 0

bb3:
  ret i32 1
}

With llc -O0

	.text
	.file	"test339.ll"
	.section	.text.test,"",@
	.globl	test                            # -- Begin function test
	.type	test,@function
test:                                   # @test
	.functype	test (i32, i32) -> (i32)
	.local  	i32, i32, i32, i32
# %bb.0:
	local.get	0
	i32.load8_u	0
	local.set	2
	local.get	2
	i32.eqz
	drop
	local.get	1
	local.get	2
	i32.store8	0
# %bb.1:                                # %bb2
	block   	
	local.get	3
	i32.eqz
	br_if   	0                               # 0: down to label0
# %bb.2:                                # %bb4
	i32.const	0
	local.set	4
	local.get	4
	return
.LBB0_3:                                # %bb3
	end_block                               # label0:
	i32.const	1
	local.set	5
	local.get	5
	return
	end_function
.Lfunc_end0:
	.size	test, .Lfunc_end0-test
                                        # -- End function

Note that local.get 3 reads a local that has not been initialized.

@nikic
Copy link
Contributor

nikic commented Aug 27, 2021

Upstream issue: https://bugs.llvm.org/show_bug.cgi?id=51651

@nikic nikic self-assigned this Aug 27, 2021
@bors bors closed this as completed in 6b9a227 Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-high High priority 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.

6 participants