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

Document auto-generated drop glue #873

Open
RalfJung opened this issue Aug 8, 2020 · 18 comments
Open

Document auto-generated drop glue #873

RalfJung opened this issue Aug 8, 2020 · 18 comments

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 8, 2020

There should be a section in the reference which document how the compiler-generated drop glue behaves. It should answer questions like this one: rust-lang/unsafe-code-guidelines#225.

@RalfJung RalfJung changed the title Document auti-generated drop glue Document auto-generated drop glue Aug 8, 2020
@ehuss
Copy link
Contributor

ehuss commented Aug 10, 2020

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 rust-lang/unsafe-code-guidelines#225 (comment), 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.

@RalfJung
Copy link
Member Author

I mostly forwarded this issue from rust-lang/unsafe-code-guidelines#225, so @joshlf would be good if you could answer the question above. :)

@elichai
Copy link

elichai commented Jan 25, 2021

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)

@joshlf
Copy link
Contributor

joshlf commented Jan 25, 2021

@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.

@Havvy
Copy link
Contributor

Havvy commented Jan 25, 2021

@elichai Perhaps ask in the compiler channel on Zulip?

@Havvy
Copy link
Contributor

Havvy commented Jan 25, 2021

@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".

@bjorn3
Copy link
Member

bjorn3 commented Jan 25, 2021

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.

@QuineDot
Copy link

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.

@bjorn3
Copy link
Member

bjorn3 commented Jan 28, 2021

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.

@joshlf
Copy link
Contributor

joshlf commented Jan 28, 2021

@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.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 28, 2021

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 rust-lang/unsafe-code-guidelines#243, rust-lang/unsafe-code-guidelines#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.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 28, 2021

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?

@joshlf
Copy link
Contributor

joshlf commented Jan 28, 2021

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.

@RalfJung
Copy link
Member Author

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.

@joshlf
Copy link
Contributor

joshlf commented Jan 28, 2021

Ah, understood. I guess I've been using "effect" and "observable" as interchangeable.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 28, 2021

So, as far as I know, the drop glue for enums 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?

@joshlf
Copy link
Contributor

joshlf commented Sep 18, 2022

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.

@NicMcPhee
Copy link

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:

  • The term "drop glue" was used more, or less, in the documentation
    • If it was used more, that might help with both searchability and the usefulness of those search results.
    • If it was used less (i.e., none?) then that might reduce the likelihood of the term being used in general discourse.
  • There was documentation & examples (perhaps as part of the documentation of destructors) of how -> 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.
  • Error messages when -> 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants