check for simd type before calling simd_extract in const eval #94441

cameron1024 · 2022-02-28T07:00:57Z

This is my first PR in this part of the codebase, so happy to change things as others see fit. I'm not super familiar with SIMD in particular but I'm assuming that a type shouldn't be eligible for SIMD platform intrinsics unless explicitly opted into with #[repr(simd)].

Like I say I'm pretty unsure about whether this is the right direction, but thought it can't hurt to try to fix it and see what other people say about it 😁

rust-highfive · 2022-02-28T07:01:00Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-02-28T07:01:01Z

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

rust-log-analyzer · 2022-02-28T07:19:16Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


---- [ui] ui/simd/issue-94382.rs stdout ----
diff of stderr:

6 LL | |
7 LL | | struct U16x2(u16, u16);
8 ...  |
- LL | |     const Y0: i8 = unsafe { simd_extract(V, 0) };  
+ LL | |     const Y0: i8 = unsafe { simd_extract(V, 0) };
10 LL | | }
12 

13 error: any use of this value will cause an error
14   --> $DIR/issue-94382.rs:14:29
14   --> $DIR/issue-94382.rs:14:29
15    |
- LL |     const Y0: i8 = unsafe { simd_extract(V, 0) };  
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
+ LL |     const Y0: i8 = unsafe { simd_extract(V, 0) };
18    |                             |
18    |                             |
19    |                             aborted execution: type `U16x2` is not `#[repr(simd)]`

The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/simd/issue-94382/issue-94382.stderr
To update references, rerun the tests and pass the `--bless` flag
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args simd/issue-94382.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/simd/issue-94382.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/simd/issue-94382" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/simd/issue-94382/auxiliary"
stdout: none
--- stderr -------------------------------
error: module has missing stability attribute
   |
   |
LL | / #![feature(platform_intrinsics)] //~ ERROR module has missing stability attribute
LL | | #![feature(staged_api)]
LL | |
LL | | struct U16x2(u16, u16);
...  |
LL | |     const Y0: i8 = unsafe { simd_extract(V, 0) };
LL | | }

error: any use of this value will cause an error
  --> /checkout/src/test/ui/simd/issue-94382.rs:14:29
   |
   |
LL |     const Y0: i8 = unsafe { simd_extract(V, 0) };
   |                             |
   |                             |
   |                             aborted execution: type `U16x2` is not `#[repr(simd)]`
   |
   = note: `#[deny(const_err)]` on by default
   = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

error: aborting due to 2 previous errors
------------------------------------------

oli-obk

That all said, it's really cheap to switch from an assert to a UB message, so I am somewhat inclined to just do that

oli-obk · 2022-02-28T07:24:30Z

src/test/ui/simd/issue-94382.rs

@@ -0,0 +1,17 @@
+#![feature(platform_intrinsics)] //~ ERROR module has missing stability attribute
+#![feature(staged_api)]


I don't think the staged_api feature is needed for the test

I tried removing this, it changes the test to only give an error on #[rustc_const_stable(...)] saying it can't be used outside stdlib, is this expected?

Oops, yea, that is kinda expected. Removing that attribute, too, should work in this test I think.

oli-obk · 2022-02-28T07:46:56Z

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -444,7 +444,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
    ) -> InterpResult<'tcx, (MPlaceTy<'tcx, M::PointerTag>, u64)> {
        // Basically we just transmute this place into an array following simd_size_and_type.
        // This only works in memory, but repr(simd) types should never be immediates anyway.
-        assert!(base.layout.ty.is_simd());
+        assert!(


I think if we change anything, we should change this assert into an if and a throw_ub_format. I'm not sure we should change anything though. @workingjubilee is it ever expected that we'll allow calling SIMD intrinsics from stable code?

Note that codegen accepts this call silently and likely with UB. If we do any fix for intrinsics, we should probably look into more general solutions, but previously we've decided to just keep ICEing on intrinsic mis-use as it's an infinite rabbit hole otherwise.

Just double checking I'm understanding right, you're saying that because extern "platform-intrinsics" is likely going to be permanently unstable, an error like this would only be hit by another bug in the compiler, or by nightly code, so the difference between an ICE and a proper error message isn't as significant as for stable code?
But because swapping out an assert for throw_ub_format is a super easy change, it's still marginally better? Or am I misunderstanding?

No misunderstanding, that's exactly what I meant. Of course this is just parrotting old information with some sprinkles of convenience on top. So I'm perfectly fine reevaluating this if you feel otherwise. We should also let @RalfJung chime in before doing anything, as this may get into a game of whack-a-mole if we keep removing such ICEs.

I don't have strong objections to replacing this assert by a throw -- it doesn't make the code harder to read or maintain. This should not give rise to the expectation that all ICEs due to misused intrinsics are bugs though, I don't want to see the codebase littered with such checks.

throw_ub seems wrong though; an "invalid program" error seems more accurate. We don't have throw_invalid_format yet, but that should not be too hard to add.

I think if we change anything, we should change this assert into an if and a throw_ub_format. I'm not sure we should change anything though. @workingjubilee is it ever expected that we'll allow calling SIMD intrinsics from stable code?

No, it is never intended that users will touch these, and we are at least somewhat considering if we can successfully deprecate #[repr(simd)] as a concept entirely (as it is also unstable) and just have it be the suite of stable core::simd types.

The era of them being "platform intrinsics" came from when there was an intention that core::simd and core::arch would be a lot closer in design and integration, and that it might be possible to write something like portable-simd as a user crate using them. That was 7 years ago, and since then core::arch has diverged towards simply recapitulating the existing intrinsics instead, and using more LLVM-particular bits and pieces.

Even if we ever do stabilize these functions, they would be, at best, unsafe functions for advanced users, "please only touch these if you would really like to blow your entire foot open".

Oh and if we change this, mplace_to_simd should probably throw the same error.

(operand_to_simd still needs its own check as otherwise assert_mem_place could ICE.)

oli-obk · 2022-03-02T08:46:38Z

Thanks for the PR, it spawned a really good discussion. I am going to close this (and the issue), since it seems like we will never allow users direct access to those intrinsics. ICEing on mis-use of regular intrinsics is already fairly easy, and the same is true for platform intrinsics.

cameron1024 · 2022-03-02T11:08:57Z

No problem 😁 I guess as a newer contributor, I previously assumed "all ICEs should be fixed", and was just filtering issues by the ICE label to find something to pick up. Is it worth documenting somewhere (maybe rustc-dev-guide) that some ICEs don't need fixing (and maybe how to know if that's the case)? Apologies if this is something obvious to more experienced people

oli-obk · 2022-03-02T12:19:17Z

😆 it's not really obvious. I guess the bar is "can someone on stable hit the ICE or can the ICE be hit with a feature that we expect to stabilize". Not sure where to put that information, is there somewhere in the rustc-dev-guide that you would have noticed this information before looking for ICEs?

RalfJung · 2022-03-02T17:13:58Z

Basically if reproducing an ICE needs the intrinsics or platform_intrinsics feature, then chances are high this is one of those ICEs that we don't necessarily want to fix.

This is certainly something that should be documented better. Do the ICE-breakers have documentation that this would be good to add to?

check for simd type before calling simd_extract

37d04aa

rust-highfive assigned wesleywiser Feb 28, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2022

oli-obk reviewed Feb 28, 2022

View reviewed changes

oli-obk closed this Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check for simd type before calling simd_extract in const eval #94441

check for simd type before calling simd_extract in const eval #94441

cameron1024 commented Feb 28, 2022

rust-highfive commented Feb 28, 2022

rust-highfive commented Feb 28, 2022

rust-log-analyzer commented Feb 28, 2022

oli-obk left a comment

oli-obk Feb 28, 2022

cameron1024 Feb 28, 2022

oli-obk Feb 28, 2022

oli-obk Feb 28, 2022

oli-obk Feb 28, 2022

cameron1024 Feb 28, 2022

oli-obk Feb 28, 2022

RalfJung Mar 2, 2022

workingjubilee Mar 2, 2022

workingjubilee Mar 2, 2022 •

edited

Loading

RalfJung Mar 2, 2022

oli-obk commented Mar 2, 2022

cameron1024 commented Mar 2, 2022

oli-obk commented Mar 2, 2022

RalfJung commented Mar 2, 2022

		@@ -0,0 +1,17 @@
		#![feature(platform_intrinsics)] //~ ERROR module has missing stability attribute
		#![feature(staged_api)]

check for simd type before calling simd_extract in const eval #94441

check for simd type before calling simd_extract in const eval #94441

Conversation

cameron1024 commented Feb 28, 2022

rust-highfive commented Feb 28, 2022

rust-highfive commented Feb 28, 2022

rust-log-analyzer commented Feb 28, 2022

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 2, 2022

cameron1024 commented Mar 2, 2022

oli-obk commented Mar 2, 2022

RalfJung commented Mar 2, 2022

workingjubilee Mar 2, 2022 •

edited

Loading