Skip to content

asm2wasm: incorrect translation of switch for big 64 bit values #1109

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

Closed
pepyakin opened this issue Jul 23, 2017 · 7 comments
Closed

asm2wasm: incorrect translation of switch for big 64 bit values #1109

pepyakin opened this issue Jul 23, 2017 · 7 comments

Comments

@pepyakin
Copy link
Contributor

Source: rust-lang/rust#42630 (comment)

For example:

function () {
  "use asm";

  var abort=env.abort;

  function main() {
    var $0 = i64(), $x = i64();
    $x = i64_const(0,0);
    $0 = $x;
    switch (i64($0)) {
      case i64_const(0,2146435072):  {
          abort();
	  break;
      }
      default: {
        return;
      }
    }
  }
  return { main: main };
}

gets translated to

(module
 (type $FUNCSIG$v (func))
 (import "env" "abort" (func $abort))
 (import "env" "memory" (memory $0 256 256))
 (import "env" "table" (table 0 0 anyfunc))
 (import "env" "memoryBase" (global $memoryBase i32))
 (import "env" "tableBase" (global $tableBase i32))
 (export "main" (func $main))
 (func $main
  (local $$0 i64)
  (local $$x i64)
  (set_local $$x
   (i64.const 0)
  )
  (set_local $$0
   (get_local $$x)
  )
  (block $switch
   (block $switch-default
    (block $switch-case
     (br_table $switch-case $switch-default
      (i32.wrap/i64
       (i64.sub
        (get_local $$0)
        (i64.const 9218868437227405312)
       )
      )
     )
    )
    (block
     (call $abort)
     (br $switch)
    )
   )
   (return)
  )
 )
)

IIUC, i32.wrap/i64 takes i64 and trunctates it to only lower 32 bits. So in this case, result of i32.wrap/i64 will be 0, meaning first switch-case will be taken and then program aborted.

@kripken
Copy link
Member

kripken commented Jul 24, 2017

Thanks for reporting, proposed fix in #1111. This makes it check if the offset condition has any upper bits, in which case it branches to the default. Does the output look ok to you?

 (func $main
  (local $$0 i64)
  (local $$x i64)
  (local $2 i64)
  (set_local $$x
   (i64.const 0)
  )
  (set_local $$0
   (get_local $$x)
  )
  (block $switch
   (block $switch-default
    (block $switch-case
     (br_table $switch-case $switch-default
      (i32.wrap/i64
       (block (result i64)
        (set_local $2
         (i64.sub
          (get_local $$0)
          (i64.const 9218868437227405312)
         )
        )
        (br_if $switch-default
         (i32.wrap/i64
          (i64.shr_u
           (get_local $2)
           (i64.const 32)
          )
         )
        )
        (get_local $2)
       )
      )
     )
    )
    (block
     (call $abort)
     (br $switch)
    )
   )
   (return)
  )
 )

@pepyakin
Copy link
Contributor Author

pepyakin commented Jul 24, 2017

Looks good to me. However, I would like to validate this PR on the original case. But I'm not sure how can I build emscripten with binaryen on #1111 branch...

@kripken
Copy link
Member

kripken commented Jul 24, 2017

To validate, check out and build binaryen manually. Then set the env var BINARYEN to point to the the dir containing binaryen when running emcc.

@pepyakin
Copy link
Contributor Author

Is there any way to pass this to rustc?
BINARYEN env var doesn't work (

@kripken
Copy link
Member

kripken commented Jul 24, 2017

Hmm, I don't know how rustc works. But if the env var fails, you can edit the ~/.emscripten file to point BINARYEN_ROOT to the dir you have binaryen. (Unless rustc customizes the location of the .emscripten file? Probably it doesn't but I'm not sure.)

@pepyakin
Copy link
Contributor Author

Yep, that worked, thank you!
Can confirm that the original case is valid!

@kripken
Copy link
Member

kripken commented Jul 26, 2017

Thanks for confirming, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants