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

-panic=trap still prints panic messages #4161

Closed
jharveyb opened this issue Feb 28, 2024 · 3 comments · Fixed by #4195
Closed

-panic=trap still prints panic messages #4161

jharveyb opened this issue Feb 28, 2024 · 3 comments · Fixed by #4195

Comments

@jharveyb
Copy link

Related to #3068, #2491.

tl;dr there should be a panic build option to short-circuit runtime.runtimePanicAt to just unreachable vs. printing a string.

I was trying to produce a WASM progra, that could accept & return a string from/to the host, similar to this wazero example.

I compiled with tinygo v0.30.0, tinygo build -o main.wasm -no-debug -panic=trap -scheduler=none -gc=leaking -target=wasm main.go.

Inspecting the output with wasmer, I found wasi_snapshot_preview1 fd_write being imported. My program does not print anything, and only imports unsafe.

Converting the WASM to WAT, I found that the runtime will unconditionally try to print during a runtimePanic. This feels pretty unavoidable, as funcs in the unsafe package like Slice or String can panic, but are also useful for passing memory between the guest and host.

Relevant WAT snippet:

(func $runtime.runtimePanicAt (type 4) (param i32 i32)
    i32.const 65536
    i32.const 22
    call $runtime.printstring
    local.get 0
    local.get 1
    call $runtime.printstring
    i32.const 10
    call $runtime.putchar
    unreachable)
  (func $runtime.printstring (type 4) (param i32 i32)
    local.get 1
    i32.const 0
    local.get 1
    i32.const 0
    i32.gt_s
    select
    local.set 1
    loop  ;; label = @1
      local.get 1
      if  ;; label = @2
        local.get 0
        i32.load8_u
        call $runtime.putchar
        local.get 1
        i32.const 1
        i32.sub
        local.set 1
        local.get 0
        i32.const 1
        i32.add
        local.set 0
        br 1 (;@1;)
      end
    end)
  (func $runtime.putchar (type 1) (param i32)
    (local i32 i32)
    i32.const 65716
    i32.load
    local.tee 1
    i32.const 119
    i32.le_u
    if  ;; label = @1
      i32.const 65716
      local.get 1
      i32.const 1
      i32.add
      local.tee 2
      i32.store
      local.get 1
      i32.const 65720
      i32.add
      local.get 0
      i32.store8
      local.get 0
      i32.const 255
      i32.and
      i32.const 10
      i32.ne
      i32.const 0
      local.get 1
      i32.const 119
      i32.ne
      select
      i32.eqz
      if  ;; label = @2
        i32.const 65668
        local.get 2
        i32.store
        i32.const 1
        i32.const 65664
        i32.const 1
        i32.const 65856
        call $runtime.fd_write
        drop
        i32.const 65716
        i32.const 0
        i32.store
      end
      return
    end
    i32.const 65581
    i32.const 18
    call $runtime.runtimePanicAt
    unreachable)

Call graph:

flowchart TD
  A[runtime.slicePanic] & B[runtime.nilPanic] --> C[runtime.runtimePanicAt]
  C --> D[runtime.printstring] & E[runtime.putchar]
  D --> E
  E --> F[runtime.fd_write]

Rust has the build option panic_immediate_abort to omit any string formatting on panic, for similar reasons of small code size & minimal dependencies. Adding something similar to TinyGo would allow for WASM programs that include the runtime without also requiring a WASI-compliant host.

@jharveyb jharveyb changed the title wasm: add new panic setting to mirror "panic_immediate_abort" wasm: add new panic setting to mirror rustlang "panic_immediate_abort" Feb 28, 2024
@aykevl
Copy link
Member

aykevl commented Mar 16, 2024

The bug here is that TinyGo still calls runtime.putchar even with -panic=trap. We don't need a new option, we just need to make that option work correctly.

@aykevl aykevl changed the title wasm: add new panic setting to mirror rustlang "panic_immediate_abort" -panic=trap still prints panic messages Mar 16, 2024
aykevl added a commit that referenced this issue Mar 16, 2024
Support for `-panic=trap` was previously a pass in the optimization
pipeline. This change moves it to the compiler and runtime, which in my
opinion is a much better place.

As a side effect, it also fixes
#4161 by trapping inside
runtime.runtimePanicAt and not just runtime.runtimePanic.

This change also adds a test for the list of imported functions. This is
a more generic test where it's easy to add more tests for WebAssembly
file properties, such as exported functions.
@aykevl
Copy link
Member

aykevl commented Mar 16, 2024

Here is a fix: #4195
Can you test it for your use case?

deadprogram pushed a commit that referenced this issue Mar 19, 2024
Support for `-panic=trap` was previously a pass in the optimization
pipeline. This change moves it to the compiler and runtime, which in my
opinion is a much better place.

As a side effect, it also fixes
#4161 by trapping inside
runtime.runtimePanicAt and not just runtime.runtimePanic.

This change also adds a test for the list of imported functions. This is
a more generic test where it's easy to add more tests for WebAssembly
file properties, such as exported functions.
@jharveyb
Copy link
Author

Here is a fix: #4195 Can you test it for your use case?

This worked btw, thanks for the fix!

I switched to a custom target like that specified in #4174 to drop all the runtime GC functions.

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

Successfully merging a pull request may close this issue.

2 participants