Open RalfJung opened 4 years ago
Can you clarify more details on what is currently missing? Comparing that issue to https://doc.rust-lang.org/nightly/reference/destructors.html, it seems like the current docs are fairly clear which values have explicit drop code generated. Based on your comment at https://github.com/rust-lang/unsafe-code-guidelines/issues/225#issuecomment-582354360, I also conclude that since a enum discriminant is not listed as needing a destructor, that there aren't any special requirements for "dropping" it.
I mostly forwarded this issue from https://github.com/rust-lang/unsafe-code-guidelines/issues/225, so @joshlf would be good if you could answer the question above. :)
Not sure if the reference is the right place for that, but I'd be curious to learn how the drop glue is implemented (ie the exact logic, and also what the generated code looks like)
@ehuss
Can you clarify more details on what is currently missing?
In the original issue, I had code like the following:
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);
}
}
}
}
The question was whether the ptr::write(res, new)
operation was sound, seeing as it overwrites not only the storage for x
, butt also the enum's discriminant. By analogy, imagine we did something like this:
let mut b = Box::new(0);
ptr::write(&mut b, Box::new(1));
While this wouldn't be UB, the act of overwriting the bits of b
would effectively mem::forget
the original contents of b
, which would have the side effect of leaking memory.
The question in the original case is: does the act of overwriting the discriminant of an enum have any side effect? Or, to put it another way, does the act of mem::forget
-ing the discriminant of an enum have any side effect?
If we wanted to be more formal, and phrase this in terms that make reference only to well-defined concepts in the Rust reference, we might rephrase this as something like: Let E
be an enum type, and e: E
. If it is the case that e
does not contain any values, v
, such that dropping v
would have observable effects, is it necessarily also the case that dropping e
does not have any observable effects?
Note: I phrase it in this convoluted manner since it's possible that some other variant of E
besides the variant that e
currently takes on might contain values whose dropping would have side effects.
@elichai Perhaps ask in the compiler channel on Zulip?
@joshlf I don't think the discriminant matters much there. Once it's checked in the match and you are inside it, you're no longer accessing it. You are accessing x
still though, so I'm not sure there. The discriminant doesn't do anything special with how it is dropped itself except for having it tell drop glue what fields to drop when the drop happens.
I think the answer to your question is a "yes".
The question was whether the ptr::write(res, new) operation was sound, seeing as it overwrites not only the storage for x, butt also the enum's discriminant.
It is not sound. x
is a reference that is only valid for a field, but you overwrite the discriminant too.
Comparing that issue to https://doc.rust-lang.org/nightly/reference/destructors.html, it seems like the current docs are fairly clear which values have explicit drop code generated.
The interaction between NLL, Drop
implementers, and scope isn't clear (to me anyway) from that document. Is it documented somewhere? (Apologies in advance if this is too off topic.)
N.b. I arrived here after looking for documentation to reference in a URLO thread.
NLL doesn't affect the location of drops. Instead drop types still behave as if they have lexical lifetimes if they are not explicitly moved or dropped.
@bjorn3
It is not sound.
x
is a reference that is only valid for a field, but you overwrite the discriminant too.
Yes, but we don't use the reference x
after the ptr::read
call. As to whether overwriting the discriminant makes this unsound, that's the question for this issue :) I've been lead to believe that it's sound because the discriminant is, to abuse an existing phrase, plain old data.
It's invalid for entirely unrelated reasons -- when you do &struct.field
, that reference (and the pointers derived from it) cannot be used to access anything outside that field. Also see https://github.com/rust-lang/unsafe-code-guidelines/issues/243, https://github.com/rust-lang/unsafe-code-guidelines/issues/256.
EDIT: Looking at the code again, I don't actually see you doing that... you are writing into res
. I am not sure how to interpret @bjorn3's comment.
Enum discriminants certainly do not have any drop glue themselves, they just guide the drop glue by determining which variant to call the fields' drop glue for.
However, overwriting with ptr::write
doesn't cause any drop glue to be called, so I fail to see how your code even has anything to do with drop glue, @joshlf. There seems to be no drop glue being executed there?
However, overwriting with
ptr::write
doesn't cause any drop glue to be called, so I fail to see how your code even has anything to do with drop glue, @joshlf. There seems to be no drop glue being executed there?
In some cases, not executing drop glue can have observable effects. E.g., mem::forget(Box::new(0))
has the effect of leaking memory.
Oh that's what you mean. I'd call that the absence of an effect -- so not executing something can be observable, but can never "have an effect". Hence my confusion.
Ah, understood. I guess I've been using "effect" and "observable" as interchangeable.
So, as far as I know, the drop glue for enum
s looks something like this:
enum E { V1(T1, T2, ...), ... }
fn drop_in_place(ptr: *mut E) {
// Call user-defined drop (if it exists).
Drop::drop(&mut *ptr);
// Call drop glue of the fields.
match *ptr {
E::V1(f1, f2, ...) => {
// FIXME: I am not entirely sure about the order here.
drop_in_place(f1); // T1 drop glue
drop_in_place(f2); // T2 drop glue
...
}
...
}
}
Does that answer your question?
Guess I dropped the ball on responding; that does answer my question, although I do still wonder whether that behavior (or at least some observable aspects of that behavior) should be guaranteed by the reference. It sounds like you're just saying that that's what happens today, not that that's what's guaranteed by the language?
FWIW, this is no longer relevant to me personally, although ofc it's still worthwhile to clarify in the reference IMO.
I would like to vote in favor of some sort of augmentation of the documentation with regard to the term "drop glue". The Drop
documentation uses the term "drop glue" in just one place, and destructors documentation never uses the term at all. It is, however, sometimes used in answers to questions, but searching for "rust drop glue" doesn't turn up much that I (at least) found very helpful.
This specifically came up for me in working through this answer to a question I asked on the Rust Users Forum. The issue I had came down to a closure definition unexpectedly (to me) capturing a lifetime in a way that led to a compiler error. A closure couldn't be dropped because it's "drop glue" required access to the captured lifetime that would otherwise have already gone out of scope. I never would have figured that out without the help of the nice responder on the Rust Users Forum, and even with their help it took some doing to chase through the details until I felt like I actually understood what was happening.
It would be nice if:
-> impl ...
types going out of scope can constitute a use of those types (executing the drop glue), which can lead to problems with dangling lifetimes.-> impl ...
types capture lifetimes were more than the "generic" message about lifetimes going out of scope, since none of the examples or explanations that points to address the issue of -> impl ...
types capturing lifetimes. In my code it wasn't at all clear (to me, and other people in my live stream at the time) where that error was coming from and what we might do about it.
There should be a section in the reference which document how the compiler-generated drop glue behaves. It should answer questions like this one: https://github.com/rust-lang/unsafe-code-guidelines/issues/225.