wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types #117351

RalfJung · 2023-10-29T11:21:19Z

I do not know what I am doing, I have not touched this code before at all. We do get a bunch of extra diagnostics since we now check every occurrence of a fn ptr type for whether it makes some basic sense, whereas before we only complained when the fn ptr was being called. I think there's probably a way to now remove some of the sizedness check on the HIR side and rely on WF instead, but I couldn't figure it out. This fixes an ICE so maybe it's okay if the diagnostics aren't great from the start.

This is a breaking change if anyone uses the type fn() -> str or extern "C" fn(str), or similar bogus function types, anywhere. We should probably crater this.

This does not reject the use of the type fn(str) on stable, (a) to reduce the amount of breakage, (b) since I don't know if WF-checking can depend on feature flags without causing havoc since a crate with less features might see types from a crate with more features that are suddenly not WF any more, and (c) since it's not required to ensure basic sanity of the ABI. This PR introduces an assertion in our ABI computation logic which checks that the computed ABI makes sense, and for extern "C" fn(str) there is no sensible ABI, and we do compute the ABI even for function pointers we never call, so we need to start rejecting that code. fn(str) has a sensible ABI, we just don't make it available on stable.

Fixes #115845

rustbot · 2023-10-29T11:21:27Z

r? @wesleywiser

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

rustbot · 2023-10-29T11:21:30Z

These commits modify compiler targets.
(See the Target Tier Policy.)

RalfJung · 2023-10-29T11:22:32Z

compiler/rustc_target/src/abi/call/x86_64.rs

@@ -153,9 +153,9 @@ fn reg_component(cls: &[Option<Class>], i: &mut usize, size: Size) -> Option<Reg
    }
 }

