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

Drop impl on type reverses order of field destruction #16492

Closed
lilyball opened this issue Aug 14, 2014 · 5 comments · Fixed by #16493
Closed

Drop impl on type reverses order of field destruction #16492

lilyball opened this issue Aug 14, 2014 · 5 comments · Fixed by #16493

Comments

@lilyball
Copy link
Contributor

When a type has no Drop impl, it fields appear to be destructed in order of declaration. When a type does have a Drop impl, that order is apparently reversed. This is very surprising.

Example:

#![feature(unsafe_destructor)]
#![allow(non_camel_case_types)]

struct DropMe_NoImpl {
    _one: DropMsg,
    _two: DropMsg,
    _three: DropMsg
}

struct DropMe_Impl {
    _one: DropMsg,
    _two: DropMsg,
    _three: DropMsg
}

impl Drop for DropMe_Impl {
    fn drop(&mut self) {
        println!("[IMPL] DropMe_Impl.Drop");
    }
}

struct DropMsg(&'static str);

impl Drop for DropMsg {
    fn drop(&mut self) {
        let DropMsg(msg) = *self;
        println!("{}", msg);
    }
}

pub fn main() {
    let hasImpl = DropMe_Impl {
        _one: DropMsg("[ONE] DropMe.Drop"),
        _two: DropMsg("[TWO] DropMe.Drop"),
        _three: DropMsg("[THREE] DropMe.Drop")
    };

    println!("Dropping DropMe_Impl:");
    drop(hasImpl);

    let noImpl = DropMe_NoImpl {
        _one: DropMsg("[ONE] DropMe.Drop"),
        _two: DropMsg("[TWO] DropMe.Drop"),
        _three: DropMsg("[THREE] DropMe.Drop")
    };

    println!("\nDropping DropMe_NoImpl:");
    drop(noImpl);
}

Prints:

Dropping DropMe_Impl:
[IMPL] DropMe_Impl.Drop
[THREE] DropMe.Drop
[TWO] DropMe.Drop
[ONE] DropMe.Drop

Dropping DropMe_NoImpl:
[ONE] DropMe.Drop
[TWO] DropMe.Drop
[THREE] DropMe.Drop
@lilyball
Copy link
Contributor Author

cc @cmr, who first hit this bug.

@lilyball
Copy link
Contributor Author

The LLVM IR for the Drop glue for DropMe_NoImpl:

define internal void @_ZN13DropMe_NoImpl14glue_drop.148117h571786c88b10fe1dE(%"struct.DropMe_NoImpl<[]>"*) unnamed_addr #3 {
entry-block:
  %1 = getelementptr inbounds %"struct.DropMe_NoImpl<[]>"* %0, i32 0, i32 0
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %1)
  %2 = getelementptr inbounds %"struct.DropMe_NoImpl<[]>"* %0, i32 0, i32 1
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %2)
  %3 = getelementptr inbounds %"struct.DropMe_NoImpl<[]>"* %0, i32 0, i32 2
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %3)
  ret void
}

The LLVM IR for the Drop glue for DropMe_Impl:

define internal void @_ZN11DropMe_Impl14glue_drop.146417h9f98bddab7694295E(%"struct.DropMe_Impl<[]>"*) unnamed_addr #3 {
entry-block:
  %1 = alloca { i8*, i32 }
  %2 = getelementptr inbounds %"struct.DropMe_Impl<[]>"* %0, i32 0, i32 3
  %3 = load i8* %2, !range !0
  %4 = trunc i8 %3 to i1
  br i1 %4, label %cond, label %next

next:                                             ; preds = %normal-return, %entry-block
  ret void

cond:                                             ; preds = %entry-block
  %5 = getelementptr inbounds %"struct.DropMe_Impl<[]>"* %0, i32 0, i32 0
  %6 = getelementptr inbounds %"struct.DropMe_Impl<[]>"* %0, i32 0, i32 1
  %7 = getelementptr inbounds %"struct.DropMe_Impl<[]>"* %0, i32 0, i32 2
  %8 = getelementptr inbounds %"struct.DropMe_Impl<[]>"* %0, i32 0, i32 3
  invoke void @_ZN16DropMe_Impl.Drop4drop20hf5d4e9dbec49bd1eCaaE(%"struct.DropMe_Impl<[]>"* noalias nocapture dereferenceable(80) %0)
          to label %normal-return unwind label %unwind_custom_

normal-return:                                    ; preds = %cond
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %7)
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %6)
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %5)
  br label %next

unwind_custom_:                                   ; preds = %cond
  %9 = landingpad { i8*, i32 } personality i32 (i32, i32, i64, %"struct.rustrt::libunwind::_Unwind_Exception<[]>[#9]"*, %"enum.rustrt::libunwind::_Unwind_Context<[]>[#9]"*)* @rust_eh_personality
          cleanup
  store { i8*, i32 } %9, { i8*, i32 }* %1
  br label %clean_custom_

resume:                                           ; preds = %clean_custom_
  %10 = load { i8*, i32 }* %1
  resume { i8*, i32 } %10

clean_custom_:                                    ; preds = %unwind_custom_
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %7)
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %6)
  call void @_ZN7DropMsg14glue_drop.146617h5eadde10e6a29588E(%"struct.DropMsg<[]>"* %5)
  br label %resume
}

I don't know why it's getting pointers to each field before calling drop(), they're not used until the cleanup, and they're cleaned up in reverse order. It's also, for some reason, grabbing a pointer to the drop flag (%8), even though it doesn't use it.

@brson
Copy link
Contributor

brson commented Aug 15, 2014

Nominating. Seems pretty strange.

@lilyball
Copy link
Contributor Author

I have an approved commit for this already, it's just failing on the pretty-rpass test at the moment.

@pcwalton
Copy link
Contributor

I'm OK with disabling the test. Going forward the pretty printer should mainly be used for compiler diagnostics and rustfmt should cover the reformatting use cases, IMO.

lilyball added a commit to lilyball/rust that referenced this issue Aug 15, 2014
When a struct implements Drop, its fields should still drop in
declaration order (just as they do when the struct does not implement
Drop).

Fixes rust-lang#16492.
bors added a commit that referenced this issue Aug 15, 2014
When a struct implements Drop, its fields should still drop in
declaration order (just as they do when the struct does not implement
Drop).

Fixes #16492.
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2024
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

Successfully merging a pull request may close this issue.

3 participants