miri: fail when calling a function that requires an unavailable target feature #113720

eduardosm · 2023-07-15T11:00:24Z

miri will report an UB when calling a function that has a #[target_feature(enable = ...)] attribute is called and the required feature is not available.

"Available features" are the same that is_x86_feature_detected! (or equivalent) reports to be available during miri execution (which can be enabled or disabled with the -C target-feature flag).

rustbot · 2023-07-15T11:00:32Z

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-07-15T11:00:35Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

bjorn3 · 2023-07-15T14:55:31Z

#113684 accidentally got merged due to a glitch in bors. We had to force push it away. It seems that you based this PR on the state of the repo when that PR was still merged. Could you please rebase and drop these commits?

eduardosm · 2023-07-15T16:08:37Z

#113684 accidentally got merged due to a glitch in bors. We had to force push it away. It seems that you based this PR on the state of the repo when that PR was still merged. Could you please rebase and drop these commits?

Done. Out of curiosity, why force pushing away those commits instead of reverting them?

oli-obk · 2023-07-15T18:14:09Z

We could check this during const eval, too.

On the miri side: are you worried this could cause real code to fail under miri, so you added an opt out flag for them to keep their miri tests passing and having time to fix their UB?

eduardosm · 2023-07-15T20:24:35Z

We could check this during const eval, too.

I agree that it would make sense, but wouldn't it be a breaking change?

On the miri side: are you worried this could cause real code to fail under miri, so you added an opt out flag for them to keep their miri tests passing and having time to fix their UB?

I mainly added it because there are already flags to disable other soundness checks, so I thought we would want one for this too.

oli-obk · 2023-07-15T20:48:54Z

but wouldn't it be a breaking change?

It's very unlikely anyone created const fns that also have target features. It's not like you could use the target features. And it's UB that you are catching, so we are allowed to break it.

Wrt the flag, i'll let the other miri folk chime in on this, but I don't see the point of a flag for this one.

saethlin · 2023-07-15T21:53:57Z

I agree, we don't need a flag here. If someone complains that the check is annoying and they'd like to turn it off we can add one but I'd prefer we not add features (especially unsound ones), unless there's a well-known use case or a request for it. (I would actually prefer that we lose some of these -Zmiri-disable-X checks that we already have)

…t feature miri will report an UB when calling a function that has a `#[target_feature(enable = ...)]` attribute is called and the required feature is not available. "Available features" are the same that `is_x86_feature_detected!` (or equivalent) reports to be available during miri execution (which can be enabled or disabled with the `-C target-feature` flag).

eduardosm · 2023-07-15T22:27:39Z

Removing the flag and checking unconditionally, then.

oli-obk · 2023-07-16T06:10:16Z

Please also add a test of a sound and an unsound usage during const eval

eduardosm · 2023-07-16T10:49:45Z

I am having trouble running compiletests locally.

---- [ui] tests/ui/higher-ranked/trait-bounds/issue-62203-hrtb-ice.rs stdout ----
diff of stderr:

44      LL | |         },
45         | |_________^ expected `Unit3`, found `Unit4`
46         |
-       note: required for `L<[closure@$DIR/issue-62203-hrtb-ice.rs:42:16: 42:19]>` to implement `for<'r> T0<'r, (&'r u8,)>`
+       note: required for `L<[[email protected]:42:16]>` to implement `for<'r> T0<'r, (&'r u8,)>`
48        --> $DIR/issue-62203-hrtb-ice.rs:17:16
49         |
50      LL | impl<'a, A, T> T0<'a, A> for L<T>

52      LL | where
53      LL |     T: FnMut(A) -> Unit3,
54         |                    ----- unsatisfied trait bound introduced here
+          = note: the full type name has been written to '$TEST_BUILD_DIR/higher-ranked/trait-bounds/issue-62203-hrtb-ice/issue-62203-hrtb-ice.long-type-7638616413998691750.txt'
55      note: required by a bound in `T1::m`
56        --> $DIR/issue-62203-hrtb-ice.rs:27:12
57         |


The actual stderr differed from the expected stderr.

There is some $DIR in the stderr file that does not show in the actual output. Also the note note: the full type name has been written to.

eduardosm · 2023-07-16T11:21:55Z

I added the tests and pushed without running the full test suite.

RalfJung

This looks great, thanks. :) Just some nits.

compiler/rustc_const_eval/src/interpret/terminator.rs

RalfJung · 2023-07-16T11:37:19Z

compiler/rustc_const_eval/src/interpret/terminator.rs

+                    .iter()
+                    .copied()
+                    .filter(|feature| !self.tcx.sess.target_features.contains(feature))
+                    .collect();


I get measurable perf changes when avoiding allocations in this function. So we should not create a new Vec here each time. Why do we need to manifest this list anyway? We just need to iterate over it and check if any of them is not in target_features.

I did not expect such impact from collecting into an empty Vec, so I will avoid it.

compiler/rustc_const_eval/src/interpret/terminator.rs

* Split into its own function * Do not build a `Vec` of unavailable features

RalfJung · 2023-07-16T15:45:02Z

