Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 10, 2018

Add a short explanation saying why undefined behavior could arise. In particular, the error many people got for "creating a pointer to a packed field requires unsafe block" was not worded great -- it lead to people just adding the unsafe block without considering if what they are doing follows the rules.

I am not sure if a "note" is the right thing, but that was the easiest thing to add...

Inspired by @gnzlbg at #46043 (comment)

@rust-highfive
Copy link
Contributor

r? @michaelwoerister

(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 Jul 10, 2018
@michaelwoerister
Copy link
Member

r? @estebank

= note: fields of packed structs might be misaligned: dereferencing a misaligned pointer or even just creating a misaligned reference is undefined behavior

error: aborting due to 2 previous errors

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nice, thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

And it took me only three months since you suggested it...^^

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 10, 2018

📌 Commit f511c5e has been approved by estebank

@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 Jul 10, 2018
"assignment to non-`Copy` union field")
"assignment to non-`Copy` union field",
"the previous content of the field may be dropped, which \
cause undefined behavior if the field was not properly \
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "which cause"

Copy link
Member Author

Choose a reason for hiding this comment

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

I must be blind, what is the typo there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, it must be "which causes", of course. Sorry!

@estebank
Copy link
Contributor

estebank commented Jul 10, 2018

@bors r-

@RalfJung can you fix the above mentioned typo? r=me afterwards.

@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 Jul 10, 2018
@RalfJung
Copy link
Member Author

I should also add that I am not yet sure why "assignment to non-Copy union field" is unsafe, still trying to get an answer from whoever implemented that.

@RalfJung
Copy link
Member Author

I should also add that I am not yet sure why "assignment to non-Copy union field" is unsafe, still trying to get an answer from whoever implemented that.

I verified from the RFC that this is exactly what happens. (There is a more elaborate scheme being proposed where fields are only sometimes dropped, but right now it seems to always drop.)

@RalfJung
Copy link
Member Author

Fixed typo.

@RalfJung
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Collaborator

bors commented Jul 11, 2018

@RalfJung: 🔑 Insufficient privileges: Not in reviewers

@RalfJung
Copy link
Member Author

@estebank looks like you have to r+ yourself.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 11, 2018

📌 Commit f68323b has been approved by estebank

@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 Jul 11, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 11, 2018
improve error message shown for unsafe operations

Add a short explanation saying why undefined behavior could arise. In particular, the error many people got for "creating a pointer to a packed field requires unsafe block" was not worded great -- it lead to people just adding the unsafe block without considering if what they are doing follows the rules.

I am not sure if a "note" is the right thing, but that was the easiest thing to add...

Inspired by @gnzlbg at rust-lang#46043 (comment)
bors added a commit that referenced this pull request Jul 11, 2018
Rollup of 14 pull requests

Successful merges:

 - #51614 (Correct suggestion for println)
 - #51952 ( hygiene: Decouple transparencies from expansion IDs)
 - #52193 (step_by: leave time of item skip unspecified)
 - #52207 (improve error message shown for unsafe operations)
 - #52223 (Deny bare trait objects in in src/liballoc)
 - #52224 (Deny bare trait objects in in src/libsyntax)
 - #52239 (Remove sync::Once::call_once 'static bound)
 - #52247 (Deny bare trait objects in in src/librustc)
 - #52248 (Deny bare trait objects in in src/librustc_allocator)
 - #52252 (Deny bare trait objects in in src/librustc_codegen_llvm)
 - #52253 (Deny bare trait objects in in src/librustc_data_structures)
 - #52254 (Deny bare trait objects in in src/librustc_metadata)
 - #52261 (Deny bare trait objects in in src/libpanic_unwind)
 - #52265 (Deny bare trait objects in in src/librustc_codegen_utils)

Failed merges:

r? @ghost
@bors bors merged commit f68323b into rust-lang:master Jul 11, 2018
@RalfJung RalfJung deleted the unsafety-errors branch July 23, 2018 09:16
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.

7 participants