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

Flow based const qualification is unsound when combined with mut references / raw pointer deref #90124

Closed
tmiasko opened this issue Oct 21, 2021 · 5 comments · Fixed by #90214
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. F-const_mut_refs `#![feature(const_mut_refs)]`

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Oct 21, 2021

For example, the constant C is presumed not to need drop (the example can be easily adapted to other qualifs & situations):

#![feature(const_mut_refs)]
#![feature(const_precise_live_drops)]
#![feature(const_swap)]

pub const C: Option<String> = {
    let mut x = None;
    let mut y = Some(String::new());
    let a = &mut x;
    let b = &mut y;
    std::mem::swap(a, b);
    std::mem::forget(y);
    x
};

pub const D: () = {
    C;
};

@rustbot label +A-const-eval +A-const-fn +F-const_mut_refs

@rustbot rustbot added A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. F-const_mut_refs `#![feature(const_mut_refs)]` labels Oct 21, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 21, 2021

There are some, inconveniently collapsed, comments, regarding indirect mutation and const qualification:

Looks like the final approach was to at least partially ignore the issue of indirect mutation, since mutable references were unsupported at that time.

@rust-lang/wg-const-eval

@oli-obk
Copy link
Contributor

oli-obk commented Oct 21, 2021

Quoting the first comment

The code that compares results with the previous pass does not belong in the final product, hence the draft PR status.

Additionally, I think I want to adopt a more conservative approach to handling indirect assignments during dataflow. Currently, we wait until an indirect assignment (e.g. opaque function call, custom Drop impl, or write through an indirection) to qualify all locals whose address has been borrowed and whose type can possibly contain that Qualif. Instead, I think we should assume that as soon as a mutable reference (or shared reference if the type has interior mutability) is taken to a local, it takes on all possible qualifiers for its type. This will simplify dataflow analysis, and is closer to the old code.

So, I thought that had happened, but it appears we lost it. Replacing the qualif of a borrowed place with the full type's qualif seems like the most reasonable approach. We don't need to be perfect with this analysis in the presence of references and raw pointers.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 22, 2021

Ouch. FYI this did get implemented:

needs_drop.get().contains(local) || self.indirectly_mutable(ccx, local, location)

There's just a bug somewhere. I could have sworn we had a test for const_mut_refs and qualifs together.

@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 22, 2021

The problem in that case, is that indirect mutation data flow is completely
separate of qualif data flow, i.e., we only union final results together
instead of incorporating indirect mutation at each step of the computation.

@ecstatic-morse
Copy link
Contributor

Ugh, you're correct of course. How embarrassing.

The problem manifests when we assign a local that has been mutated indirectly to one that has not been (in this case the "return place" of the const initializer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. F-const_mut_refs `#![feature(const_mut_refs)]`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants