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

#[unsafe_destructor] is excessively unsafe (monomorphizes wrong) #6971

Closed
ben0x539 opened this issue Jun 6, 2013 · 8 comments · Fixed by #9538
Closed

#[unsafe_destructor] is excessively unsafe (monomorphizes wrong) #6971

ben0x539 opened this issue Jun 6, 2013 · 8 comments · Fixed by #9538
Labels
A-codegen Area: Code generation A-destructors Area: Destructors (`Drop`, …) I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.

Comments

@ben0x539
Copy link
Contributor

ben0x539 commented Jun 6, 2013

struct S<T> { field: T }

#[unsafe_destructor]
impl<T> Drop for S<T> {
    fn drop(&self) {
        println(fmt!("drop %?", self));
    }
}

fn main() {
    let i = 7496034;
    let _s1 = S { field: (&i, 4) };
    let _s2 = S { field: "foo" };
}

prints

drop &{field: "foo"}
drop &{field: "bar"}

I suppose the clue is in the name of that attribute, but I figured this case is worth noting--I expected somewhat more subtle unsafety.

It does the right thing if the destructor refers to the field specifically.

@Aatch
Copy link
Contributor

Aatch commented Jun 8, 2013

Hmm, I do see your point, but I'm not sure if this is something we can actually fix. As far as I can tell, this is exactly why generic Drops are unsafe.

I'm not certain though, so I'll let somebody more knowledgeable than I chime in.

@bblum
Copy link
Contributor

bblum commented Jun 11, 2013

This looks suspiciously similar to #2734

@bblum
Copy link
Contributor

bblum commented Jun 11, 2013

There's not really any reason for generic drops to be unsafe. The reason they currently aren't is that because of #4301/#3167, destructors are limited to types that fulfill Owned. But that is too conservative of a requirement -- destructors for polymorphic structs should be fine because the destructor can't treat the generic data as anything that would let it build a cycle (whether as concrete pointers, or through calling typeclass methods).

But here the compiler appears to make the assumption that, because destructorful data shouldn't be polymorphic, it should never need to monomorphize finalize in multiple different ways. I don't think there's any theoretical difficulty here (no dynamic dispatch to drop should be needed), just more machinery needed.

@pnkfelix
Copy link
Member

Nominating for maturity milestone 2: backwards-compatible. (Though I could see it also getting thrown in the milestone 1 bin).

Here are the issues that I think this bug identifies:

  • As bblum points out, if we are going to support attaching a destructor to polymorphic structs at all (even if only via the unsafe_destructor attribute), we cannot monomorphize such destructors so eagerly.
  • As bblum also points out, we might be able to generalize the rules for what types have destructors to support a case like this, and thus not require the unsafe_destructor attribute in this instance.
  • We also need to document the unsafe_destructor attribute itself. For example, we need to determine whether it is meant for end-user code in general, or targeted mainly for internal libraries (and the documentation audience is adjusted accordingly). Usage of the attribute crept into the FFI tutorial (in a well-motivated example), but there is no accompanying explanation of the attribute (e.g. how it is to be used, in what manner it is unsafe, etc).

Also, I just want to chime in: That is a very nifty test case. :)

(we may want to create sub-issues for some/all of the bullets above; I just wanted to get them out of my head in the short term.)

@Aatch
Copy link
Contributor

Aatch commented Jul 17, 2013

I'd say "well-defined" is fine for this (or at least some related issue). It certainly seems like something that should be well defined.

@bblum
Copy link
Contributor

bblum commented Jul 17, 2013

Actually I think well-covered is the right milestone. (Anybody want to cast a vote for the other two?)

@catamorphism
Copy link
Contributor

De-nominating

@nikomatsakis
Copy link
Contributor

Will not be relevant once #8861 is complete

bors added a commit that referenced this issue Sep 27, 2013
This is broken, and results in poor performance due to the undefined
behaviour in the LLVM IR. LLVM's `mergefunc` is a *much* better way of
doing this since it merges based on the equality of the bytecode.

For example, consider `std::repr`. It generates different code per
type, but is not included in the type bounds of generics.

The `mergefunc` pass works for most of our code but currently hits an
assert on libstd. It is receiving attention upstream so it will be
ready soon, but I don't think removing this broken code should wait any
longer. I've opened #9536 about enabling it by default.

Closes #8651
Closes #3547
Closes #2537
Closes #6971
Closes #9222
flip1995 pushed a commit to flip1995/rust that referenced this issue Mar 25, 2021
Rustup

r? `@ghost`

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-destructors Area: Destructors (`Drop`, …) I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants