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

CTFE: select different code for compile-time vs run-time #80489

Closed
usbalbin opened this issue Dec 29, 2020 · 7 comments
Closed

CTFE: select different code for compile-time vs run-time #80489

usbalbin opened this issue Dec 29, 2020 · 7 comments
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...)

Comments

@usbalbin
Copy link
Contributor

usbalbin commented Dec 29, 2020

During #79684 I bumped into a problem that led @oli-obk and me on to a discussion. I am still not quite sure about what we discussed(the smart ideas were not mine). Please correct me on the things I have gotten wrong and I will update this post. I believe the gist of it is:

Some functions, like align_offset contains code that is impossible to evaluate in a const context, but the functionality does exist in miri. One potential solution for making align_offset a const fn would be to add an attribute, say #[rustc_const_intrinsic] that would make the function look like a const fn from the outside but would still be allowed to contain non-const code. When the function is called during runtime the contents of the function is executed as expected. However if called in a const context, the const evaluator would catch the call and instead invoke its special logic.

@usbalbin
Copy link
Contributor Author

usbalbin commented Dec 29, 2020

Some of the discussion from #79684

@oli-obk:

[...] align_offset was a big topic around CTFE some time ago, but we only ever ended up landing it in miri: https://github.com/rust-lang/miri/blob/510e62c9ea5c9da6c7b29ba89568456e035f70c6/src/shims/mod.rs#L36

So... before you can do this PR, we'd need to move the align_offset logic from miri to rustc and thus CTFE. This is not as easy as with other low level intrinsics, because align_offset is a lang_item which contains the runtime code, but different code from the function body will be run in the CTFE/miri case. While miri has this working already, and it would be simple to use the same trick as the one linked above, it would still not compile, because our checks would see non-const code in the const fn align_offset. So we would need to create a new system which makes a function const from the outside, but not const on the inside. I'm not sure about the extend of the work necessary to do that, but it's probably as simple as not const-checking the function body of lang items.

@usbalbin:

@oli-obk

So we would need to create a new system which makes a function const from the outside, but not const on the inside. I'm not sure about the extend of the work necessary to do that, but it's probably as simple as not const-checking the function body of lang items.

Would something like the following do?

compiler/rustc_mir/src/transform/check_consts/validation.rs:322

 if self.tcx.sess.opts.debugging_opts.unleash_the_miri_inside_of_you {
     self.tcx.sess.miri_unleashed_feature(span, gate);
     return;
 }
+
+let attributes = self.tcx.get_attrs(self.def_id().to_def_id());
+if self.tcx.sess.contains_name(&attributes, sym::lang) {
+    // Disable const check for lang items
+    // FIXME: What should be done here?
+    return;
+}

 let mut err = op.build_error(self.ccx, span);
 assert!(err.is_error())

Seems like a very powerful change (if it even works) with risk of accidental usage of non-const things? Might it make more sense to have a new attribute #[unleash_the_miri_inside_of_you_here] to explicitly mark where this behavior is wanted?

Just speculating, I am quite far outside my comfort zone, and don't understand any far reaching implications :)

@oli-obk:

Might it make more sense to have a new attribute #[unleash_the_miri_inside_of_you_here] to explicitly mark where this behavior is wanted?

laughing I love it. I would definitely accept a PR that adds this (forever and ever unstable) attribute, even if we don't end up using it for align_offset. As an initial step I would use it for the ptr_guaranteed_eq and ptr_guaranteed_ne intrinsics by making them lang items instead. This way we can get rid of the codegen_ssa implementation of these intrinsics, because runtime will just use the function body, while CTFE will just never load the body but run its own logic instead.

And yes, you found the right place to do this.

@usbalbin:

Would it even be possible to implement the proposed

pub fn const_eval_select<ARG, F, G, RET>(arg: ARG, called_in_const: F, called_at_rt: G) -> RET;
as an "unleashed" const fn?

@oli-obk:

good idea, that could be doable, but we'd need a Fn bound on G, not sure if that triggers some other checks.

@usbalbin:

I tested with this but i still get calling non-const function 'non_const_fn', however it does seem to notice the attribute.

let attributes = self.tcx.get_attrs(self.def_id().to_def_id());
if self.tcx.sess.contains_name(&attributes, sym::unleash_the_miri_inside_of_you_here) {
    // FIXME: What should be done here?
    println!("Yay I noticed the unleash_the_miri_inside_of_you_here attribute!!!");
    return;
}

Is there some code to add before the return to tell the compiler that everything is fine or is the instant return what tells it that all is fine?

Test Code
#![feature(rustc_attrs)]

fn main() {
    const FOO: bool = call_non_const_fn_unleashed();
    assert!(FOO);
}

#[unleash_the_miri_inside_of_you_here]
const fn call_non_const_fn_unleashed() -> bool {
    non_const_fn()
}

fn non_const_fn() -> bool { true }
Error from ./x.py test src/test/ui/unleash-the-miri-inside-of-you-here
---- [ui] ui/unleash-the-miri-inside-of-you-here/call-non-const-from-const-fn.rs stdout ----
normalized stdout:
Yay I noticed the unleash_the_miri_inside_of_you_here attribute!!!