-fn cast_target(cls: &[Option<Class>], size: Size) -> Option<CastTarget> {


Here we revert eddfce5, which is no longer needed since we now reject the problematic type earlier.

RalfJung · 2023-10-29T11:22:58Z

tests/ui/associated-types/associated-types-unsized.rs

@@ -1,4 +1,3 @@
-// run-rustfix


rustfix now adds where <T as Get>::Value: Sized twice which obviously doesn't work...

RalfJung · 2023-10-29T11:29:28Z

tests/ui/abi/compatibility.rs

+/* revisions: nvptx64
+  [nvptx64] compile-flags: --target nvptx64-nvidia-cuda
+  [nvptx64] needs-llvm-components: nvptx
+*/


@kjetilkjeka I had to disable this test on nvptx64 since the extern "C" ABI uses Direct pass mode in invalid ways. I think that's caused by this code:

rust/compiler/rustc_target/src/abi/call/nvptx64.rs

Lines 10 to 14 in b75b3b3

fn classify_arg<Ty>(arg: &mut ArgAbi<'_, Ty>) {

if arg.layout.is_aggregate() && arg.layout.size.bits() > 64 {

arg.make_indirect();

}

}

That's invalid since it doesn't say what to do for smaller aggregates, and they default to the (bad) Direct. Instead you have to say which register they are supposed to be passed in. You can check what the other targets are doing. Targets are expected to set an explicit pass mode for all aggregate arguments.

This should be easy to reproduce by having a function like

#[repr(C)] struct ReprC1<T: ?Sized>(T); extern "C" fn myfn(x: ReprC1<i32>) {}

which will likely ICE on the current compiler already.

RalfJung · 2023-10-29T12:35:52Z

@bors try

bors · 2023-10-29T12:37:01Z

⌛ Trying commit 1d0b64e with merge ac89e27...

wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types I do *not* know what I am doing, I have not touched this code before at all. We do get a bunch of extra diagnostics since we now check every occurrence of a fn ptr type for whether it makes some basic sense, whereas before we only complained when the fn ptr was being called. I think there's probably a way to now remove some of the sizedness check on the HIR side and rely on WF instead, but I couldn't figure it out. This fixes an ICE so maybe it's okay if the diagnostics aren't great from the start. This is a breaking change if anyone uses the type `fn() -> str` or `extern "C" fn(str)`, or similar bogus function types, anywhere. We should probably crater this. This does *not* reject the use of the type `fn(str)` on stable, (a) to reduce the amount of breakage, (b) since I don't know if WF-checking can depend on feature flags without causing havoc since a crate with less features might see types from a crate with more features that are suddenly not WF any more, and (c) since it's not required to ensure basic sanity of the ABI. This PR introduces an assertion in our ABI computation logic which checks that the computed ABI makes sense, and for `extern "C" fn(str)` there is no sensible ABI, and we *do* compute the ABI even for function pointers we never call, so we need to start rejecting that code. `fn(str)` has a sensible ABI, we just don't make it available on stable. Fixes rust-lang#115845

… has sized return types

bors · 2023-10-29T13:59:10Z

☀️ Try build successful - checks-actions
Build commit: ac89e27 (ac89e27721b9b4d1482ebc87f2f83b4e23d49d71)

RalfJung · 2023-10-29T14:03:39Z

@craterbot check

craterbot · 2023-10-29T14:03:48Z

👌 Experiment pr-117351 created and queued.
🤖 Automatically detected try build ac89e27
⚠️ Try build based on commit 1d0b64e, but latest commit is 5e6e31a. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2023-10-29T19:16:37Z

This needs an FCP because like you said, it breaks code like:

fn foo() -> Option<fn() -> str> { None }

craterbot · 2023-10-30T03:26:01Z

🚧 Experiment pr-117351 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2023-10-31T17:09:08Z

🎉 Experiment pr-117351 is completed!
📊 1786 regressed and 3 fixed (383106 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

RalfJung · 2023-11-01T06:47:43Z

Oh, oh no. This is completely busted. :( It breaks even crossbeam-skiplist, which uses PhantomData<fn() -> Q> on an unsized type.

Well that's bad, we can add this to the list of Rust 1.0 regrets. :/

Urgau · 2023-11-01T15:15:09Z

While there are many regressions, the list of affected crates seems small enough to at least have a future incompatibility lint, and maybe after some time we could turn the lint into a hard-error with compatibility hacks for unpatched crates.

RalfJung · 2023-11-01T16:25:51Z

Oh, most of these fail because a dependency failed.

Maybe... but I am not sure if we can ever get away with breaking this. We could delay the check until the ABI is actually computed, but that'd be a post-mono error, which are annoying both implementation-wise and from a user perspective.

RalfJung · 2023-11-01T22:10:23Z

Superseded by #117500.

Ensure sanity of all computed ABIs This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`. So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument. To achieve this, this re-lands the parts of rust-lang#80594 that got reverted in rust-lang#81388. To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default! Cc `@bjorn3` Fixes rust-lang#115845

Ensure sanity of all computed ABIs This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang/rust#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`. So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument. To achieve this, this re-lands the parts of rust-lang/rust#80594 that got reverted in rust-lang/rust#81388. To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default! Cc `@bjorn3` Fixes rust-lang/rust#115845

rustbot assigned wesleywiser Oct 29, 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 Oct 29, 2023

RalfJung commented Oct 29, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the fn-ptr-sized-wf branch from 9183407 to f0bc6cc Compare October 29, 2023 12:25

This comment has been minimized.

Sign in to view

RalfJung force-pushed the fn-ptr-sized-wf branch from f0bc6cc to 1d0b64e Compare October 29, 2023 12:33

This comment has been minimized.

Sign in to view

RalfJung added 2 commits October 29, 2023 14:28

wf: ensure that non-Rust-functions have sized arguments, and everyone…

86e717d

… has sized return types

move ABI sanity check from LLVM codegen backend to ABI computation logic

5e6e31a

RalfJung force-pushed the fn-ptr-sized-wf branch from 1d0b64e to 5e6e31a Compare October 29, 2023 14:03

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2023

compiler-errors added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Oct 29, 2023

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 31, 2023

kjetilkjeka mentioned this pull request Nov 1, 2023

nvptx abi adjustment may keep PassMode::Direct for small aggregates in conv extern "C" #117480

Open

RalfJung mentioned this pull request Nov 1, 2023

Ensure sanity of all computed ABIs #117500

Merged

RalfJung closed this Nov 1, 2023

RalfJung deleted the fn-ptr-sized-wf branch November 1, 2023 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types #117351

wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types #117351

RalfJung commented Oct 29, 2023 •

edited

Loading

rustbot commented Oct 29, 2023

rustbot commented Oct 29, 2023

RalfJung Oct 29, 2023

RalfJung Oct 29, 2023

RalfJung Oct 29, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

RalfJung commented Oct 29, 2023

bors commented Oct 29, 2023

This comment has been minimized.

bors commented Oct 29, 2023

RalfJung commented Oct 29, 2023

craterbot commented Oct 29, 2023

compiler-errors commented Oct 29, 2023

craterbot commented Oct 30, 2023

craterbot commented Oct 31, 2023

RalfJung commented Nov 1, 2023 •

edited

Loading

Urgau commented Nov 1, 2023

RalfJung commented Nov 1, 2023

RalfJung commented Nov 1, 2023

	fn classify_arg<Ty>(arg: &mut ArgAbi<'_, Ty>) {
	if arg.layout.is_aggregate() && arg.layout.size.bits() > 64 {
	arg.make_indirect();
	}
	}

wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types #117351

wf: ensure that non-Rust-functions have sized arguments, and everyone has sized return types #117351

Conversation

RalfJung commented Oct 29, 2023 • edited Loading

rustbot commented Oct 29, 2023

rustbot commented Oct 29, 2023

RalfJung Oct 29, 2023

Choose a reason for hiding this comment

RalfJung Oct 29, 2023

Choose a reason for hiding this comment

RalfJung Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

RalfJung commented Oct 29, 2023

bors commented Oct 29, 2023

This comment has been minimized.

bors commented Oct 29, 2023

RalfJung commented Oct 29, 2023

craterbot commented Oct 29, 2023

compiler-errors commented Oct 29, 2023

craterbot commented Oct 30, 2023

craterbot commented Oct 31, 2023

RalfJung commented Nov 1, 2023 • edited Loading

Urgau commented Nov 1, 2023

RalfJung commented Nov 1, 2023

RalfJung commented Nov 1, 2023

RalfJung commented Oct 29, 2023 •

edited

Loading

RalfJung Oct 29, 2023 •

edited

Loading

RalfJung commented Nov 1, 2023 •

edited

Loading