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

#62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit) should be backed out #62825

Open
ishitatsuyuki opened this issue Jul 20, 2019 · 14 comments

Comments

@ishitatsuyuki
Copy link
Contributor

commented Jul 20, 2019

#62150 has changed the behavior of mem::uninitialized() (and zeroed()), causing a few popular crates to be killed by SIGILL. A writeup can be found here.

There are cases where outdated (EOL) versions of downstream dependencies have this issue. In that case, the upstream crate will have to migrate to a newer major version of the dependency, which means it will have to deal with those breaking changes.

Therefore, I suggest we back out the change for at least one stable release, providing the community with the time to upgrade.

cc @RalfJung

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Note that (to my knowledge) this change is in nightly only currently, and won't become stable until 1.38 (released in late September). But it causes trouble already now e.g. by making CI fail, I suppose.

Nominating for lang team discussion. I hope I am doing this right.^^

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Some more relevant discussion:

  • #52898 (comment) is an old issue about mem::zeroed causing SIGILLs when used incorrectly.
  • I proposed to eventually make zeroed (and uninitialized) panic deterministically for some more cases that we can detect statically, likely including the ones causing issues here. But we should probably wait a bit with this.

@RalfJung RalfJung changed the title #62150 should be backed out #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit) should be backed out Jul 20, 2019

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Rather than panicking at runtime, could we detect this at compile-time, and issue a compile-time warning about calling mem::uninitialized or mem::zeroed with a type of &T or a function type or similar?

I think we need some kind of compile-time warning that people can start fixing, so that people know they need to fix this. Otherwise, we'll be in the same boat when we un-revert this.

@Centril Centril removed the I-nominated label Jul 25, 2019

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Based on discussion in the lang team: we'd be fine with backing this out, but we do need a lint or similar mechanism to make sure people notice and fix code.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@joshtriplett

Rather than panicking at runtime, could we detect this at compile-time, and issue a compile-time warning about calling mem::uninitialized or mem::zeroed with a type of &T or a function type or similar?

I assume you are responding to this proposal?

For some cases yes, but not reliably in generic contexts.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Agreed. I think we can do something that traverses the type as far as we see it, and complains when it finds a reference, fn, NonNull or NonZero*.

We should probably stop looking when encountering an enum, or else we'd have to figure out which variant would be picked by the all-0 bit pattern and then look at the fields of that.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@ishitatsuyuki beta branches in 12 days. If you want this to be backed out, someone has to do it before that.

@ishitatsuyuki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Is the lint a requirement for backing out? If so, we may want to reach a consensus on how to implement the check.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Is the lint a requirement for backing out?

No, I don't think so. @joshtriplett could you clarify?

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Definitely not a requirement for backing out. More a requirement to happen well in advance of re-adding.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

I'm working on a lint, should have a PR ready tomorrow.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Lint PR up at #63346

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@joshtriplett what is the timeline you had in mind for putting the change back in place? uninit and init are two rather questionable intrinsics, so I'd like to get rid of them for good as soon as we can.

To my knowledge, the only crates that we know got broken by this are x11-rs (the broken part of which is "one giant hack" in the words of the maintainer) and old versions of crossbeam::queue (version 0.4 is reported to be broken; 0.5 has been released 9 months ago but I do not know if that one is fixed but crossbeam-queue 0.1 released 7 months ago and it is fixed).

Centril added a commit to Centril/rust that referenced this issue Aug 9, 2019

Rollup merge of rust-lang#63346 - RalfJung:zeroed-lint, r=eddyb
Lint on some incorrect uses of mem::zeroed / mem::uninitialized

Cc rust-lang#62825 and https://internals.rust-lang.org/t/make-mem-uninitialized-and-mem-zeroed-panic-for-some-types-where-0-is-a-niche/10605

This does not yet handle `NonNull`/`NonZero*`, but it is a start.

I also improved some doc issues I hit on the way, and added a useful helper to `TyS`.

bors added a commit that referenced this issue Aug 11, 2019

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Aug 11, 2019

Rollup merge of rust-lang#63346 - RalfJung:zeroed-lint, r=eddyb
Lint on some incorrect uses of mem::zeroed / mem::uninitialized

Cc rust-lang#62825 and https://internals.rust-lang.org/t/make-mem-uninitialized-and-mem-zeroed-panic-for-some-types-where-0-is-a-niche/10605

This does not yet handle `NonNull`/`NonZero*`, but it is a start.

I also improved some doc issues I hit on the way, and added a useful helper to `TyS`.

EDIT: I added the relnotes label mostly as a proposal -- I think this is worth mentioning, but leave the decision up to the release team.

bors added a commit that referenced this issue Aug 11, 2019

Auto merge of #63346 - RalfJung:zeroed-lint, r=eddyb
Lint on some incorrect uses of mem::zeroed / mem::uninitialized

Cc #62825 and https://internals.rust-lang.org/t/make-mem-uninitialized-and-mem-zeroed-panic-for-some-types-where-0-is-a-niche/10605

This does not yet handle `NonNull`/`NonZero*`, but it is a start.

I also improved some doc issues I hit on the way, and added a useful helper to `TyS`.

EDIT: I added the relnotes label mostly as a proposal -- I think this is worth mentioning, but leave the decision up to the release team.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Aug 11, 2019

Rollup merge of rust-lang#63346 - RalfJung:zeroed-lint, r=eddyb
Lint on some incorrect uses of mem::zeroed / mem::uninitialized

Cc rust-lang#62825 and https://internals.rust-lang.org/t/make-mem-uninitialized-and-mem-zeroed-panic-for-some-types-where-0-is-a-niche/10605

This does not yet handle `NonNull`/`NonZero*`, but it is a start.

I also improved some doc issues I hit on the way, and added a useful helper to `TyS`.

EDIT: I added the relnotes label mostly as a proposal -- I think this is worth mentioning, but leave the decision up to the release team.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.