The actual stdout differed from the expected stdout.
Actual stdout saved to /home/albin/rust/rustc/build/x86_64-unknown-linux-gnu/test/ui/unleash-the-miri-inside-of-you-here/call-non-const-from-const-fn/call-non-const-from-const-fn.stdout
normalized stderr:
error: any use of this value will cause an error
  --> $DIR/call-non-const-from-const-fn.rs:12:5
   |
LL |     const FOO: bool = call_non_const_fn_unleashed();
   |     ------------------------------------------------
...
LL |     non_const_fn()
   |     ^^^^^^^^^^^^^^
   |     |
   |     calling non-const function `non_const_fn`
   |     inside `call_non_const_fn_unleashed` at $DIR/call-non-const-from-const-fn.rs:12:5
   |     inside `FOO` at $DIR/call-non-const-from-const-fn.rs:5:23

@oli-obk:

We've reached a point in the design where I think we need to invoke @rust-lang/wg-const-eval and see what they think of it.

Is there some code to add before the return to tell the compiler that everything is fine or is the instant return what tells it that all is fine?

No, because it is already working! You did everything right, you can notice this because FOO actually is getting evaluated now, when before it errored before evaluation. The error you are seeing is an actual evaluation error with a backtrace and everything. We have a second safety net for catching calls to non-const fn during evaluation, and we don't want to change this. What we do want is to catch the call to call_con_const_fn_unleashed and not even evaluate its body, but run special code within the const evaluator.

Initially I thought we'd have to make such functions a lang item in addition to unleash_the_miri_inside_of_you_here, but I think I have an alternative idea that could work: we change the attribute to also take a string argument, so we have unleash_the_miri_inside_of_you_here(guaranteed_eq) and then fetch that attribute in

if let ty::InstanceDef::Item(def) = instance.def {

and if it's one of a list of things that we know (match on the symbol, you'll have to register the symbol in

), we do the special logic, and otherwise we go back and continue with the current
if !ecx.tcx.is_const_fn_raw(def.did) {

(which we'll be able to replace with your new scheme, too, which is great imo).

@usbalbin:

Ok, I'll try to wrap my head around that :)

But essentially you are saying that my changes this far removes the check before the actual const eval. So with the attribute on, the actual const eval gets to decide?

Would that mean that const eval would be happy with (ignoring the bounds and stuff for now)

#[unleash_the_miri_inside_of_you_here]
fn const_eval_select<ARG, F, G, RET>(arg: ARG, called_in_const: F, called_at_rt: G) -> RET {
    if this_is_run_in_const_eval() {
        called_in_const(arg)
    } else {
        called_at_rt(arg)
    }
}

since it never actually tries to call called_at_rt?


What we do want is to catch the call to call_con_const_fn_unleashed and not even evaluate its body, but run special code within the const evaluator.

Wait, so we are not just catching the call to non_const_fn, but the entire call to call_non_const_fn_unleashed? So then the const_eval_select thing would not work?

@oli-obk:

Wait, so we are not just catching the call to non_const_fn, but the entire call to call_non_const_fn_unleashed?

Yes. Otherwise we'd need another system to catch calls to non_const_fn, and this way we can just catch the call_non_const_fn_unleashed. If you need some preprocessing, you can do that ahead of time in a regular const fn that calls call_non_const_fn_unleashed.

So then the const_eval_select thing would not work?

I think you would write it as

 #[unleash_the_miri_inside_of_you_here(const_eval_select)]
fn const_eval_select<ARG, F, G, RET>(arg: ARG, called_in_const: F, called_at_rt: G) -> RET {
    called_at_rt(arg)
}

and then the CTFE engine has to process that function and can't just keep evaluating on.

But you are right, we could also do this without catching, if we made this_is_run_in_const_eval a unleash_the_miri_inside_of_you_here and caught only that and returned true from CTFE but the function is declare as

#[unleash_the_miri_inside_of_you_here]
fn this_is_run_in_const_eval() -> bool {
false
}
I guess we could do it this way, but it will require some care because we have never done function calls via generics in const eval before, I'm not sure what problems you would encounter. We should carefully try out all of these different use cases in individual PRs that make it easy to look at all effects of just the single change

@RalfJung:

We've reached a point in the design where I think we need to invoke @rust-lang/wg-const-eval and see what they think of it.

Is there a summary of the current proposal?

My first reaction upon skimming the discussion here is: I feel like this is RFC material. There's clearly a large design space, and having the design documented seems important. We don't want to accidentally subvert our CTFE safety net.

"unleash-miri" was meant as a never-stable hack that lets us test the safety net, I don't think it should become an attribute ever.

Sorry to be the bearer of bad news here, but I don't think having such important design discussions in a thread in a PR whose title is about something entirely different is a good way to go about this.

@usbalbin:

I fully agree :) This PR is probably far from a good place for the discussion. However I do not think I am anywhere near qualified to make an RFC on the subject. However I am here should there be anything I can help with :)

