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

segfault with target-feature=+soft-float #63466

Closed
roblabla opened this issue Aug 11, 2019 · 9 comments
Closed

segfault with target-feature=+soft-float #63466

roblabla opened this issue Aug 11, 2019 · 9 comments
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-linux Operating system: Linux P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@roblabla
Copy link
Contributor

roblabla commented Aug 11, 2019

The following program currently segfaults on x86_64-unknown-linux-gnu when compiling with stable rust 1.36.0 with soft-float, and running with TARGET unset.

use std::env;

fn main(){
    let target = env::var("TARGET").unwrap();
    println!("{}", target);
}

Here's the output:

roblabla@roblab ~/Dropbox/dev/src/rust/hello_world $ env "RUSTFLAGS=-C target-feature=+soft-float" cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/hello_world`
Segmentation fault (core dumped)

A backtrace:

#0  <core::str::lossy::Utf8LossyChunksIter as core::iter::traits::iterator::Iterator>::next () at src/libcore/str/lossy.rs:62
#1  0x00005555555619b8 in std::sys_common::bytestring::debug_fmt_bytestring () at src/libstd/sys_common/bytestring.rs:16
#2  <std::sys_common::os_str_bytes::Slice as core::fmt::Debug>::fmt () at src/libstd/sys_common/os_str_bytes.rs:27
#3  0x000055555557507e in core::fmt::builders::DebugTuple::field::{{closure}} () at src/libcore/fmt/builders.rs:288
#4  core::result::Result<T,E>::and_then () at src/libcore/result.rs:639
#5  core::fmt::builders::DebugTuple::field () at src/libcore/fmt/builders.rs:276
#6  0x0000555555564264 in <std::env::VarError as core::fmt::Debug>::fmt () at src/libstd/env.rs:239
#7  0x00005555555759bc in core::fmt::write () at src/libcore/fmt/mod.rs:1016
#8  0x0000555555563725 in core::fmt::Write::write_fmt () at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/fmt/mod.rs:195
#9  std::panicking::continue_panic_fmt::PanicPayload::fill::{{closure}} () at src/libstd/panicking.rs:356
#10 core::option::Option<T>::get_or_insert_with () at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/option.rs:816
#11 std::panicking::continue_panic_fmt::PanicPayload::fill () at src/libstd/panicking.rs:354
#12 <std::panicking::continue_panic_fmt::PanicPayload as core::panic::BoxMeUp>::get () at src/libstd/panicking.rs:369
#13 0x00005555555639dc in std::panicking::rust_panic_with_hook () at src/libstd/panicking.rs:473
#14 0x0000555555563572 in std::panicking::continue_panic_fmt () at src/libstd/panicking.rs:381
#15 0x0000555555563456 in rust_begin_unwind () at src/libstd/panicking.rs:308
#16 0x00005555555734ed in core::panicking::panic_fmt () at src/libcore/panicking.rs:85
#17 0x000055555555941f in core::result::unwrap_failed (msg=..., error=...) at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/macros.rs:18
#18 0x000055555555957b in core::result::Result<T,E>::unwrap (self=...) at /rustc/a53f9df32fbb0b5f4382caaad8f1a46f36ea887c/src/libcore/result.rs:800
#19 0x0000555555558560 in hello_world::main () at src/main.rs:4

Binary attached.

hello_world.zip

@Centril Centril added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-linux Operating system: Linux T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Aug 11, 2019
@nikomatsakis
Copy link
Contributor

Compiler team pre-triage: Seems problematic! @roblabla do you know if this used to work in older versions? Or has it always been buggy?

@nikomatsakis nikomatsakis added P-high High priority and removed I-nominated labels Aug 15, 2019
@nikic
Copy link
Contributor

nikic commented Aug 15, 2019

When using +soft-float on x86, you generally also need to specify -mmx,-sse,-sse2 (possibly other FP features).

@hanna-kruppe
Copy link
Contributor

The use of soft-float disables use of xmm registers, but parts of std are compiled without soft-float and expect parameters to be passed in xmm registers. This can be seen by looking at the assembly build without soft-float, searching for xmm, and comparing with same call under +soft-float. In this example, core::result::unwrap_failed, but this function is is no way "the culprit" or the only one (or one of a few) affected.

Note that adding target features to individual functions with the #[target_feature] attribute makes the function unsafe in part because of such ABI mismatches. (I don't think the attribute allows removing target features but that's not really relevant.)

However, we don't have a similar safeguard when mixing crates compiled with different "global" target-features. I don't know if reasonably can (and want) do that. Maybe we could put the target-features into metadata and error if they don't match during crate loading, but that's going to break some popular workflows that work fine today. In particular, -Ctarget-cpu=native would become unusable without Xargo, as would more specific -Ctarget-features that don't affect ABIs (e.g., bmi).

@nikomatsakis
Copy link
Contributor

We discussed this in today's compiler team triage meeting. We decided to close this issue in favor of the (newly opened) #63597, which indicates that we ought to document the safe usage of target features. Thanks @roblabla for bringing this to our attention. =)

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 13, 2019

However, we don't have a similar safeguard when mixing crates compiled with different "global" target-features. I don't know if reasonably can (and want) do that.

I think that we want a solution that either errors when incompatible features are used, or works (e.g. generates shims).

How we store the target-feature information is an implementation-detail (and the crate metadata sounds like a reasonable place to put this information for the features a crate was compiled with), but since functions can also have target features that deviate from the crate, we probably need to perform a validation at each call-site, comparing the target features of the caller with that of the callee.

The interaction is also ABI dependent, e.g., target-features that are incompatible with the C ABI (e.g. avx2 vs sse) might be compatible with the Rust ABI (e.g. because types are passed by memory and not in registers).

So I think that more precisely, the target features are part of the call ABI of a function (cc @eddyb ), and when calling a function we should check whether the function can be called as is, or whether shims are required and we can generate them, or error instead.

The problem here is exacerbated because we pass the -C target-features=... without any validation to LLVM (e.g. one can currently pass features that are not white-listed IIRC).

In particular, -Ctarget-cpu=native would become unusable without Xargo, as would more specific -Ctarget-features that don't affect ABIs (e.g., bmi).

native would just need to be expanded by rustc into a list of target-features, and then normal validation should be performed.

@hanna-kruppe
Copy link
Contributor

native would just need to be expanded by rustc into a list of target-features, and then normal validation should be performed.

My point is that you'll always be linking in at least libcore, which was compiled with a different and most likely incompatible list of features (unless you're using Xargo or an equivalent Cargo capability which doesn't exist yet).

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 14, 2019

My point is that you'll always be linking in at least libcore, which was compiled with a different and most likely incompatible list of features

If it were true that libcore is often compiled in an incompatible way, using -C target-cpu=native would be very often incorrect. In my experience, it isn't. While there are features that are truly incompatible with each other in an unfixable way, most features are actually compatible. For example, consider:

#[repr(simd)] f64x4(f64, f64, f64, f64);
#[target_feature(enable = "avx")] fn foo(x: f64x4) { ... }
#[target_feature(enable = "sse")] fn bar(x: f64x4} { foo(x) }

We currently incorrectly assume that foo and bar are extern "Rust" fn(f64x4), but that is wrong: foo is "avx" extern "Rust" fn(f64x4) while bar is "sse" extern "Rust" fn(64x4). The SSE version expects the f64x4 to be passed in two 128-bit wide registers, while the AVX version passes it in a 256-bit wide register. When the AVX version passes the argument in a 256-bit register to the SSE version, the behavior is undefined. Is this a user bug? I don't think so. Are these features "incompatible"? I don't think so. I think this is a fundamental rustc bug. rustc assumes that target-features are not part of the call ABI of a function, and ends up generating code that has undefined behavior. Instead, rustc should just appropriately respect the call ABI of these functions, and split the 256-bit register into two 128-bit registers when calling foo from bar to properly follow foo's calling convention.

Compiling whole crates with different features is only one of the many ways to generate functions that have different sets of target features. It is not the only one, and it is not the bug itself.

In some cases, calling one function from another might be truly incompatible in an unfixable way. We should just error when that happens, and the error should say what went wrong. On top of the fix, we could also maybe have a warning that warns when rustc needs to shim these functions to make the call ABI work.

@hanna-kruppe
Copy link
Contributor

Please take a step back to consider context. In the month old comment you quoted, I was talking about the possibility of erroring when crates are linked together which are compiled with different "global" target_features. That would also have prevented the issue @roblabla experienced, and it's vastly simpler to implement than everything you propose here (none of which is original, IIRC, the two of us and several others have discussed it extensively in the past).

I am very well aware of what we could do to make calls between functions with different sets of target_features work fine. It is indeed conceivable for us to paper over all the ABI mismatches currently caused both by #[target_feature] attributes and by mixing crates compiled with different -C target-features. But it still hasn't been done after years, presumably because it's non-trivial, so I thought about (and sadly found impractical) a more short-term improvement to the status quo.

@amab8901
Copy link
Contributor

if this issue is closed, then maybe you should stop referring to it on this page?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-linux Operating system: Linux P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants