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

Show a constant's virtual memory on validation errors #53325

Closed
robsmith11 opened this issue Aug 14, 2018 · 11 comments · Fixed by #76881
Closed

Show a constant's virtual memory on validation errors #53325

robsmith11 opened this issue Aug 14, 2018 · 11 comments · Fixed by #76881
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@robsmith11
Copy link

robsmith11 commented Aug 14, 2018

The following code worked up until nightly-2018-07-30-x86_64-unknown-linux-gnu:

pub union Transmute<T:Copy, U:Copy> {                                                                          
    pub from:T,
    pub to:U,                                                                                              
}                       
                                                                                                                                                                                              
#[derive(Clone,Copy)]
struct Foo(i64);                                                                                           

static FOO:Foo = unsafe { Transmute { from:0 }.to };                                                                                                                                                                  

fn main() {
}
error[E0080]: this static likely exhibits undefined behavior
  --> src/main.rs:9:1                                                                                      
   |                                                                                                      
 9 | static FOO:Foo = unsafe { Transmute { from:0 }.to };                                               
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered undefined bytes at .0                                                                                                
   |                                                                                                  
     = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior                                                                                                 
error: aborting due to previous error
@eddyb
Copy link
Member

eddyb commented Aug 14, 2018

cc @oli-obk

@eddyb
Copy link
Member

eddyb commented Aug 14, 2018

This seems legit! Your 0 there is not typed as 0i64, but instead it infers by default to i32, which means half of your Foo would be undefined.

@robsmith11
Copy link
Author

Ah.. that makes sense. Making the from type explicit does indeed work.

Using the default i32 value does work for non-static types, which is different behavior, but I suppose that's not a real problem since it's unsafe and UB anyway.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 14, 2018

Reopening to track the improvement of the diagnostic to show the bit pattern and which bytes were undefined.

@oli-obk oli-obk reopened this Aug 14, 2018
@oli-obk oli-obk added A-diagnostics Area: Messages for errors, warnings, and lints A-const-eval Area: constant evaluation (mir interpretation) labels Aug 14, 2018
@XOSplicer
Copy link

The work-around where an untagged union is used to crate uninitialized memory in a const fn also seems to be affected by this issue.

union Foo<T> {
    t: T,
    u: (),
}
unsafe const fn uninitialized<T>() -> T {
    Foo { u: () }.t
}

This code does no longer compile since the t part of the union is intentionally uninitialized.
However according to the RFC below, uninitialized memeory handling should in future be done with a new MaybeUninit type.

see also discussion in rust-lang/rfcs#411 and #50150 and rust-lang/rfcs#1892

@oli-obk
Copy link
Contributor

oli-obk commented Aug 16, 2018

Yes, you should be returning a Foo<T> and have the user initialize the t field whenever they are ready instead of producing an invalid T.

Additionally, your code is not affected. Only if you use the function to produce a const or static that contains this value.

@oli-obk oli-obk self-assigned this Jan 28, 2019
@oli-obk oli-obk added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Mar 5, 2019
@saleemjaffer
Copy link
Contributor

@oli-obk I would like to take this up.

@oli-obk oli-obk removed their assignment Mar 13, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Mar 13, 2019

So... Before we start out on this, we should bikeshed what the output should look like.

cc @rust-lang/wg-diagnostics I'm wondering if it would make sense to render the memory of a constant similar to how hex editors render memory. Then we could dump the rendered version in a virtual file that diagnostics can get spans into. For the given example above (where only half of a u64 is initialzed) I'm thinking that the virtual file could look like

00 00 00 00 ⓍⓍ ⓍⓍ ⓍⓍ ⓍⓍ

and thus a diagnostic pointing into it would look like

note: this `i64` does not fully consist of defined bytes
  --> FOO.0:1:12
00 00 00 00 ⓍⓍ ⓍⓍ ⓍⓍ ⓍⓍ
            ^^ undefined byte

@eddyb
Copy link
Member

eddyb commented Mar 14, 2019

Then we could dump the rendered version in a virtual file that diagnostics can get spans into.

Hopefully we can bypass needing an actual Span in order to report a good-looking error.

Also keep in mind IDEs can't really show your "virtual hex dump" files, as we don't have a way to export them from the compiler (other than saving them in a temp dir), so it'd be nicer to make the long-form "rendered" message itself contain the hex dump.

Minor bikeshed: looks too much like a 0 or a 8 for me.
Also, it's wider than a regular monospace character, so ^ wouldn't work.

Alternatives (press to expand)
00 00 00 00 UU UU UU UU
            ^^ undefined byte
00 00 00 00 uu uu uu uu
            ^^ undefined byte
00 00 00 00 XX XX XX XX
            ^^ undefined byte
00 00 00 00 xx xx xx xx
            ^^ undefined byte
00 00 00 00 ?? ?? ?? ??
            ^^ undefined byte
00 00 00 00 ## ## ## ##
            ^^ undefined byte
00 00 00 00 __ __ __ __
            ^^ undefined byte

@oli-obk
Copy link
Contributor

oli-obk commented Mar 14, 2019

00 00 00 00 __ __ __ __
            ^^ undefined byte

I like that one, it's very clear on a glance

@oli-obk oli-obk changed the title Using untagged unions in statics is broken in latest nightly Show a constant's virtual memory on validation errors Mar 14, 2019
@solson
Copy link
Member

solson commented Mar 14, 2019

Underscores are also the format I used for undefined bytes in the original Miri report and slide deck, so +1 from me. :)

@estebank estebank added the P-low Low priority label May 25, 2019
@hdhoang hdhoang added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Nov 21, 2019
@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
Add allocation information to undefined behaviour errors.

So far I'm looking on information on whether the error messages are suitable.

Fixes rust-lang#53325.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
Add allocation information to undefined behaviour errors.

So far I'm looking on information on whether the error messages are suitable.

Fixes rust-lang#53325.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
Add allocation information to undefined behaviour errors.

So far I'm looking on information on whether the error messages are suitable.

Fixes rust-lang#53325.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 31, 2021
Add allocation information to undefined behaviour errors.

So far I'm looking on information on whether the error messages are suitable.

Fixes rust-lang#53325.
@bors bors closed this as completed in 23fa536 Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants