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 ICE in mir when evaluating SizeOf on unsized type #81185

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Jan 19, 2021

Not quite ready yet. This tries to fix #80742 as discussed on Zulip topic,
by using delay_span_bug.

I don't understand what delay_span_bug does. It seems like my error message
is never used. With this patch, in this program:

#![allow(incomplete_features)]
#![feature(const_evaluatable_checked)]
#![feature(const_generics)]

use std::fmt::Debug;
use std::marker::PhantomData;
use std::mem::size_of;

struct Inline<T>
where
    [u8; size_of::<T>() + 1]: ,
{
    _phantom: PhantomData<T>,
    buf: [u8; size_of::<T>() + 1],
}

impl<T> Inline<T>
where
    [u8; size_of::<T>() + 1]: ,
{
    pub fn new(val: T) -> Inline<T> {
        todo!()
    }
}

fn main() {
    let dst = Inline::<dyn Debug>::new(0); // line 27
}

these errors are printed, both for line 27 (annotated line above):

  • "no function or associated item named new found for struct Inline<dyn Debug> in the current scope"
  • "the size for values of type dyn Debug cannot be known at compilation time"

Second error makes sense, but I'm not sure about the first one and why it's
even printed.

Finally, I'm not sure about the span passing in const_eval.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Jan 19, 2021

I don't understand what delay_span_bug does. It seems like my error message
is never used.

That means it's working 😆 . delay_span_bug is a way to make sure we poison the compiler to cause an ICE if no errors are reported. The message and span would be used in that case, but as long as an error is reported elsewhere, it is not shown. While we could report an error here, that would just add to the noise and likely be a less useful error than the one emitted by typeck.

Second error makes sense, but I'm not sure about the first one and why it's
even printed.

The first error occurs because we fail to satisfy the generic bounds, so we don't find the new method that you wrote. Basically you have to treat it as if the compiler goes through all impl blocks to find methods, but it doesn't even look into the impl block if it can't satisfy the bounds on the impl block.

@osa1
Copy link
Contributor Author

osa1 commented Jan 19, 2021

Thanks for the detailed answer @oli-obk , very helpful.

@osa1
Copy link
Contributor Author

osa1 commented Jan 19, 2021

Interesting.. I updated by branch (https://github.com/osa1/rust/tree/fix_80742) but the PR is not updated, it's still showing the old version.

@osa1
Copy link
Contributor Author

osa1 commented Jan 19, 2021

@oli-obk This is ready for reviews & merge.

@osa1 osa1 requested a review from oli-obk January 19, 2021 14:33
@oli-obk
Copy link
Contributor

oli-obk commented Jan 19, 2021

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 19, 2021

📌 Commit 3fb53c2 has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 20, 2021
Fix ICE in mir when evaluating SizeOf on unsized type

Not quite ready yet. This tries to fix rust-lang#80742 as discussed on [Zulip topic][1],
by using `delay_span_bug`.

I don't understand what `delay_span_bug` does. It seems like my error message
is never used. With this patch, in this program:

```rust
#![allow(incomplete_features)]
#![feature(const_evaluatable_checked)]
#![feature(const_generics)]

use std::fmt::Debug;
use std::marker::PhantomData;
use std::mem::size_of;

struct Inline<T>
where
    [u8; size_of::<T>() + 1]: ,
{
    _phantom: PhantomData<T>,
    buf: [u8; size_of::<T>() + 1],
}

impl<T> Inline<T>
where
    [u8; size_of::<T>() + 1]: ,
{
    pub fn new(val: T) -> Inline<T> {
        todo!()
    }
}

fn main() {
    let dst = Inline::<dyn Debug>::new(0); // line 27
}
```

these errors are printed, both for line 27 (annotated line above):

- "no function or associated item named `new` found for struct `Inline<dyn
  Debug>` in the current scope"
- "the size for values of type `dyn Debug` cannot be known at compilation time"

Second error makes sense, but I'm not sure about the first one and why it's
even printed.

Finally, I'm not sure about the span passing in `const_eval`.

[1]: https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Help.20fixing.20.2380742
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 20, 2021
Fix ICE in mir when evaluating SizeOf on unsized type

Not quite ready yet. This tries to fix rust-lang#80742 as discussed on [Zulip topic][1],
by using `delay_span_bug`.

I don't understand what `delay_span_bug` does. It seems like my error message
is never used. With this patch, in this program:

```rust
#![allow(incomplete_features)]
#![feature(const_evaluatable_checked)]
#![feature(const_generics)]

use std::fmt::Debug;
use std::marker::PhantomData;
use std::mem::size_of;

struct Inline<T>
where
    [u8; size_of::<T>() + 1]: ,
{
    _phantom: PhantomData<T>,
    buf: [u8; size_of::<T>() + 1],
}

impl<T> Inline<T>
where
    [u8; size_of::<T>() + 1]: ,
{
    pub fn new(val: T) -> Inline<T> {
        todo!()
    }
}

fn main() {
    let dst = Inline::<dyn Debug>::new(0); // line 27
}
```

these errors are printed, both for line 27 (annotated line above):

- "no function or associated item named `new` found for struct `Inline<dyn
  Debug>` in the current scope"
- "the size for values of type `dyn Debug` cannot be known at compilation time"

Second error makes sense, but I'm not sure about the first one and why it's
even printed.

Finally, I'm not sure about the span passing in `const_eval`.

[1]: https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Help.20fixing.20.2380742
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#79655 (Add Vec visualization to understand capacity)
 - rust-lang#80172 (Use consistent punctuation for 'Prelude contents' docs)
 - rust-lang#80429 (Add regression test for mutual recursion in obligation forest)
 - rust-lang#80601 (Improve grammar in documentation of format strings)
 - rust-lang#81046 (Improve unknown external crate error)
 - rust-lang#81178 (Visit only terminators when removing landing pads)
 - rust-lang#81179 (Fix broken links with `--document-private-items` in the standard library)
 - rust-lang#81184 (Remove unnecessary `after_run` function)
 - rust-lang#81185 (Fix ICE in mir when evaluating SizeOf on unsized type)
 - rust-lang#81187 (Fix typo in counters.rs)
 - rust-lang#81219 (Document security implications of std::env::temp_dir)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
self.frame().current_span(),
&format!("SizeOf nullary MIR operator called for unsized type {}", ty),
);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still abort execution. The result that we return here is wrong. So there should be a new error variant, similar to TransmuteSizeDiff, and that should be raised here.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2021

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 21, 2021
@RalfJung
Copy link
Member

(Too late, it was rolled up.)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2021

Either the rollup succeeds, then this can be fixed in a follow up, or it does not, then it can be fixed here

@bors bors merged commit bc950c8 into rust-lang:master Jan 21, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 21, 2021
@osa1 osa1 deleted the fix_80742 branch January 21, 2021 16:13
@osa1
Copy link
Contributor Author

osa1 commented Jan 21, 2021

I'll submit another PR.

@osa1
Copy link
Contributor Author

osa1 commented Jan 22, 2021

#81243

@lcnr lcnr added the F-generic_const_exprs `#![feature(generic_const_exprs)]` label Jan 23, 2021
@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE'd during const eval of generic with DST
8 participants