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.

Sign up for GitHub

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

Jump to bottom

Values that need to be dropped are promoted in const / static initializers #91009

Closed
tmiasko opened this issue Nov 18, 2021 · 9 comments · Fixed by #105085
Closed

Values that need to be dropped are promoted in const / static initializers #91009

tmiasko opened this issue Nov 18, 2021 · 9 comments · Fixed by #105085
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug.

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Nov 18, 2021

For example, promotion succeeds in the example below, although compilation nevertheless fails due to checks for live drops:

pub const fn id<T>(x: T) -> T { x }
pub const C: () = {
    let _: &'static _ = &String::new();
    // Promotion failed, two errors:
    // ERROR: destructors cannot be evaluated at compile-time
    // ERROR: temporary value dropped while borrowed

    let _: &'static _ = &id(&String::new());
    // Promoted. Only one error:
    // ERROR: destructors cannot be evaluated at compile-time

    let _: &'static _ = &std::mem::ManuallyDrop::new(String::new());
    // Promoted. No errors.
};

With const_precise_live_drops, checking for live drops is delayed until after the promotion, so the following example compiles successfully:

#![feature(const_precise_live_drops)]
pub const fn id<T>(x: T) -> T { x }
pub const C: () = {
    let _: &'static _ = &id(&String::new());
};

The check for live drops can also be passed by providing a const drop implementation. The following example compiles successfully as well:

#![feature(const_fn_trait_bound)]
#![feature(const_mut_refs)]
#![feature(const_trait_impl)]
struct Panic;
impl const Drop for Panic { fn drop(&mut self) { panic!(); } }
pub const fn id<T>(x: T) -> T { x }
pub const C: () = {
    let _: &'static _ = &id(&Panic);
};

@rustbot label A-const-eval
@tmiasko tmiasko added the C-bug Category: This is a bug. label Nov 18, 2021
@rustbot rustbot added the A-const-eval Area: constant evaluation (mir interpretation) label Nov 18, 2021
@RalfJung RalfJung mentioned this issue Nov 23, 2021
Open
@ecstatic-morse ecstatic-morse mentioned this issue Dec 1, 2021
Merged
@RalfJung
Copy link
Member

This is very strange. There is only one place in promotion where we treat a const different from regular functions:

rust/compiler/rustc_const_eval/src/transform/promote_consts.rs

Line 637 in c9dd1d9