@usbalbin:

[..] Do you think it would would help if I made an experimental "dont-merge-PR" with the unleash_the_miri_inside_of_you_here changes(if I can find them again)? In that case, I could then copy paste the relevant comments from above into that PR. [..]

@oli-obk:

Maybe just open an issue for now that collects all the information from this PR. Anything related to const_eval_select definitely needs an RFC, I was getting a bit overexcited there. But I see no reason not to replace some of the existing intrinsics with the described scheme where a const fn's body is marked as not-const and thus does not exist for CTFE. Instead CTFE needs to provide a handler for it just like it does with intrinsics. Allowing us to write the non-CTFE body in Rust instead of in codegen library code will make this less fragile imo. Though it does reach the level where it needs at least a compiler MCP. We should also wait for #78407, since then we have an easy way to ensure we never accidentally leak a non-const body into CTFE. It simply won't have a mir_for_ctfe body to evaluate.

We should probably not name the attribute unleash_the_miri_inside_of_you as that's not what we want, but rather something like #[rustc_const_intrinsic] paired with #[lang_item = "some_name"], where the #[rust_const_intrinsic] causes the body not to be const checked and to not exist in mir_for_ctfe and the #[lang_item...] ensures we can hook the function call in CTFE to not even try to fetch a body but just invoke the custom logic.

@RalfJung RalfJung changed the title #[rustc_const_intrinsic] CTFE: select different code for compile-time vs run-time Dec 29, 2020
@RalfJung
Copy link
Member

Some functions, like align_offset contains code that is impossible to evaluate in a const context, but the functionality does exist in miri. One potential solution for making align_offset a const fn would be to add an attribute, say #[rustc_const_intrinsic] that would make the function look like a const fn from the outside but would still be allowed to contain non-const code. When the function is called during runtime the contents of the function is executed as expected. However if called in a const context, the const evaluator would catch the call and instead invoke its special logic.

align_offset is a bad example IMO since CTFE actually can't do anything here most of the time. There is no way to implement the equivalent logic during CTFE.

This feature should be mostly for cases where the logic is equivalent but run-time still has different code, e.g. using SIMD. And it can be used for debug assertions that should only be run at runtime (that was what triggered the discussion in the previous PR).

We should probably not name the attribute unleash_the_miri_inside_of_you as that's not what we want, but rather something like #[rustc_const_intrinsic] paired with #[lang_item = "some_name"], where the #[rust_const_intrinsic] causes the body not to be const checked and to not exist in mir_for_ctfe and the #[lang_item...] ensures we can hook the function call in CTFE to not even try to fetch a body but just invoke the custom logic.

Hm, wouldn't it be better if the CTFE logic also existed as Rust code, instead of having to implement it manually in the interpreter?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 29, 2020

wouldn't it be better if the CTFE logic also existed as Rust code, instead of having to implement it manually in the interpreter?

That's not possible in some cases. Take ptr_guaranteed_eq for example. There's no possible code you could write in Rust that the CTFE engine could evaluate, even with unleash_the_miri_inside_of_you enabled. For these we do need custom engine code. But you are right, there are many cases where we just want two different Rust code implementations. In order to easily support all such functions without excessive language support, we can then use the new scheme to write a function like the const_eval_select mentioned above, that will allow us to call different code at runtime and compile-time. This const_eval_select's body would call the runtime version, while the CTFE engine would have an implementation that calls the ctfe-version. From then on all functions that need support like this can just use const_eval_select and implement both versions in separate functions.

@RalfJung
Copy link
Member

In particular, if the CTFE version cannot be implemented in Rust, we can easily add an intrinsic there -- no need to have another "selection" mechanism.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 30, 2020

In particular, if the CTFE version cannot be implemented in Rust, we can easily add an intrinsic there -- no need to have another "selection" mechanism.

If we make it an intrinsic, we can't write the runtime version in Rust. Or am I misunderstanding what you mean by another selection mechanism? I want us to have the following schemes in the end:

  1. the current intrinsic scheme for when we need custom CTFE code and custom codegen for an operation
  2. a scheme that allows us to write the runtime code in Rust and only have to provide a CTFE handler
  3. Using 2. we implement a selection function that allows us to provide Rust code for both the runtime and the CTFE version and runs the appropriate version depending on what context the selection function is being used in.

@RalfJung
Copy link
Member

RalfJung commented Dec 30, 2020

If we make it an intrinsic, we can't write the runtime version in Rust.

We make the const part an intrinsic. Like

const_select!(runtime_code_in_rust(), ctfe_intrinsic());

So I think the underlying primitive should be one that simply takes Rust/MIR bodies both for the runtime and CTFE versions. That is most expressive and allows encoding the other cases.

@camelid camelid added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Dec 30, 2020
@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

Closing in favor of #124625.

@RalfJung RalfJung closed this as completed May 2, 2024
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, covers all const contexts (static, const fn, ...)
Projects
None yet
Development

No branches or pull requests

4 participants