Skip to content

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Jul 18, 2022

The aliasing rules of Box<T> are still not decided, but currently, Box<T> is unique and gets noalias. To aid making an informed decision about the future of Box<T>, this PR adds a flag -Zbox-noalias to configure noalias for Box<T> (for example, for benchmarking). The same flag already exists for &mut T noalias, where it was added because it was the problem of various miscompilations in LLVM.

For more information, see rust-lang/unsafe-code-guidelines#326

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 18, 2022
@rust-highfive
Copy link
Contributor

r? @fee1-dead

(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 18, 2022
@Noratrieb Noratrieb force-pushed the toggle-box-noalias branch from 26aaa45 to 09ac838 Compare July 18, 2022 20:15
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

Wait, -Zbox-noalias turns noalias for Box off? The flag name sounds like it turns it on.^^

(Checking the code) ah, -Zbox-noalias=false turns it off. That makes more sense. Please update the PR description to be less confusing. :)

@Noratrieb
Copy link
Member Author

How about box-nonoalias? /j

@Noratrieb Noratrieb force-pushed the toggle-box-noalias branch from 09ac838 to 200f3b8 Compare July 19, 2022 09:40
@Noratrieb Noratrieb changed the title Add flag to turn off noalias on Box<T> Add flag to configure noalias on Box<T> Jul 19, 2022
@fee1-dead
Copy link
Member

The changes look good to me, but to make sure that it works, can you please add a test? (see this for info)

@Noratrieb Noratrieb force-pushed the toggle-box-noalias branch from 200f3b8 to a8253c4 Compare July 19, 2022 13:30
@Noratrieb
Copy link
Member Author

I added a test to ensure that noalias doesn't get passed when the flag is set to no. Now that I think about it, I should add another test for the default case.

To aid making an informed decision about the aliasing
rules of box, give users an option to remove `noalias`
from box.
@Noratrieb
Copy link
Member Author

So, I hope it works now.

@Noratrieb Noratrieb force-pushed the toggle-box-noalias branch from a8253c4 to 7c900c9 Compare July 19, 2022 14:03
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

looks good to me pending CI green.

@Noratrieb
Copy link
Member Author

@fee1-dead and it's green now 🎉

@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 19, 2022

📌 Commit 7c900c9 has been approved by fee1-dead

It is now in the queue for this repository.

@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 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#98101 (stdlib support for Apple WatchOS)
 - rust-lang#99345 (Do not allow typeck children items to constrain outer RPITs)
 - rust-lang#99383 (Formalize defining_use_anchor)
 - rust-lang#99436 (Add flag to configure `noalias` on `Box<T>`)
 - rust-lang#99483 (Fix a numerical underflow in tuple wrap suggestion)
 - rust-lang#99485 (Stop injecting `#[allow(unused_qualifications)]` in generated `derive` implementations)
 - rust-lang#99486 (Refactor: remove a string comparison between types in `check_str_addition`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fffc650 into rust-lang:master Jul 20, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 20, 2022
@Noratrieb Noratrieb deleted the toggle-box-noalias branch July 20, 2022 17:04
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants