Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #56806 by using delay_span_bug in object safety layout sanity checks #57229

Merged
merged 1 commit into from Jan 5, 2019

Conversation

mikeyhew
Copy link
Contributor

It's possible that is_object_safe is called on a trait method that with an invalid receiver type. This caused an ICE in #56806, because receiver_is_dispatchable returns true for self: Box<dyn Trait>, which causes one of the layout sanity checks in object_safety.rs to fail. Replacing bug! with delay_span_bug solves this.

The fact that receiver_is_dispatchable returns true here could be considered a bug. It passes the check that the method implements, though: Box<dyn Trait> implements DispatchFromDyn<Box<dyn Trait>> because dyn Trait implements Unsize<dyn Trait>. It would be good to hear what @eddyb and @nikomatsakis think.

Note that I only added a test for the case encountered in #56806. I could not come up with a case that triggered an ICE from the other check, bug!("receiver when Self = dyn Trait should be ScalarPair, found Scalar"). There is no way, to my knowledge, that you can make receiver_is_dispatchable return true but still have a Scalar ABI when Self = dyn Trait.

One other case I encountered while debugging #56806 was that if you have a type parameter T that implements Deref<Target=Self> and DispatchFromDyn<T>, and use it as a method receiver, it will cause an ICE during is_object_safe because T has no layout (playground):

trait Trait<T: Deref<Target=Self> + DispatchFromDyn<T>> {
    fn foo(self: T) -> dyn Trait<T>;
}

I don't intend to remove the ICE there because it is a pathological case, especially since there is no way to implement DispatchFromDyn<T> for T — the checks in typeck/coherence/builtin.rs do not allow that.

fixes #56806
r? @varkor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2018
@varkor
Copy link
Member

varkor commented Jan 1, 2019

I don't intend to remove the ICE there because it is a pathological case, especially since there is no way to implement DispatchFromDyn for T — the checks in typeck/coherence/builtin.rs do not allow that.

If it's possible for the user to trigger it, it should be addressed, even if it isn't something that's likely to be input. But that can be left for a future PR. It would be good to open an issue to keep track of it, though.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with the copyright notice removed.

@varkor varkor added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 1, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:046d1c72:start=1546444275453806109,finish=1546444335251201728,duration=59797395619
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:57:45] .................................................................................................... 2500/5224
[00:57:49] .................................................................................................... 2600/5224
[00:57:53] .................................................................................................... 2700/5224
[00:57:57] .................................................................................................... 2800/5224
[00:58:00] ...................F................................................................................ 2900/5224
[00:58:07] .................................................................................................... 3100/5224
[00:58:10] .............i...................................................................................... 3200/5224
[00:58:13] ............................................................................ii...i..ii.............. 3300/5224
[00:58:17] .................................................................................................... 3400/5224
---
[00:59:19] 
[00:59:19] ---- [ui] ui/issues/issue-56806.rs stdout ----
[00:59:19] diff of stderr:
[00:59:19] 
[00:59:19] 1 error[E0307]: invalid method receiver type: std::boxed::Box<(dyn Trait + 'static)>
[00:59:19] +   --> $DIR/issue-56806.rs:2:34
[00:59:19] 3    |
[00:59:19] 3    |
[00:59:19] 4 LL |     fn dyn_instead_of_self(self: Box<dyn Trait>);
[00:59:19] 
[00:59:19] 
[00:59:19] The actual stderr differed from the expected stderr.
[00:59:19] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-56806/issue-56806.stderr
[00:59:19] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-56806/issue-56806.stderr
[00:59:19] To update references, rerun the tests and pass the `--bless` flag
[00:59:19] To only update this specific test, also pass `--test-args issues/issue-56806.rs`
[00:59:19] error: 1 errors occurred comparing output.
[00:59:19] status: exit code: 1
[00:59:19] status: exit code: 1
[00:59:19] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/issues/issue-56806.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-56806/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknorting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:59:19] {"message":"For more information about this error, try `rustc --explain E0307`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0307`.\n"}
[00:59:19] ------------------------------------------
[00:59:19] 
[00:59:19] thread '[ui] ui/issues/issue-56806.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3245:9
[00:59:19] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:59:19] 
[00:59:19] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:495:22
[00:59:19] 
[00:59:19] 
[00:59:19] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/ltravis_time:end:0ca3a460:start=1546444343601187202,finish=1546447903148233843,duration=3559547046641
travis_time:start:0de4a138
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Jan  2 16:51:43 UTC 2019
Wed, 02 Jan 2019 16:51:43 GMT

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 @TimNN. (Feature Requests)

