-C target-feature/-C target-cpu are unsound

In a nutshell, target-features are part of the call ABI, but Rust does not take that into account, and that's the underlying issue causing #63466, #53346, and probably others (feel free to refer them here).

For example, for an x86 target without SSE linking these two crates shows the issue:

// crate A
pub fn foo(x: f32) -> f32 { x }

// crate B
extern "Rust" {
     #[target_feature(enable = "sse")]  fn foo(x: f32) -> f32;
}
unsafe { assert_eq!(foo(42.0), 42.0) }; // UB

The ABI of B::foo is "sse" "Rust" but the ABI defined in A::foo is just "Rust", no SSE, since the SSE feature is not enabled globally. That's an ABI mismatch and results in UB of the form "calling a function with an incompatible call ABI". For this particular case, B::foo expects the f32 in an xmm register, while A::foo expects it in an x87 register, so the roundtrip of 42.0 via foo will return garbage.

Now, this example is not unsound, because it requires an incompatible function declaration, and unsafe code to call it - and arguably, the unsafe asserts that the declaration is, at least correct (note: we forbid assigning #[target_feature] functions to function pointers and only allow enabling features and using white-listed features because of this).

However, you can cause the exact same issue, but on a larger scale, by using -C target-feature/-C target-cpu to change the features that a Rust crate assumes the "Rust" ABI has, without any unsafe code:

// crate A: compiled without -C target-feature
pub fn foo(x: f32) -> f32 { x }

// crate B: compiled with -C target-feature=+sse
// this is now safe Rust code:
assert_eq!(A::foo(42.0), 42.0) }; // UB

So that's the soundness bug. Safe Rust can exhibit undefined behavior of the form "calling a function with an incompatible call ABI".

This is an issue, because when RUSTFLAGS="-C target-cpu=native" is used, not all crates in the dependency graph are compiled with those features. In particular, libstd and its dependencies are not recompiled at all, so their "Rust" ABI might be different than what the rest of the dependency graph uses. -C target-feature also allows disabling features, -C target-feature/target-cpu allow enabling/disabling features that are not white-listed (e.g. avx512f if your CPU supports it can be enabled using -C target-feature and will be enabled using -C target-cpu=skylake or =nativeeven though the avx512f feature is unstable).

How important is fixing this ? I'd say moderately important, because many features are compatible. For example, the "sse2" "Rust" ABI has the same calling convention as the "sse3" "Rust", so even though the call ABIs aren't identical, they are compatible. That is, this bug is unlikely to cause issues in practice, unless you happen to end up with two crates where the difference in target features changes the ABI.

I think that rustc should definitely detect that the ABIs are incompatible, and at least, error at compile-time when this happen, explaining what went wrong, which target-features differed in an incompatible way, etc.

We could make such code work, e.g., by just treating target-features as part of the calling convention, and following the calling convention properly. I don't think fixing this would be useful for people doing -C target-feature globally, because when that happens, chances are that they wanted to compile their whole binary in a specific way, as opposed to having different parts of the binary somehow interoperate.

It would however be useful for improving how we currently handle SIMD vectors. For example, a vector types like f64x4 will be passed in different registers (2x 128-bit or 1x 256-bit) depending on whether sse or avx are available. We currently "solve" this problem by always passing vectors to functions indirectly by memory. That is, on the caller side we spill the vector registers into the stack, create a pointer to it, pass it to the callee, and then the callee loads the content into the appropriate vectors. Since target-features are not part of the calling convention, we always do this, as opposed to, only when the calling convention is incompatible. So having target-features be part of the calling convention might be a way to improve that.

Going with p-medium based on the discussion

Is this unique to Rust's ABI? or is this also a problem in C/C++?

@elichai

@elichai C and C++ appear to handle this mostly via using the hardware vector types in question and then passing the rest of the burden on to the programmer, or avoiding the subject. This is solved by C and C++ programmers writing sophisticated SIMD libraries that opaquely handle the hardware details, providing a higher level abstraction that other C and C++ programmers can use. Here, we can do a bit better than that.

For x86 SIMD (an obvious source of compatibility concerns here), AVX512, AVX, and SSE enable the zmm, ymm, and xmm registers, which support the _m512, _m256, and _m128 data types, respectively. While their actual usage in intrinsics is slightly more complex, as far as the System V AMD64 ABI is concerned, these are mostly just bags of that many bits with corresponding alignment. The first 16 registers are aliased for x86 CPUs such that, assuming all of these registers exist, xmm{N} aliases a subset of ymm{N} aliases a subset of zmm{N}, starting at bit 0 for each. AVX512 enables another 16 registers but the rules are slightly more complicated there so I won't go into them. Actually operating on the data as if it was a certain type requires specific features enabled, but that's essentially a concern about the callee and already addressed by existing target feature rules. This soundness hole is instead about what happens at the moment the caller picks up the phone, having filled a vector with bits.

Given #81931, #35093, and other issues that basically amount to "the compiler is not generating efficient or correct code because it is not really aware of the capabilities of the target in a meaningful fashion, even though it is functionally required to use a target as an argument to compilation", I agree generically with the plan outlined here and think that the best thing to do is to teach the compiler about the reality of what it is staring down, which will allow it to both recognize ABI compatibilities and emit more efficient code.

Isn't calling a function with additional enabled target_features unsafe? It seems like crossing -C target-feature bounderies in the same case, should likewise be unsafe, or, at the very least, warned against (possibly a future compat warning).
If the information to track target features is available, it should also be possible to enable the correct ABI for the safe case.

In either case, the safe version is still unsound, because you've just passed into code that enables previously disabled target features. If it is not the case that they are available, then it would trap (or result in UB), just like it would if the function in the second crate was explicitly #[target_feature(enable="sse")].

it's not a matter of what is allowed by the CPU, an ABI mismatch will cause problems even if the CPU supports the target feature.

So the root of the problem is that you can't check at runtime if the function you're calling was compiled with non-default target features assumed or not.

For extern "Rust" the abi stays the same independent of the enabled target features. Vector types are forced to be passed by-ref to avoid abi incompatibility:

rust/compiler/rustc_middle/src/ty/layout.rs

Lines 2881 to 2906 in d04c3aa

    
           // This is a fun case! The gist of what this is doing is 
        
           // that we want callers and callees to always agree on the 
        
           // ABI of how they pass SIMD arguments. If we were to *not* 
        
           // make these arguments indirect then they'd be immediates 
        
           // in LLVM, which means that they'd used whatever the 
        
           // appropriate ABI is for the callee and the caller. That 
        
           // means, for example, if the caller doesn't have AVX 
        
           // enabled but the callee does, then passing an AVX argument 
        
           // across this boundary would cause corrupt data to show up. 
        
           // 
        
           // This problem is fixed by unconditionally passing SIMD 
        
           // arguments through memory between callers and callees 
        
           // which should get them all to agree on ABI regardless of 
        
           // target feature sets. Some more information about this 
        
           // issue can be found in #44367. 
        
           // 
        
           // Note that the platform intrinsic ABI is exempt here as 
        
           // that's how we connect up to LLVM and it's unstable 
        
           // anyway, we control all calls to it in libstd. 
        
           Abi::Vector { .. } 
        
               if abi != SpecAbi::PlatformIntrinsic 
        
                   && cx.tcx().sess.target.simd_types_indirect => 
        
           { 
        
               arg.make_indirect(); 
        
               return; 
        
           }

So the root of the problem is that you can't check at runtime if the function you're calling was compiled with non-default target features assumed or not

Compilers should be able to detect this (using the same, or a similar mechanism to the check for #[target_feature]), then require unsafe (and, if necessary, adjust the call abi at the call site). I was considering the same for lccc, which includes a WIP implementation of rust, and it's ABI spec. The solution I chose was to maintain the ABI as-is, use the ABI of the callee, and warn when crossing -C target-features bounderies in ways that affect ABI.
The ABI issue still exists for extern"C" functions, and you're still crossing target_feature bounderies in safe code regardless.

This is fully subsumed by #116344 and #116558, isn't it? I missed the existence of this issue when filing the above ones.

I am confused by one of the examples above:

// crate A: compiled without -C target-feature
pub fn foo(x: f32) -> f32 { x }

// crate B: compiled with -C target-feature=+sse
// this is now safe Rust code:
assert_eq!(A::foo(42.0), 42.0) }; // UB

AFAIK we use the same ABI for f32 no matter which target features are available?

@gnzlbg

I am also curious about this claim:

This is an issue, because when RUSTFLAGS="-C target-cpu=native" is used, not all crates in the dependency graph are compiled with those features. In particular, libstd and its dependencies are not recompiled at all, so their "Rust" ABI might be different than what the rest of the dependency graph uses.

I don't think that's true today. The Rust ABI does not depend on target features that would be (un)set by -Ctarget-cpu. (No idea whether it was true 5 years ago.)
@gnzlbg @chorman0773 @workingjubilee would you agree?

floats are not as carefully compiled as vector types and could be susceptible to this. -C target-cpu=native in particular wouldn't make a difference in that though (unless someone was actually compiling on a pentium pro).

floats depend only on the x87 feature. Even the Pentium Pro has that one. So I think floats should be fine?

I seem to recall there being a desire to make it use sse for extern "Rust" on p4 targets. So that could cause problems if -C target-cpu=native isn't properly checked, but only really in the circumstance where someone is building using an i686 target and -C target-cpu=native on a pentium pro.

Yes if we implement some of the future possibilities mentioned in rust-lang/compiler-team#808, this would become an issue. Hence implementing that is blocked on preventing the issue. But the question is, do we have a problem today?

Not to my knowledge, though I confess I am not entirely familiar with particular ABI choices made by rustc for extern "Rust".

@gnzlbg

After talking with @gnzlbg I came to the conclusion that everything which is still relevant today is tracked by #116344 and #116558, so this issue can be closed. A bummer that it took a few years before any actual movement on this soundness issue, but better late than never.

The other part of this is #125033 and generalizations of it: we don't limit -C target-cpu to enabling only the features that that CPU is guaranteed to support; e.g. -C target_cpu=cortex-a72 enables features that are optional on Cortex-A72 CPUs. I think we should still have an issue that tracks that general problem.

That seems like a bug in the specific implementation of -C target-cpu (which is mostly handled by LLVM), which is quite different from "ABI unsoundness when putting together crates built with different flags". I don't see a relation to this issue, this is not a grab-bag of "everything wrong with -Ctarget-cpu" ;) .

Which issue does track making -Ctarget=cpu sound?

You mentioned #125033 (which is more a missing-docs issue than a soundness issue IMO), which other problems are there?

which other problems are there?

We don't know, because we don't have a system in place to ensure -C target-cpu is sound; we just assume (wrongly) that LLVM does it for us.

I am not sure what there is to track then... the unsoundness (if one wants to call it that) consists in the user and LLVM having different expectations for what a given CPU name means, so the most actionable thing I cam imagine here is better documentation? Or maybe the pipe dream of having our own CPU model database, but that sounds like a nightmare to maintain.

Anyway this issue here was already tracking none of that, it was tracking ABI issues. If you think there's a problem here that needs tracking, please file an issue.

Navigation Menu

Repository navigation

-C target-feature/-C target-cpu are unsound #64609

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// This is a fun case! The gist of what this is doing is
	// that we want callers and callees to always agree on the
	// ABI of how they pass SIMD arguments. If we were to not
	// make these arguments indirect then they'd be immediates
	// in LLVM, which means that they'd used whatever the
	// appropriate ABI is for the callee and the caller. That
	// means, for example, if the caller doesn't have AVX
	// enabled but the callee does, then passing an AVX argument
	// across this boundary would cause corrupt data to show up.
	//
	// This problem is fixed by unconditionally passing SIMD
	// arguments through memory between callers and callees
	// which should get them all to agree on ABI regardless of
	// target feature sets. Some more information about this
	// issue can be found in #44367.
	//
	// Note that the platform intrinsic ABI is exempt here as
	// that's how we connect up to LLVM and it's unstable
	// anyway, we control all calls to it in libstd.
	Abi::Vector { .. }
	if abi != SpecAbi::PlatformIntrinsic
	&& cx.tcx().sess.target.simd_types_indirect =>
	{
	arg.make_indirect();
	return;
	}

-C target-feature/-C target-cpu are unsound #64609

Description

Activity

Dylan-DPC-zz commented on Jun 14, 2020

elichai commented on Sep 12, 2020

workingjubilee commented on Feb 18, 2021

workingjubilee commented on Feb 19, 2021

chorman0773 commented on Mar 22, 2021

Lokathor commented on Mar 22, 2021

bjorn3 commented on Mar 22, 2021

chorman0773 commented on Mar 22, 2021

10 remaining items

RalfJung commented on Nov 23, 2024

RalfJung commented on Nov 24, 2024

chorman0773 commented on Nov 24, 2024

RalfJung commented on Nov 24, 2024

chorman0773 commented on Nov 24, 2024

RalfJung commented on Nov 24, 2024

chorman0773 commented on Nov 24, 2024

RalfJung commented on Feb 3, 2025

briansmith commented on Feb 3, 2025

RalfJung commented on Feb 3, 2025

briansmith commented on Feb 3, 2025

RalfJung commented on Feb 3, 2025

briansmith commented on Feb 3, 2025

RalfJung commented on Feb 3, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions