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

Wasm32-unknown-unknown does not need SYS_ROOT override #1126

Closed
NobodyXu opened this issue Jul 4, 2024 · 5 comments · Fixed by #1284
Closed

Wasm32-unknown-unknown does not need SYS_ROOT override #1126

NobodyXu opened this issue Jul 4, 2024 · 5 comments · Fixed by #1284

Comments

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jul 4, 2024

#1114 conflates wasm with wasi targets. While wasi targets should change the sysroot to the wasi-sdk-provided one, the non-wasi target wasm32-unknown-unknown should probably keep the default sysroot.

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 1, 2024

Things also changed a bit in #1225, we should be using target.os == "wasi" or target.arch == "wasm32" || target.arch == "wasm64" depending on what the desired condition is.

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 6, 2024

Reviewing all the places where wasm or wasi is mentioned:

  1. Uses WASI_SYSROOT, seems like it is correct to use target.os:

    cc-rs/src/lib.rs

    Lines 1406 to 1415 in 2050013

    // Link c++ lib from WASI sysroot
    if target.os == "wasi" {
    if let Ok(wasi_sysroot) = self.wasi_sysroot() {
    self.cargo_output.print_metadata(&format_args!(
    "cargo:rustc-flags=-L {}/lib/{} -lstatic=c++ -lstatic=c++abi",
    Path::new(&wasi_sysroot).display(),
    self.get_raw_target()?
    ));
    }
    }
  2. Might need target.arch,

    cc-rs/src/lib.rs

    Lines 1996 to 2011 in 2050013

    // Disable generation of PIC on bare-metal for now: rust-lld doesn't support this yet
    if self.pic.unwrap_or(
    target.os != "windows"
    && target.os != "none"
    && target.os != "uefi"
    && target.os != "wasi",
    ) {
    cmd.push_cc_arg("-fPIC".into());
    // PLT only applies if code is compiled with PIC support,
    // and only for ELF targets.
    if (target.os == "linux" || target.os == "android")
    && !self.use_plt.unwrap_or(true)
    {
    cmd.push_cc_arg("-fno-plt".into());
    }
    }
  3. Might need target.arch for -fno-exceptions and -pthread?

    cc-rs/src/lib.rs

    Lines 2012 to 2027 in 2050013

    if target.os == "wasi" {
    // WASI does not support exceptions yet.
    // https://github.com/WebAssembly/exception-handling
    cmd.push_cc_arg("-fno-exceptions".into());
    // Link clang sysroot
    if let Ok(wasi_sysroot) = self.wasi_sysroot() {
    cmd.push_cc_arg(
    format!("--sysroot={}", Path::new(&wasi_sysroot).display()).into(),
    );
    }
    // FIXME(madsmtm): Read from `target_features` instead?
    if raw_target.contains("threads") {
    cmd.push_cc_arg("-pthread".into());
    }
    }
  4. Preferring Clang as the compiler might need to be done for target.arch instead?

    cc-rs/src/lib.rs

    Lines 2707 to 2712 in 2050013

    } else if target.os == "wasi" {
    if self.cpp {
    "clang++".to_string()
    } else {
    "clang".to_string()
    }
  5. The standard C++ library might need target.arch? Though might also be correct to use target.os

    cc-rs/src/lib.rs

    Lines 2936 to 2971 in 2050013

    /// Returns the C++ standard library:
    /// 1. If [`cpp_link_stdlib`](cc::Build::cpp_link_stdlib) is set, uses its value.
    /// 2. Else if the `CXXSTDLIB` environment variable is set, uses its value.
    /// 3. Else the default is `c++` for OS X and BSDs, `c++_shared` for Android,
    /// `None` for MSVC and `stdc++` for anything else.
    fn get_cpp_link_stdlib(&self) -> Result<Option<Cow<'_, Path>>, Error> {
    match &self.cpp_link_stdlib {
    Some(s) => Ok(s.as_deref().map(Path::new).map(Cow::Borrowed)),
    None => {
    if let Ok(stdlib) = self.getenv_with_target_prefixes("CXXSTDLIB") {
    if stdlib.is_empty() {
    Ok(None)
    } else {
    Ok(Some(Cow::Owned(Path::new(&stdlib).to_owned())))
    }
    } else {
    let target = self.get_target()?;
    if target.env == "msvc" {
    Ok(None)
    } else if target.vendor == "apple"
    || target.os == "freebsd"
    || target.os == "openbsd"
    || target.os == "aix"
    || (target.os == "linux" && target.env == "ohos")
    || target.os == "wasi"
    {
    Ok(Some(Cow::Borrowed(Path::new("c++"))))
    } else if target.os == "android" {
    Ok(Some(Cow::Borrowed(Path::new("c++_shared"))))
    } else {
    Ok(Some(Cow::Borrowed(Path::new("stdc++"))))
    }
    }
    }
    }
    }
  6. Probably need to also check wasm64 when finding archiver?

    cc-rs/src/lib.rs

    Lines 3097 to 3114 in 2050013

    } else if target.arch == "wasm32" {
    // Formally speaking one should be able to use this approach,
    // parsing -print-search-dirs output, to cover all clang targets,
    // including Android SDKs and other cross-compilation scenarios...
    // And even extend it to gcc targets by searching for "ar" instead
    // of "llvm-ar"...
    let compiler = self.get_base_compiler().ok()?;
    if compiler.is_like_clang() {
    name = format!("llvm-{}", tool).into();
    self.search_programs(
    &mut self.cmd(&compiler.path),
    &name,
    &self.cargo_output,
    )
    .map(|name| self.cmd(name))
    } else {
    None
    }

But ultimately, I am unsure of all of these, which is why I haven't pursued it myself.

CC @daxpedda, do you know which of these places it would be correct to use target.os == "wasi", and which ones it'd be correct to use matches!(target.arch, "wasm32" | "wasm64")?

@daxpedda
Copy link

daxpedda commented Nov 6, 2024

CC @daxpedda, do you know which of these places it would be correct to use target.os == "wasi", and which ones it'd be correct to use matches!(target.arch, "wasm32" | "wasm64")?

No, unfortunately not.

But while I'm here, I should mention that target_family == "wasm" && target_os == "unknown" might be more accurate to describe wasm32-unknown-unknown and wasm64-unknown-unknown. Because wasm32-unknown-emscripten would be covered with target.arch == "wasm32" || target.arch == "wasm64" as well. Emscripten is better covered with target_os == "emscripten".

On that note, I don't know what a sysroot for wasm32-unknown-unknown for C/C++ would represent, so maybe you are actually trying to target Emscripten here? Just speculating though, I'm clueless about the C/C++ toolchain surrounding Wasm.

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 6, 2024

No, unfortunately not.

Fair, thanks for the answer anyhow!

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Nov 7, 2024

For 1, I think you are right.

For 3, I think the code we actually trying to detect wasip1-thread

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.

3 participants