Separate unsized fn param from unsized locals #72029
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
spastorino
force-pushed
the
separate-unsized-locals
branch
from
1f81b5c
to
c072dc1
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
spastorino
force-pushed
the
separate-unsized-locals
branch
from
c072dc1
to
8a0d3f6
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
reviewed
May 9, 2020
RalfJung
reviewed
May 9, 2020
| @@ -1,4 +1,5 @@ | |||
| #![feature(unsized_locals)] | |||
| //~^ WARN the feature `unsized_locals` is incomplete and may cause the compiler to crash | |||
There was a problem hiding this comment.
That message doesn't really capture "or produce wrong code".
I am changing the message to something more appropriate for cases like this in #71420, but that PR is seemingly stuck unfortunately.
There was a problem hiding this comment.
I'll make a PR with just the message change.
|
Ideally we'd have a sanity check in codegen to make sure we only reach the "dynamic alloca" path (confusingly called "array alloca" in codegen and not to be confused with But I think we can get there when the feature was enabled anywhere in the crate tree, so... not sure if there is a good way to do that. |
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
May 11, 2020
…ochenkov Incomplete features can also be unsound Some incomplete features do not just ICE, they are also currently unsound (e.g. rust-lang#72029, and also `specialization` -- which is not yet marked incomplete but [should be](rust-lang#71420)). This makes the message reflect that. While at it I also added a link to the tracking issue, which hopefully should explain what is incomplete/unsound about the feature.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
May 14, 2020
…ochenkov Incomplete features can also be unsound Some incomplete features do not just ICE, they are also currently unsound (e.g. rust-lang#72029, and also `specialization` -- which is not yet marked incomplete but [should be](rust-lang#71420)). This makes the message reflect that. While at it I also added a link to the tracking issue, which hopefully should explain what is incomplete/unsound about the feature.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
May 16, 2020
…ochenkov Incomplete features can also be unsound Some incomplete features do not just ICE, they are also currently unsound (e.g. rust-lang#72029, and also `specialization` -- which is not yet marked incomplete but [should be](rust-lang#71420)). This makes the message reflect that. While at it I also added a link to the tracking issue, which hopefully should explain what is incomplete/unsound about the feature.
|
Tests are failing here because I still didn't add a gate test for the feature #![feature(unsized_locals)]
#![feature(unsized_fn_params)]
use std::any::Any;
#[repr(align(256))]
#[allow(dead_code)]
struct A {
v: u8,
}
impl A {
fn f(&self) -> *const A {
assert_eq!(self as *const A as usize % 256, 0);
self
}
}
fn foo(x: Box<dyn Any>) {
let x = *x;
let dwncst = x.downcast_ref::<A>().unwrap();
let addr = dwncst.f();
assert_eq!(addr as usize % 256, 0);
}
fn main() {
let x = Box::new(A { v: 4 });
foo(x);
}But this seems to be the case described in #71416 that doesn't work with the feature flag added. I'm still unsure why, need to investigate a bit on that. cc @RalfJung |
spastorino
force-pushed
the
separate-unsized-locals
branch
2 times, most recently
from
1a62fe8
to
e8d880a
Compare
|
Just migrated some of the code from #![feature(unsized_fn_params)]
#[repr(align(256))]
#[allow(dead_code)]
struct A {
v: u8,
}
trait Foo {
fn foo(&self);
}
impl Foo for A {
fn foo(&self) {
assert_eq!(self as *const A as usize % 256, 0);
}
}
fn foo(x: dyn Foo) {
x.foo()
}
fn main() {
let x: Box<dyn Foo> = Box::new(A { v: 22 });
foo(*x);
}Gives ... Unsure what else I should have migrated. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
With which feature flag? For the "sound unsized locals" flag, the existing code for And for the "unsound unsized locals" flag, the test in #71416 is the right one -- but that test is different from what you quoted! Having So there should be a test ensuring that even with the sound feature flag, the test in #71416 gets rejected. |
That test looks like it should pass, unsized stuff is only used in argument position so can be passed by reference to the heap. |
|
@RalfJung to clarify a bit ... Right now what the PR tries to do is to split all that is related to #![feature(unsized_fn_params)]
#[repr(align(256))]
#[allow(dead_code)]
struct A {
v: u8,
}
trait Foo {
fn foo(&self);
}
impl Foo for A {
fn foo(&self) {
assert_eq!(self as *const A as usize % 256, 0);
}
}
fn foo(x: dyn Foo) {
x.foo()
}
fn main() {
let x: Box<dyn Foo> = Box::new(A { v: 22 });
foo(*x);
}Which I think should suffice, but getting ... And I currently don't know why exactly. As you can see here, here, here and here, I've migrated existing code to use the new flag but it seems that I'm lacking something because the error I'm getting. That's the current problem I'm having, then I can go back the tidy asking for a test with the feature flag ( |
|
@spastorino wrote:
I skimmed over the branch and noticed a few places in (This is a consequence of the general observation that fn params are a special case of patterns...) (The above note is a summary of commentary I wrote on zulip over here) |
pnkfelix
added
S-waiting-on-author
|
Yeah, looks to me like there are "just" a few places where the code still checks for the wrong feature gate. I think the test case is right. I don't think I can help with this, I have no familiarity with this code. |
RalfJung
reviewed
May 23, 2020
| @@ -1,4 +1,5 @@ | |||
| #![feature(unsized_tuple_coercion, unsized_locals)] | |||
| #![feature(unsized_tuple_coercion, unsized_locals, unsized_fn_params)] | |||
There was a problem hiding this comment.
Why does this need unsized_locals?
There was a problem hiding this comment.
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> src/test/ui/unsized-locals/unsized-exprs.rs:22:26
|
22 | udrop::<(i32, [u8])>((42, *foo()));
| ^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: within `({integer}, [u8])`, the trait `std::marker::Sized` is not implemented for `[u8]`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= note: required because it appears within the type `({integer}, [u8])`
= note: tuples must have a statically known size to be initialized
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> src/test/ui/unsized-locals/unsized-exprs.rs:24:22
|
24 | udrop::<A<[u8]>>(A { 0: *foo() });
| ^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: within `A<[u8]>`, the trait `std::marker::Sized` is not implemented for `[u8]`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= note: required because it appears within the type `A<[u8]>`
= note: structs must have a statically known size to be initialized
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> src/test/ui/unsized-locals/unsized-exprs.rs:26:22
|
26 | udrop::<A<[u8]>>(A(*foo()));
| ^ doesn't have a size known at compile-time
|
= help: within `A<[u8]>`, the trait `std::marker::Sized` is not implemented for `[u8]`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= note: required because it appears within the type `A<[u8]>`
= note: the return type of a function must have a statically known size
error: aborting due to 3 previous errors
For more information about this error, try `rustc --explain E0277`.
There was a problem hiding this comment.
Ah I see, there's an unsized return value?
Seems a bit odd that you would need both feature gates, but maybe that makes sense?
RalfJung
reviewed
May 23, 2020
RalfJung
reviewed
May 23, 2020
| @@ -117,6 +117,7 @@ | |||
| #![feature(track_caller)] | |||
| #![feature(transparent_unions)] | |||
| #![feature(unboxed_closures)] | |||
| #![cfg_attr(not(bootstrap), feature(unsized_fn_params))] | |||
| #![feature(unsized_locals)] | |||
There was a problem hiding this comment.
unsized_locals should not be needed any more, so can you gate this by cfg_attr(bootstrap, ...)?
There was a problem hiding this comment.
If I do this change in libcore I get ...
Building stage1 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
Compiling cc v1.0.52
Compiling core v0.0.0 (/home/santiago/src/oss/rust2/src/libcore)
Compiling libc v0.2.69
Compiling autocfg v0.1.7
Compiling std v0.0.0 (/home/santiago/src/oss/rust2/src/libstd)
Compiling hashbrown v0.6.2
Compiling compiler_builtins v0.1.28
Compiling backtrace-sys v0.1.37
Compiling unwind v0.0.0 (/home/santiago/src/oss/rust2/src/libunwind)
error[E0161]: cannot move a value of type T: the size of T cannot be statically determined
--> src/libcore/mem/mod.rs:162:33
|
162 | unsafe { intrinsics::forget(t) }
| ^
error: aborting due to previous error
For more information about this error, try `rustc --explain E0161`.
error: could not compile `core`.
To learn more, run the command again with --verbose.
command did not execute successfully: "/home/santiago/src/oss/rust2/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "8" "--release" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/home/santiago/src/oss/rust2/src/libtest/Cargo.toml" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
failed to run: /home/santiago/src/oss/rust2/build/bootstrap/debug/bootstrap build --stage 1 src/libstd
Build completed unsuccessfully in 0:02:00
There was a problem hiding this comment.
Something is wrong then. The unsound feature must not be used in libcore/liballoc.
There was a problem hiding this comment.
If I'm not wrong this is the last missing bit to fix but I don't understand it exactly.
Wasn't libcore/liballoc already using that?. Isn't fixing some of this stuff for a different PR? or am I not understanding what you're suggesting?.
There was a problem hiding this comment.
Wasn't libcore/liballoc already using that?
Using what?
libstd/liballoc/libcore (and all of rustc really except for the test suite) should only be using the sound version of the feature. After all, we don't want unsound code in our standard library or the compiler. So clearly unsized_locals should not be enabled. Does this make sense so far?
The claim, back when passing unsized parameters to functions was carved out as a "soundly implementable subset", was that this would suffice for what libstd is doing. So assuming that claim is correct, no further changes should be needed to disable unsized_locals.
However, the bootstrap compiler does not understand unsized_fn_params, so we still have to enable the unsound unsized_locals but only when building the standard library with the bootstrap compiler.
There was a problem hiding this comment.
The error here looks like this is just an unsized parameter, not an arbitrary unsized local. So probably, your PR has a bug somewhere where it is still checking the wrong feature gate at some place.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
spastorino
force-pushed
the
separate-unsized-locals
branch
from
7dbd3f0
to
5a0baa5
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
spastorino
force-pushed
the
separate-unsized-locals
branch
from
dba6a87
to
f194518
Compare
|
☔ The latest upstream changes (presumably #72756) made this pull request unmergeable. Please resolve the merge conflicts. |
Elinvynia
added
S-waiting-on-author
|
@spastorino what is the status of this? Would be nice to see libcore and friends not rely on an unsound feature. :) |
|
@RalfJung hey, I didn't do any investigation since our last exchanges. Hope I'm able to do so next week but if someone else wants to take this over I'm more than happy to let others continue this work which is probably very very close to be ready. |
Muirrum
added
S-waiting-on-author
|
closing this as it is reworked in #74971 |
Dylan-DPC-zz
added
S-inactive
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Oct 28, 2020
… r=oli-obk Separate unsized locals Closes rust-lang#71694 Takes over again rust-lang#72029 and rust-lang#74971 cc @RalfJung @oli-obk @pnkfelix @eddyb as they've participated in previous reviews of this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #71694