Skip to content

Dropping enum discriminants? #225

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

Closed
joshlf opened this issue Feb 5, 2020 · 11 comments
Closed

Dropping enum discriminants? #225

joshlf opened this issue Feb 5, 2020 · 11 comments
Labels
A-drop Topic: related to dropping C-support Category: Supporting a user to solve a concrete problem

Comments

@joshlf
Copy link

joshlf commented Feb 5, 2020

Are we guaranteed that enum discriminants have trivial drop impls? In other words:

fn foo<T> {
    assert_eq!(std::mem::needs_drop<T>(), std::mem::needs_drop<Option<T>>());
}

The context is that I have the following code:

match self {
    Ok(...) => ...,
    Err(x) => {
        unsafe {
            let x = ptr::read(x);
            let new = // compute on x
            ptr::write(self, new);
        }
    }
}

Am I guaranteed that ptr::write(self, new) is fine even though it clobbers the enum discriminants, which are technically still initialized?

@Diggsey
Copy link

Diggsey commented Feb 5, 2020

This line doesn't really make sense unless self is somehow a raw pointer?

ptr::write(self, new);

It's difficult to tell exactly what you're asking, but writing an invalid value to an enum is always UB even if it's immediately dropped. If you're not doing that it's probably OK?

@Lokathor
Copy link
Contributor

Lokathor commented Feb 5, 2020

(Reminder: if self is &mut T then that will automatically be converted to *mut T at the function call site)

@joshlf
Copy link
Author

joshlf commented Feb 5, 2020

The full context is here for the curious.

Let me make this more precise, though. Here's a slightly filled out version of the method in question:

fn do_thing<T, E>(res: &mut Result<T, E>) {
    match res {
        Ok(...) => ...,
        Err(x) => {
            unsafe {
                let y = ptr::read(x);
                let new = // compute on y
                ptr::write(res, new);
            }
        }
    }
}

Since x is uninitialized by ptr::read(x), there's no possibility of a resource leak when we overwrite it. However, ptr::write(res, new) overwrites not only the now-uninitialized x, but also the enum discriminant. My question is: Am I guaranteed that the discriminant bits have a trivial drop, and thus there are no observable side-effects (including leaking resources) from forgetting those bits without dropping them?

@Lokathor
Copy link
Contributor

Lokathor commented Feb 5, 2020

Since x is uninitialized by ptr::read(x)

This isn't true. x is untouched when you read it out. You're effectively making a Copy of the thing, even if the type isn't normally Copy.

Citation: https://doc.rust-lang.org/core/ptr/fn.read.html#ownership-of-the-returned-value

@joshlf
Copy link
Author

joshlf commented Feb 5, 2020

I'd actually be curious about how we model the behavior of ptr::read. My understanding was that, after y = ptr::read(x), y is considered initialized and x is considered uninitialized (though of course there's no literal "uninitialize" operation in any real-world CPU architecture). The wording in the docs makes it sound like the programmer gets to decide which location they treat as still initialized (although why you'd do y = ptr::read(x) and then discard y and keep using x, I don't know). If that's true, how does the compiler reason about that? That strikes me like the sort of thing that we probably want to be a bit more strict about in the docs.

@Lokathor
Copy link
Contributor

Lokathor commented Feb 5, 2020

Yes, "live" or "not live" is just "in the compiler's head", so to speak.

  • If a type has Drop code, and a value of that type is "live", and it goes out of scope, the Drop will run on the value that just went out of scope.
  • When you do a normal move, y = x, now y is live and x is not (unless x is Copy, but in that case it has no Drop code).
  • When you read, y = read(x), then the read call forces a bitwise identical value to come into being without affecting x. That is, we evaluate the expression read(x) on its own, without thinking of any outer expression. Then that value from the read expression is moved to y. This makes y live, but since the value that's now in y was moved from just the return position of the read call, then x is also still alive. Both are alive.

This is one part of what makes read so dangerous. It effectively makes a type count as being Copy, even when it's not normally. This lets you make a Copy of a Vec, and both Vecs will contain a pointer to the same heap memory, and (for example) when you drop the first Vec the heap memory frees and when you drop the second Vec then you've done some UB (a double-free).

@RalfJung
Copy link
Member

RalfJung commented Feb 5, 2020

I'd actually be curious about how we model the behavior of ptr::read.

It reads the storage pointed to by its argument at the given type, and returns that. Nothing more.


Re: the example code, it seems fine to em assuming you are sure that the "compute on y" code will never panic. Because if it does, you'll double-free the contents of x.

As far as you original question goes, I think it is fair to assume that the enum discriminant doesn't care about dropping.

@Lokathor, you are right but note that the code here doesn't do read(&mut x), which would most easily lead to a double-drop. It does red(x), where x is a reference (that has no drop glue). I am assuming here that y isn't dropped either, it is moved into new which is later consumed by write.

@joshlf
Copy link
Author

joshlf commented Feb 5, 2020

Yeah, we certainly need to ensure that we don't unwind (in the code that I'm actually writing, I do this using panic::catch_unwind followed by an abort).

As far as you original question goes, I think it is fair to assume that the enum discriminant doesn't care about dropping.

Sounds good. What do you think is the right way to articulate that, and should we put that in the spec somewhere? I imagine articulating it something like:

The discriminant bits of an enum do not have any drop code. It is always sound to create any number of live copies of the discriminant bits of an enum.

@RalfJung
Copy link
Member

RalfJung commented Feb 5, 2020

The thing is, drop code is associated with types, not with data. And there is no type for discriminants. So there's no plausible way, for me, how they could have any drop glue.

Given that you raised the question, this is probably worth documenting, but your formulation sounds like data has drop glue, and that gives the wrong impression.

@RalfJung RalfJung added C-support Category: Supporting a user to solve a concrete problem A-drop Topic: related to dropping labels Feb 19, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 8, 2020

I guess the remaining action item here is documenting the behavior of drop glue. But I'd say that's reference material, not UCG, so I will open an issue over there.

@RalfJung
Copy link
Member

RalfJung commented Aug 8, 2020

Closing in favor of rust-lang/reference#873.

@RalfJung RalfJung closed this as completed Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-drop Topic: related to dropping C-support Category: Supporting a user to solve a concrete problem
Projects
None yet
Development

No branches or pull requests

4 participants