Skip to content

Use "C-unwind" API in our vtable#6930

Merged
ogoffart merged 3 commits intomasterfrom
olivier/unwind
Jun 6, 2025
Merged

Use "C-unwind" API in our vtable#6930
ogoffart merged 3 commits intomasterfrom
olivier/unwind

Conversation

@ogoffart
Copy link
Copy Markdown
Member

When using rust, allow panics to cross the boundaries of our vtable traits. This avoids panic producing two backtrace with panic=unwind

This patch doesn't touch the ABI of out FFI interface, they stay extern "C", because if a panic or exception crosses these boundaries, we are in trouble. (Also, we have a panic=abort in our Cargo.toml anyway)

@ogoffart ogoffart requested a review from tronical November 27, 2024 13:27
@ogoffart
Copy link
Copy Markdown
Member Author

ogoffart commented Nov 27, 2024

Looks like this causes problem with wasm:

error: failed getting Wasm module for '/home/runner/work/slint/slint/target/wasm32-unknown-unknown/release/slint_wasm_interpreter.wasm'

Caused by:
    0: failed to parse input as wasm
    1: failed to parse code section
    2: exceptions support is not enabled (at offset 0xbd231)
Error: Running the wasm-bindgen CLI
Caused by: Running the wasm-bindgen CLI
Caused by: failed to execute `wasm-bindgen`: exited with exit status: 1
  full command: "/home/runner/.cache/.wasm-pack/wasm-bindgen-46a41f966b4cfc69/wasm-bindgen" "/home/runner/work/slint/slint/target/wasm32-unknown-unknown/release/slint_wasm_interpreter.wasm" "--out-dir" "/home/runner/work/slint/slint/api/wasm-interpreter/pkg" "--typescript" "--target" "web"

(same error for all wasm builds)

2: exceptions support is not enabled (at offset 0xbd231)

Maybe we need to enable them somehow?

@ogoffart
Copy link
Copy Markdown
Member Author

ogoffart commented Jun 5, 2025

Added a commit that will solve the wasm failure and #8449

@ogoffart ogoffart force-pushed the olivier/unwind branch 2 times, most recently from c90f975 to 781cb17 Compare June 5, 2025 14:51
Copy link
Copy Markdown
Member

@tronical tronical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remove_extern macro is a nice solution :)

Comment thread internal/core-macros/lib.rs Outdated
quote!(#doc).into()
}

/// Attribute macro that removes `extern "..."` from the functon sugnatures
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Attribute macro that removes `extern "..."` from the functon sugnatures
/// Attribute macro that removes `extern "..."` from the functon signatures

Comment thread internal/core-macros/lib.rs Outdated

/// Attribute macro that removes `extern "..."` from the functon sugnatures
///
/// This is usefull because wasm does not support `extern "C-unwind"` and also
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This is usefull because wasm does not support `extern "C-unwind"` and also
/// This is useful because wasm does not support `extern "C-unwind"` and also

Comment thread internal/core/graphics/image.rs Outdated
#[allow(missing_docs)]
#[vtable::vtable]
#[cfg_attr(not(feature = "ffi"), i_slint_core_macros::remove_extern)]
#[vtable::vtable(no_extern)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is used unconditionally and all fn fields have an ABI, perhaps it would be a better API if it was called require_abi and error out if a fn has no ABI?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change in the vtable crate.
I could release vtable 0.3 for it though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it breaking if the proposed behaviour applies only when using the new #[vtable(require_abi)] attribute?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous #[vtable] macro was always adding the extern "C"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, indeed, that would make it a little awkward. Ok, never mind - I'm just (unintentionally) nitpicking :)

@ogoffart ogoffart force-pushed the olivier/unwind branch 3 times, most recently from a07a162 to 4efc132 Compare June 6, 2025 11:59
@ogoffart
Copy link
Copy Markdown
Member Author

ogoffart commented Jun 6, 2025

Updated to have a new major release of vtable that no longer change the extern "C" automatically.

Comment thread helper_crates/vtable/CHANGELOG.md Outdated
ogoffart added 3 commits June 6, 2025 14:47
When using rust, allow panics to cross the boundaries of our vtable traits.
This avoids panic producing two backtrace with panic=unwind

This patch doesn't touch the ABI of out FFI interface, they stay extern "C", because
if a panic or exception crosses these boundaries, we are in trouble.
(Also, we have a panic=abort in our Cargo.toml anyway)
In fact, remove it for non-C++

Because wasm doesn't support C-unwind. And actually warns about
incompatible ABI for the rest

Fixes #8449
@ogoffart ogoffart merged commit af573e6 into master Jun 6, 2025
1 of 4 checks passed
@ogoffart ogoffart deleted the olivier/unwind branch June 6, 2025 12:47
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 this pull request may close these issues.

2 participants