Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
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, covers all const contexts (static, const fn, ...) 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, covers all const contexts (static, const fn, ...) label Nov 18, 2021
@RalfJung RalfJung mentioned this issue Nov 23, 2021
Open
1 task
@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`
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) 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