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

Define shims for stack probes using naked functions with RFC 2873 style asm! #2082

Closed
wants to merge 2 commits into from

Conversation

nlewycky
Copy link
Contributor

@nlewycky nlewycky commented Feb 1, 2021

This should fix #1722.

It's not clear whether we can merge this, new features are enabled in the lib.rs

Comment on lines +401 to +447
cfg_if::cfg_if! {
if #[cfg(all(
target_os = "windows",
target_env = "msvc",
target_pointer_width = "64"
))] {
extern "C" {
/// The probestack for Windows when compiled with MSVC
pub fn __chkstk();
}
/// Probestack check
#[naked]
#[no_mangle]
pub unsafe extern "C" fn wasmer_probestack() {
asm!("jmp {}", sym __chkstk, options (noreturn));
}
} else if #[cfg(all(target_os = "windows", target_env = "gnu"))] {
extern "C" {
/// ___chkstk (note the triple underscore) is implemented in compiler-builtins/src/x86_64.rs
/// by the Rust compiler for the MinGW target
pub fn ___chkstk();
}
/// Probestack check
#[naked]
#[no_mangle]
pub unsafe extern "C" fn wasmer_probestack() {
asm!("jmp {}", sym ___chkstk, options (noreturn));
}
} else if #[cfg(any(target_arch = "x86_64", target_arch="x86"))] {
extern "C" {
/// Stack probe function for rust.
pub fn __rust_probestack();
}
/// Probestack check
#[naked]
#[no_mangle]
pub unsafe extern "C" fn wasmer_probestack() {
asm!("jmp {}", sym __rust_probestack, options (noreturn));
}
} else {
/// Probestack check is not used on this platform.
#[no_mangle]
pub unsafe extern "C" fn wasmer_probestack() {
panic!();
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to move this to the probestack file, if possible

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

It would be a good idea to re-enable the skipped test

@Hywan
Copy link
Contributor

Hywan commented Feb 1, 2021

👍

@Hywan Hywan added bug Something isn't working 📦 lib-compiler-llvm About wasmer-compiler-llvm 📦 lib-vm About wasmer-vm and removed 📦 lib-compiler-llvm About wasmer-compiler-llvm labels Feb 1, 2021
Comment on lines +22 to +23
#![feature(asm)]
#![feature(naked_functions)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider cfg_attr(nightly, feature(asm))

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to ship a nightly-only feature

Comment on lines +401 to +406
cfg_if::cfg_if! {
if #[cfg(all(
target_os = "windows",
target_env = "msvc",
target_pointer_width = "64"
))] {
Copy link
Contributor

Choose a reason for hiding this comment

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

And if we want to ship this for nigthly-only, we should feature gate this code based on nightly vs not nightly

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Since this is in the VM layer, and we already have C code in VM layer, perhaps we can bypass the "asm" feature if the user is not in nightly when compiling by using a asm function implemented in C.

@nlewycky
Copy link
Contributor Author

nlewycky commented Feb 2, 2021

Since this is in the VM layer, and we already have C code in VM layer, perhaps we can bypass the "asm" feature if the user is not in nightly when compiling by using a asm function implemented in C.

C proper, as in official standard C, doesn't permit assembly in it either. That said, gcc and clang support assembly in C files for linux and mac with the same syntax. We'd need to do something different for Windows though.

@Hywan Hywan self-assigned this Jul 19, 2021
@syrusakbary
Copy link
Member

Closing this as it was fixed by #2548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-vm About wasmer-vm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

probestack broken with engine-native and compiler-llvm
4 participants