@mikeyhew mikeyhew changed the title Fix #54806 by using delay_span_bug in object safety layout sanity checks Fix #56806 by using delay_span_bug in object safety layout sanity checks Jan 2, 2019
@varkor
Copy link
Member

varkor commented Jan 4, 2019

@mikeyhew: sorry for the delay — could you squash the changes? After that, r=me.

It's possible that `is_object_safe` is called on a trait that is ill-formed, and we shouldn't ICE unless there are no errors being raised. Using `delay_span_bug` solves this.

fixes rust-lang#56806
@mikeyhew
Copy link
Contributor Author

mikeyhew commented Jan 4, 2019

@varkor squashed

@varkor
Copy link
Member

varkor commented Jan 4, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2019

📌 Commit 2433526 has been approved by varkor

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2019
kennytm added a commit to kennytm/rust that referenced this pull request Jan 5, 2019
Fix rust-lang#56806 by using `delay_span_bug` in object safety layout sanity checks

It's possible that `is_object_safe` is called on a trait method that with an invalid receiver type. This caused an ICE in rust-lang#56806, because `receiver_is_dispatchable` returns `true` for `self: Box<dyn Trait>`, which causes one of the layout sanity checks in object_safety.rs to fail. Replacing `bug!` with `delay_span_bug` solves this.

The fact that `receiver_is_dispatchable` returns `true` here could be considered a bug. It passes the check that the method implements, though: `Box<dyn Trait>` implements `DispatchFromDyn<Box<dyn Trait>>` because `dyn Trait` implements `Unsize<dyn Trait>`. It would be good to hear what @eddyb and @nikomatsakis think.

Note that I only added a test for the case encountered in rust-lang#56806. I could not come up with a case that triggered an ICE from the other check, `bug!("receiver when Self = dyn Trait should be ScalarPair, found Scalar")`. There is no way, to my knowledge, that you can make `receiver_is_dispatchable` return true but still have a `Scalar` ABI when `Self = dyn Trait`.

One other case I encountered while debugging rust-lang#56806 was that if you have a type parameter `T` that implements `Deref<Target=Self>` and `DispatchFromDyn<T>`, and use it as a method receiver, it will cause an ICE during `is_object_safe` because `T` has no layout ([playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d9b7497b3be0ca8382fa7d9497263214)):

```rust
trait Trait<T: Deref<Target=Self> + DispatchFromDyn<T>> {
    fn foo(self: T) -> dyn Trait<T>;
}
```

I don't intend to remove the ICE there because it is a pathological case, especially since there is no way to implement `DispatchFromDyn<T>` for `T` — the checks in typeck/coherence/builtin.rs do not allow that.

fixes rust-lang#56806
r? @varkor
bors added a commit that referenced this pull request Jan 5, 2019
Rollup of 17 pull requests

Successful merges:

 - #57219 (Remove some unused code)
 - #57229 (Fix #56806 by using `delay_span_bug` in object safety layout sanity checks)
 - #57233 (Rename and fix nolink-with-link-args test)
 - #57238 (Fix backtraces for inlined functions on Windows)
 - #57249 (Fix broken links to second edition TRPL.)
 - #57267 (src/jemalloc is gone, remove its mention from COPYRIGHT)
 - #57273 (Update the stdsimd submodule)
 - #57278 (Add Clippy to config.toml.example)
 - #57295 (Fix 'be be' constructs)
 - #57311 (VaList::copy should not require a mutable ref)
 - #57312 (`const fn` is no longer coming soon (const keyword docs))
 - #57313 (Improve Box<T> -> Pin<Box<T>> conversion)
 - #57314 (Fix repeated word typos)
 - #57326 (Doc rewording, use the same name `writer`)
 - #57338 (rustdoc: force binary filename for compiled doctests)
 - #57342 (librustc_mir: Make qualify_min_const_fn module public)
 - #57343 (Calculate privacy access only via query)

Failed merges:

 - #57340 (Use correct tracking issue for c_variadic)

r? @ghost
@bors bors merged commit 2433526 into rust-lang:master Jan 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Receiver when Self = () should have a Scalar ABI, found ScalarPair ...
4 participants