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

Attempting to move a non-copyable static segfaults #10577

Closed
sfackler opened this issue Nov 20, 2013 · 20 comments
Closed

Attempting to move a non-copyable static segfaults #10577

sfackler opened this issue Nov 20, 2013 · 20 comments
Labels
I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.
Milestone

Comments

@sfackler
Copy link
Member

fn main() {
    let x = std::unstable::atomics::INIT_ATOMIC_INT;
}

That will segfault inside of memset, preseumably because AtomicInt is non-copyable and it's trying to zero out part of .rodata.

@thestinger
Copy link
Contributor

I thought we didn't allow types with a Drop implementation in a static?

@alexcrichton
Copy link
Member

That's an interesting question. I intended the type to not have Drop implemented, but it appears that the nested NonCopyable structs inside of the atomics are causing this problem.

This will be an interesting conundrum to figure out how to make this type non-copyable but also without destructors.

@thestinger
Copy link
Contributor

@alexcrichton: I could open a pull request adding a non-copyable attribute. The TypeContents code knows how to do this already for types like &mut T.

@sfackler
Copy link
Member Author

I would be on board with a #[no_copy] (or whatever) attribute. It'll be consistent with #[no_freeze] and is a bit better to work with than tossing "useless" NonCopyable fields inside of structs.

@alexcrichton
Copy link
Member

We had #[non_copyable] originally, and we removed it in favor of the NonCopyable type. I would agree that this use-case is an argument for bringing it back, however. I agree with @sfackler though in that I think the name should be #[no_copy] to align with #[no_freeze] and #[no_send].

cc @nikomatsakis and @pcwalton, you guys may have opinions on bringing back the #[no_copy].

@thestinger
Copy link
Contributor

Did we really have #[non_copyable]? I don't remember it.

@nikomatsakis
Copy link
Contributor

Per discussion in meeting on Jan 14, 2014:

  • Enum variants should be considered rvalues, not static items
    • (requires modification to borrowck / mem categorization)
  • Static items should not be permitted to have types that own things with destructors
  • Moves from static items should be forbidden

@flaper87
Copy link
Contributor

I'll work on this one, if that's fine!

@alexcrichton
Copy link
Member

@flaper87, sure! This will involve adding a way to flagging a type as noncopyable without using a destructor (to allow atomics in statics (see #10834), but other than that I don't think anyone will contend with you to work on this.

Nominating, this is a backwards-compat thing for 1.0

@pnkfelix
Copy link
Member

accepted for P-backcompat-lang

@flaper87
Copy link
Contributor

@sfackler could you please update the issue report and use std::sync::atomics::INIT_ATOMIC_INT ? Atomics were pulled out of unstable.

@flaper87
Copy link
Contributor

Quick update on this issue: The restriction for static items with Drop implementations has been added. In order to move that work forward, it's necessary to find a better way to tag NonCopyable types. @nikomatsakis alreay has a PR for this. As soon as the PR is proposed, I'll based the above commit on that PR.

@nikomatsakis
Copy link
Contributor

On Wed, Jan 22, 2014 at 02:42:08PM -0800, Flavio Percoco Premoli wrote:

@nikomatsakis alreay has a PR for this

#11768

@nikomatsakis
Copy link
Contributor

Still weighing what I think is best path. The necessity of modifying trans is a drag, although it's temporary. But it feels like constants like DUMMY_SP etc are nice to be able to do.

@alexcrichton
Copy link
Member

In theory, if we had a Cell or RefCell static constructor you could have a static that you could leak memory into. That being said, both of those structures are implemented with unsafe code, so perhaps part of the safe interface is just not providing static constructors.

Having two separate sets of rules seems a little odd though. I'd almost prefer to start with the more restrictive path and then loosen it later if necessary.

@nikomatsakis
Copy link
Contributor

You cannot create a Cell or RefCell because, to do so, you must invoke Cell::new() etc, which is not permitted in the static patterns. Still, a good point to be careful of. We could add a rule that you cannot create a value that is not Freeze.

@nikomatsakis
Copy link
Contributor

Oh, I see you wrote "if we had a Cell or RefCell static constructor". I missed that at first. Yes, if you had one of those, it'd be bad (actually Cell would be ok, since Cell types must be POD, but ...).

@alexcrichton
Copy link
Member

I've grown to like that static values must have no destructors and the type must be Freeze, and static mut types must have no destructors and.

@alexcrichton
Copy link
Member

This was discussed in today's meeting. We agreed on what Niko commented

bors added a commit that referenced this issue Feb 27, 2014
This pull request partially addresses the 2 issues listed before. As part of the work required for this PR, `NonCopyable` was completely removed.

This PR also replaces the content of `type_is_pod` with `TypeContents::is_pod`, although `type_is_content` is currently not being used anywhere. I kept it for consistency with the other functions that exist in this module.

cc #10834
cc #10577

Proposed static restrictions
=====================

Taken from [this](#11979 (comment)) comment.

I expect some code that, at a high-level, works like this:

- For each *mutable* static item, check that the **type**:
    - cannot own any value whose type has a dtor
    - cannot own any values whose type is an owned pointer
- For each *immutable* static item, check that the **value**:
      - does not contain any ~ or box expressions (including ~[1, 2, 3] sort of things, for now)
      - does not contain a struct literal or call to an enum variant / struct constructor where
          - the type of the struct/enum is freeze
          - the type of the struct/enum has a dtor
bors added a commit that referenced this issue Feb 28, 2014
This pull request partially addresses the 2 issues listed before. As part of the work required for this PR, `NonCopyable` was completely removed.

This PR also replaces the content of `type_is_pod` with `TypeContents::is_pod`, although `type_is_content` is currently not being used anywhere. I kept it for consistency with the other functions that exist in this module.

cc #10834
cc #10577

Proposed static restrictions
=====================

Taken from [this](#11979 (comment)) comment.

I expect some code that, at a high-level, works like this:

- For each *mutable* static item, check that the **type**:
    - cannot own any value whose type has a dtor
    - cannot own any values whose type is an owned pointer
- For each *immutable* static item, check that the **value**:
      - does not contain any ~ or box expressions (including ~[1, 2, 3] sort of things, for now)
      - does not contain a struct literal or call to an enum variant / struct constructor where
          - the type of the struct/enum is freeze
          - the type of the struct/enum has a dtor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.
Projects
None yet
Development

No branches or pull requests

6 participants