let promote_all_const_fn = matches!(

and the only difference is whether we check for the presence of the #[rustc_promotable] attribute on a function before promoting it. But even if we add that attribute promotion still seems to behave differently inside const than inside main.

@oli-obk any idea what is going on? Could this be due to some difference in how MIR is generated for consts vs regular functions?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2022

Could this be due to some difference in how MIR is generated for consts vs regular functions?

I did remove the explicit difference where storage markers were often not generated at all.

What we still have is the fact that construct_const differs from construct_fn in that we don't wrap the const's body in a scope, but I don't see how that would affect anything, considering that manually putting it into a smaller scope inside the constant (by putting it into a shorter lived block), doesn't affect it.

We'll need to look at the actual MIR generated for these

@tmiasko
Copy link
Contributor Author

tmiasko commented Jul 14, 2022

The reason I described the issue as applying to const / static initializers, is that there are no existing rustc_promotable functions to demonstrate the problem. With custom rustc_promotable functions it is possible. Sorry if the title was misleading.

To be effective rustc_promotable also needs stability attributes.The attribute also need to be applied to all functions in the promoted code, not just the outermost one.

It was quite a bit of time since I looked at this, but as far as I remember the root problem was complete omission of check for live drops in MIR that construct call operands.

@RalfJung
Copy link
Member

RalfJung commented Jul 14, 2022 •
edited
Loading

MIR for this code, looking at the 001-000.PromoteTemps.before.mir in each case.

For the constant:

const C: () = {
    let mut _0: ();                      // return place in scope 0 at promo.rs:5:14: 5:16
    let mut _1: &&std::string::String;   // in scope 0 at promo.rs:6:25: 6:44
    let _2: &&std::string::String;       // in scope 0 at promo.rs:6:25: 6:44
    let _3: &std::string::String;        // in scope 0 at promo.rs:6:26: 6:44
    let mut _4: &std::string::String;    // in scope 0 at promo.rs:6:29: 6:43
    let _5: &std::string::String;        // in scope 0 at promo.rs:6:29: 6:43
    let _6: std::string::String;         // in scope 0 at promo.rs:6:30: 6:43
    let mut _7: &std::mem::ManuallyDrop<std::string::String>; // in scope 0 at promo.rs:10:25: 10:68
    let _8: &std::mem::ManuallyDrop<std::string::String>; // in scope 0 at promo.rs:10:25: 10:68
    let _9: std::mem::ManuallyDrop<std::string::String>; // in scope 0 at promo.rs:10:26: 10:68
    let mut _10: std::string::String;    // in scope 0 at promo.rs:10:54: 10:67
    scope 1 {
        scope 2 {
        }
    }

    bb0: {
        StorageLive(_1);                 // scope 0 at promo.rs:6:25: 6:44
        StorageLive(_2);                 // scope 0 at promo.rs:6:25: 6:44
        StorageLive(_3);                 // scope 0 at promo.rs:6:26: 6:44
        StorageLive(_4);                 // scope 0 at promo.rs:6:29: 6:43
        StorageLive(_5);                 // scope 0 at promo.rs:6:29: 6:43
        StorageLive(_6);                 // scope 0 at promo.rs:6:30: 6:43
        _6 = String::new() -> [return: bb1, unwind: bb8]; // scope 0 at promo.rs:6:30: 6:43
                                         // mir::Constant
                                         //   span: promo.rs:6:30: 6:41
                                         //   literal: Const { ty: fn() -> String {String::new}, val: Value(<ZST>) }
    }

    bb1: {
        _5 = &_6;                        // scope 0 at promo.rs:6:29: 6:43
        _4 = &(*_5);                     // scope 0 at promo.rs:6:29: 6:43
        _3 = id::<String>(move _4) -> [return: bb2, unwind: bb7]; // scope 0 at promo.rs:6:26: 6:44
                                         // mir::Constant
                                         //   span: promo.rs:6:26: 6:28
                                         //   literal: Const { ty: fn(&'static String) -> &'static String {id::<String>}, val: Value(<ZST>) }
    }

    bb2: {
        StorageDead(_4);                 // scope 0 at promo.rs:6:43: 6:44
        _2 = &_3;                        // scope 0 at promo.rs:6:25: 6:44
        _1 = &(*_2);                     // scope 0 at promo.rs:6:25: 6:44
        AscribeUserType(_1,  , UserTypeProjection { base: UserType(1), projs: [] }); // scope 0 at promo.rs:6:12: 6:22
        drop(_6) -> [return: bb3, unwind: bb8]; // scope 0 at promo.rs:6:44: 6:45
    }

    bb3: {
        StorageDead(_6);                 // scope 0 at promo.rs:6:44: 6:45
        StorageDead(_5);                 // scope 0 at promo.rs:6:44: 6:45
        StorageDead(_2);                 // scope 0 at promo.rs:6:44: 6:45
        StorageDead(_1);                 // scope 0 at promo.rs:6:44: 6:45
        StorageLive(_7);                 // scope 1 at promo.rs:10:25: 10:68
        StorageLive(_8);                 // scope 1 at promo.rs:10:25: 10:68
        StorageLive(_9);                 // scope 1 at promo.rs:10:26: 10:68
        StorageLive(_10);                // scope 1 at promo.rs:10:54: 10:67
        _10 = String::new() -> [return: bb4, unwind: bb8]; // scope 1 at promo.rs:10:54: 10:67
                                         // mir::Constant
                                         //   span: promo.rs:10:54: 10:65
                                         //   literal: Const { ty: fn() -> String {String::new}, val: Value(<ZST>) }
    }

    bb4: {
        _9 = ManuallyDrop::<String>::new(move _10) -> [return: bb5, unwind: bb6]; // scope 1 at promo.rs:10:26: 10:68
                                         // mir::Constant
                                         //   span: promo.rs:10:26: 10:53
                                         //   user_ty: UserType(3)
                                         //   literal: Const { ty: fn(String) -> ManuallyDrop<String> {ManuallyDrop::<String>::new}, val: Value(<ZST>) }
    }

    bb5: {
        StorageDead(_10);                // scope 1 at promo.rs:10:67: 10:68
        _8 = &_9;                        // scope 1 at promo.rs:10:25: 10:68
        _7 = &(*_8);                     // scope 1 at promo.rs:10:25: 10:68
        AscribeUserType(_7,  , UserTypeProjection { base: UserType(4), projs: [] }); // scope 1 at promo.rs:10:12: 10:22
        StorageDead(_8);                 // scope 1 at promo.rs:10:68: 10:69
        StorageDead(_7);                 // scope 1 at promo.rs:10:68: 10:69
        _0 = const ();                   // scope 0 at promo.rs:5:19: 12:2
        StorageDead(_9);                 // scope 1 at promo.rs:12:1: 12:2
        StorageDead(_3);                 // scope 0 at promo.rs:12:1: 12:2
        return;                          // scope 0 at promo.rs:5:1: 5:16
    }

    bb6 (cleanup): {
        drop(_10) -> bb8;                // scope 1 at promo.rs:10:67: 10:68
    }

    bb7 (cleanup): {
        drop(_6) -> bb8;                 // scope 0 at promo.rs:6:44: 6:45
    }

    bb8 (cleanup): {
        resume;                          // scope 0 at promo.rs:5:1: 5:16
    }
}

For main:

fn main() -> () {
    let mut _0: ();                      // return place in scope 0 at promo.rs:14:11: 14:11
    let mut _1: &&std::string::String;   // in scope 0 at promo.rs:15:25: 15:44
    let _2: &&std::string::String;       // in scope 0 at promo.rs:15:25: 15:44
    let _3: &std::string::String;        // in scope 0 at promo.rs:15:26: 15:44
    let mut _4: &std::string::String;    // in scope 0 at promo.rs:15:29: 15:43
    let _5: &std::string::String;        // in scope 0 at promo.rs:15:29: 15:43
    let _6: std::string::String;         // in scope 0 at promo.rs:15:30: 15:43
    let mut _7: &std::mem::ManuallyDrop<std::string::String>; // in scope 0 at promo.rs:19:25: 19:68
    let _8: &std::mem::ManuallyDrop<std::string::String>; // in scope 0 at promo.rs:19:25: 19:68
    let _9: std::mem::ManuallyDrop<std::string::String>; // in scope 0 at promo.rs:19:26: 19:68
    let mut _10: std::string::String;    // in scope 0 at promo.rs:19:54: 19:67
    scope 1 {
        scope 2 {
        }
    }

    bb0: {
        StorageLive(_1);                 // scope 0 at promo.rs:15:25: 15:44
        StorageLive(_2);                 // scope 0 at promo.rs:15:25: 15:44
        StorageLive(_3);                 // scope 0 at promo.rs:15:26: 15:44
        StorageLive(_4);                 // scope 0 at promo.rs:15:29: 15:43
        StorageLive(_5);                 // scope 0 at promo.rs:15:29: 15:43
        StorageLive(_6);                 // scope 0 at promo.rs:15:30: 15:43
        _6 = String::new() -> [return: bb1, unwind: bb8]; // scope 0 at promo.rs:15:30: 15:43
                                         // mir::Constant
                                         //   span: promo.rs:15:30: 15:41
                                         //   literal: Const { ty: fn() -> String {String::new}, val: Value(<ZST>) }
    }

    bb1: {
        _5 = &_6;                        // scope 0 at promo.rs:15:29: 15:43
        _4 = &(*_5);                     // scope 0 at promo.rs:15:29: 15:43
        _3 = id::<String>(move _4) -> [return: bb2, unwind: bb7]; // scope 0 at promo.rs:15:26: 15:44
                                         // mir::Constant
                                         //   span: promo.rs:15:26: 15:28
                                         //   literal: Const { ty: fn(&'static String) -> &'static String {id::<String>}, val: Value(<ZST>) }
    }

    bb2: {
        StorageDead(_4);                 // scope 0 at promo.rs:15:43: 15:44
        _2 = &_3;                        // scope 0 at promo.rs:15:25: 15:44
        _1 = &(*_2);                     // scope 0 at promo.rs:15:25: 15:44
        AscribeUserType(_1,  , UserTypeProjection { base: UserType(1), projs: [] }); // scope 0 at promo.rs:15:12: 15:22
        drop(_6) -> [return: bb3, unwind: bb8]; // scope 0 at promo.rs:15:44: 15:45
    }

    bb3: {
        StorageDead(_6);                 // scope 0 at promo.rs:15:44: 15:45
        StorageDead(_5);                 // scope 0 at promo.rs:15:44: 15:45
        StorageDead(_2);                 // scope 0 at promo.rs:15:44: 15:45
        StorageDead(_1);                 // scope 0 at promo.rs:15:44: 15:45
        StorageLive(_7);                 // scope 1 at promo.rs:19:25: 19:68
        StorageLive(_8);                 // scope 1 at promo.rs:19:25: 19:68
        StorageLive(_9);                 // scope 1 at promo.rs:19:26: 19:68
        StorageLive(_10);                // scope 1 at promo.rs:19:54: 19:67
        _10 = String::new() -> [return: bb4, unwind: bb8]; // scope 1 at promo.rs:19:54: 19:67
                                         // mir::Constant
                                         //   span: promo.rs:19:54: 19:65
                                         //   literal: Const { ty: fn() -> String {String::new}, val: Value(<ZST>) }
    }

    bb4: {
        _9 = ManuallyDrop::<String>::new(move _10) -> [return: bb5, unwind: bb6]; // scope 1 at promo.rs:19:26: 19:68
                                         // mir::Constant
                                         //   span: promo.rs:19:26: 19:53
                                         //   user_ty: UserType(3)
                                         //   literal: Const { ty: fn(String) -> ManuallyDrop<String> {ManuallyDrop::<String>::new}, val: Value(<ZST>) }
    }

    bb5: {
        StorageDead(_10);                // scope 1 at promo.rs:19:67: 19:68
        _8 = &_9;                        // scope 1 at promo.rs:19:25: 19:68
        _7 = &(*_8);                     // scope 1 at promo.rs:19:25: 19:68
        AscribeUserType(_7,  , UserTypeProjection { base: UserType(4), projs: [] }); // scope 1 at promo.rs:19:12: 19:22
        StorageDead(_8);                 // scope 1 at promo.rs:19:68: 19:69
        StorageDead(_7);                 // scope 1 at promo.rs:19:68: 19:69
        _0 = const ();                   // scope 0 at promo.rs:14:11: 21:2
        StorageDead(_9);                 // scope 1 at promo.rs:21:1: 21:2
        StorageDead(_3);                 // scope 0 at promo.rs:21:1: 21:2
        return;                          // scope 0 at promo.rs:21:2: 21:2
    }

    bb6 (cleanup): {
        drop(_10) -> bb8;                // scope 1 at promo.rs:19:67: 19:68
    }

    bb7 (cleanup): {
        drop(_6) -> bb8;                 // scope 0 at promo.rs:15:44: 15:45
    }

    bb8 (cleanup): {
        resume;                          // scope 0 at promo.rs:14:1: 21:2
    }
}

Looks... pretty similar? In fact a diff after removing comments says they are entirely identical.
When I diff the PromoteTemps.after, there are quite a few differences indeed.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jul 19, 2022

To be effective rustc_promotable also needs stability attributes.The attribute also need to be applied to all functions in the promoted code, not just the outermost one.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=3291dde278a238c06ce81db3decfb294

@RalfJung
Copy link
Member

RalfJung commented Jul 19, 2022 •
edited
Loading

Okay, so it is not just a const-context bug then. That code in main should not be accepted, at least the &id(&new_string()). The ManuallyDrop seems fine since it does not drop?

Looks like something is wrong with the handling of nested promotion, where we then fail to properly check for destructors. I never understood what happens with nested promotion...

@RalfJung RalfJung added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 15, 2022
@RalfJung
Copy link
Member

This seems pretty bad, we are promoting code in a way that has side-effects (drop should be run but isn't).

@eddyb @oli-obk any idea why this happens?

@eddyb
Copy link
Member

eddyb commented Nov 28, 2022

So this is the relevant code that bans &new_string() today:

rust/compiler/rustc_const_eval/src/transform/promote_consts.rs

Lines 219 to 257 in dd12cd6

// We cannot promote things that need dropping, since the promoted value
// would not get dropped.
if self.qualif_local::<qualifs::NeedsDrop>(place.local) {
return Err(Unpromotable);
}
Ok(())
}
_ => bug!(),
}
}
// FIXME(eddyb) maybe cache this?
fn qualif_local<Q: qualifs::Qualif>(&mut self, local: Local) -> bool {
if let TempState::Defined { location: loc, .. } = self.temps[local] {
let num_stmts = self.body[loc.block].statements.len();
if loc.statement_index < num_stmts {
let statement = &self.body[loc.block].statements[loc.statement_index];
match &statement.kind {
StatementKind::Assign(box (_, rhs)) => qualifs::in_rvalue::<Q, _>(
&self.ccx,
&mut |l| self.qualif_local::<Q>(l),
rhs,
),
_ => {
span_bug!(
statement.source_info.span,
"{:?} is not an assignment",
statement
);
}
}
} else {
let terminator = self.body[loc.block].terminator();
match &terminator.kind {
TerminatorKind::Call { .. } => {
let return_ty = self.body.local_decls[local].ty;
Q::in_any_value_of_ty(&self.ccx, return_ty)

The problem with that approach is that the check is once per candidate.
And so &id(&new_string()), only the type of id(&new_string()) is checked for Drop, even though validate_ref/validate_local gets called for both the temporary for new_string() andthe one forid(&new_string())`.

The safest thing would be to move that self.qualif_local::<qualifs::NeedsDrop> check into validate_local - while validate_ref would also work for the example in this issue, I'm not sure we want to be able to promote &id(NeedsDrop {...}.field) either, and validate_local would catch both.

@oli-obk oli-obk mentioned this issue Nov 30, 2022
Merged
@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 1, 2022

I'm not sure we want to be able to promote &id(NeedsDrop {...}.field) either, and validate_local would catch both.

Good point. That kind of code leads to even simpler reproducer:

struct NoisyDrop(&'static str);
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}
fn main() {
    let _: &'static _ = &&(NoisyDrop("drop!"), 0).1;
    // NosiyDrop is not dropped ...
}

@bors bors closed this as completed in f5c3dfd Dec 24, 2022
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 25, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
@bors
Auto merge of rust-lang#105085 - oli-obk:stop_promoting_all_the_thing…
1972be5 
…s, r=RalfJung

Stop promoting all the things

fixes rust-lang#91009

r? `@RalfJung`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stop promoting all the things oli-obk/rust
6 participants
@eddyb @RalfJung @oli-obk @apiraino @rustbot @tmiasko