@bors try @rust-timer queue

bors · 2023-07-16T15:45:12Z

⌛ Trying commit ae2a72d with merge 3ecfbdaab0a5f6c3fefde8060dde9ddd54f92a18...

bors · 2023-07-16T17:01:07Z

☀️ Try build successful - checks-actions
Build commit: 3ecfbdaab0a5f6c3fefde8060dde9ddd54f92a18 (3ecfbdaab0a5f6c3fefde8060dde9ddd54f92a18)

rust-timer · 2023-07-16T20:43:17Z

Finished benchmarking commit (3ecfbdaab0a5f6c3fefde8060dde9ddd54f92a18): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.6%, 1.2%]	10
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[0.6%, 1.2%]	10

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.0%, 2.2%]	2
Regressions ❌ (secondary)	2.4%	[1.4%, 5.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.0%, 2.2%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 658.073s -> 659.039s (0.15%)

oli-obk · 2023-07-17T13:29:16Z

@bors r=RalfJung,oli-obk

bors · 2023-07-17T13:29:19Z

📌 Commit ae2a72d has been approved by RalfJung,oli-obk

It is now in the queue for this repository.

bors · 2023-07-17T19:57:41Z

⌛ Testing commit ae2a72d with merge 1787f31...

bors · 2023-07-17T21:51:38Z

☀️ Test successful - checks-actions
Approved by: RalfJung,oli-obk
Pushing 1787f31 to master...

rust-timer · 2023-07-18T02:49:26Z

Finished benchmarking commit (1787f31): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.5%, 3.0%]	2
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.794s -> 657.068s (-0.11%)

rustbot assigned fee1-dead Jul 15, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 15, 2023

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the master branch from bef6ff6 to ad96323 Compare July 15, 2023 13:39

This comment has been minimized.

Sign in to view

Add const-eval test for #[target_feature(enable = ...)] function calls

3e8ce42

RalfJung reviewed Jul 16, 2023

View reviewed changes

Refactor checking function target features during const-eval

ae2a72d

* Split into its own function * Do not build a `Vec` of unavailable features

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 16, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 16, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 17, 2023

bors merged commit 1787f31 into rust-lang:master Jul 17, 2023

rustbot added this to the 1.73.0 milestone Jul 17, 2023

rustbot removed the perf-regression Performance regression. label Jul 18, 2023

eduardosm deleted the miri-target-feature branch July 18, 2023 15:52

ehuss mentioned this pull request Oct 7, 2023

#[target_feature(enable = "simd128")] does not work properly on 1.73, but work fine on 1.72.1 #116516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

miri: fail when calling a function that requires an unavailable target feature #113720

miri: fail when calling a function that requires an unavailable target feature #113720

eduardosm commented Jul 15, 2023

rustbot commented Jul 15, 2023

rustbot commented Jul 15, 2023

This comment has been minimized.

bjorn3 commented Jul 15, 2023

This comment has been minimized.

eduardosm commented Jul 15, 2023

oli-obk commented Jul 15, 2023

eduardosm commented Jul 15, 2023

oli-obk commented Jul 15, 2023

saethlin commented Jul 15, 2023

eduardosm commented Jul 15, 2023

oli-obk commented Jul 16, 2023

eduardosm commented Jul 16, 2023

eduardosm commented Jul 16, 2023

RalfJung left a comment •

edited

Loading

RalfJung Jul 16, 2023

eduardosm Jul 16, 2023

RalfJung commented Jul 16, 2023

This comment has been minimized.

bors commented Jul 16, 2023

bors commented Jul 16, 2023

This comment has been minimized.

rust-timer commented Jul 16, 2023

oli-obk commented Jul 17, 2023

bors commented Jul 17, 2023

bors commented Jul 17, 2023

bors commented Jul 17, 2023

rust-timer commented Jul 18, 2023

miri: fail when calling a function that requires an unavailable target feature #113720

miri: fail when calling a function that requires an unavailable target feature #113720

Conversation

eduardosm commented Jul 15, 2023

rustbot commented Jul 15, 2023

rustbot commented Jul 15, 2023

This comment has been minimized.

bjorn3 commented Jul 15, 2023

This comment has been minimized.

eduardosm commented Jul 15, 2023

oli-obk commented Jul 15, 2023

eduardosm commented Jul 15, 2023

oli-obk commented Jul 15, 2023

saethlin commented Jul 15, 2023

eduardosm commented Jul 15, 2023

oli-obk commented Jul 16, 2023

eduardosm commented Jul 16, 2023

eduardosm commented Jul 16, 2023

RalfJung left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 16, 2023

This comment has been minimized.

bors commented Jul 16, 2023

bors commented Jul 16, 2023

This comment has been minimized.

rust-timer commented Jul 16, 2023

Overall result: ❌ regressions - ACTION NEEDED

oli-obk commented Jul 17, 2023

bors commented Jul 17, 2023

bors commented Jul 17, 2023

bors commented Jul 17, 2023

rust-timer commented Jul 18, 2023

Overall result: no relevant changes - no action needed

RalfJung left a comment •

edited

Loading