[TEST] Don't allow constant eval to access generator layouts #115359

Zoxc · 2023-08-29T21:02:19Z

This emits an error if generator layouts are observed by the frontend via constant evaluation. This ensures that the frontend does not depend on optimizations done later in MIR. This is a breaking change as generator layouts can be observed via the async feature combined with constant evaluation.

r? @oli-obk

rustbot · 2023-08-29T21:32:55Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rustbot · 2023-08-29T21:35:20Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2023-08-30T06:55:30Z

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -357,6 +357,9 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,

    const PANIC_ON_ALLOC_FAIL: bool = false; // will be raised as a proper error

+    // We don't allow access to generator layout at compile time.
+    const ACCESS_GENERATOR_LAYOUT: bool = false;


Please remove the default value here, and move it to the compile_time_machine macro below. After all this is also shared with Miri, so we don't set defaults here based on "being compile-time".

RalfJung · 2023-08-30T06:56:27Z

compiler/rustc_const_eval/src/interpret/step.rs

+
+                // Ensure we don't need the layout of any generators if applicable.
+                // This prevents typeck from depending on MIR optimizations.
+                self.validate_generator_layout_access(ty)?;


Why is this the one place where the layout is checked? That seems odd, aren't there tons of ways the layout could become relevant?

RalfJung · 2023-08-30T06:57:06Z

compiler/rustc_const_eval/src/interpret/validity.rs

+
+    /// This function checks if the given `ty`'s layout depends on generators.
+    #[inline(always)]
+    pub fn validate_generator_layout_access(&self, ty: Ty<'tcx>) -> InterpResult<'tcx> {


"validity.rs" is for checking that a value is valid. Please don't put unrelated functions here.

RalfJung · 2023-08-30T06:58:32Z

compiler/rustc_middle/src/mir/interpret/error.rs

@@ -404,6 +404,7 @@ pub enum ValidationErrorKind<'tcx> {
    InvalidBool { value: String },
    InvalidChar { value: String },
    InvalidFnPtr { value: String },
+    GeneratorLayoutAccess { ty: Ty<'tcx>, generators: &'tcx List<Ty<'tcx>> },


"ValidationErrorKind" is for when a value doesn't satisfy its validity invariant. This is unrelated, please don't put that here.

Sound more like an InvalidProgram error to me. Or ideally the error variant is only added in const_eval::error, so we structurally know it will not occur in Miri.

RalfJung · 2023-08-30T06:59:26Z

src/tools/miri/src/machine.rs

-            .allocation_spans
-            .borrow_mut()
-            .insert(alloc_id, (span, None));
+        ecx.machine.allocation_spans.borrow_mut().insert(alloc_id, (span, None));


Please don't format these files, they'll have to be reformatted when syncing changes back to Miri. (Miri has a different rustfmt.toml.)

Why doesn't rustfmt use those settings? :/

I think it's up to RA to tell it the config file to use (rustfmt isn't invoked on a file, it is fed data via stdin and returned via stdout). Not sure if there's an open RA bug for this.

EDIT: rust-lang/rust-analyzer#15540

It seems like RA changes the current directory to match the file for rustfmt. I guess that doesn't work out.

oli-obk · 2023-08-30T10:49:31Z

This is a breaking change as generator layouts can be observed via the async feature combined with constant evaluation.

do you have an example snippet that compiles on stable but doesn't compile anymore?

Zoxc · 2023-08-30T16:52:58Z

As @RalfJung pointed out, this isn't the only place layout is used. It would have apply more generally to all allocations, effectively banning async and generator blocks from const eval, at least while allocations are byte based. That's not a particularly great outcome.

@oli-obk I'll relay your example back to you:

const X: usize = {
    let x = Thing(|x: u128| async move {x});
    x.size()
};

struct Thing<T>(T);

impl<U, T: Fn(u128) -> U> Thing<T> {
    const fn size(&self) -> usize {
        std::mem::size_of::<U>()
    }
}

oli-obk · 2023-08-30T18:23:31Z

Oh whoops. I remember this.

Yea, this seems very annoying to prevent. I forget where the original discussion was. Why did we consider avoiding getting generator layouts during const eval? Avoiding some cycle errors?

Zoxc · 2023-08-30T18:42:27Z

Here's the discussion on Zulip.

The problem is that having const eval depend on generator layouts places constraints on optimization. We can't have an out of process LTO optimization of generator layouts for example. Just partially optimizing generator layouts within the query system gets quite hairy. I suspect there's some interesting interactions with monomorphization too.

oli-obk · 2023-08-30T19:01:09Z

Thanks! I reread the thread. Splitting Reveal::All into Optimizations and Codegen indeed seems like the only solution to do this properly. We can probably get away with letting const eval always use Codegen for its Allocations, but forward its real Reveal otherwise. layout_of with Reveal::Optimizations or UserFacing would then error in case it tries to compute the layout of a generator

RalfJung · 2023-08-30T21:04:09Z

Inevitably at some point people will ask to execute generators at compiletime. What do we do then?

I don't think doing LTO on pre-lowering generators is a realistic option.

oli-obk · 2023-09-01T14:38:19Z

The moment offset_of works on stable in const eval, I don't see how we could possibly hide the actual size of generators. Without offset_of, we can always do some shenanigans where the size of a generator is treated as something opaque just like pointers. That would be a lot of complexity, but it would be possible. Since this will break down once we have offset_of, there's no real point to it though.

the new Reveal mode would resolve the issue raised in the zulip, as all it would do is make optimizations before the generator lowering pass stop being able to rely on the size of generators (without causing errors, just by returning something akin to TooGeneric, which all optimizations must already silently handle)

RalfJung · 2023-09-01T14:53:18Z

https://github.com/Gilnaa/memoffset already works in const-eval when there is no interior mutability.

bors · 2023-09-13T18:21:47Z

☔ The latest upstream changes (presumably #115797) made this pull request unmergeable. Please resolve the merge conflicts.

apiraino · 2023-10-05T11:52:31Z

hey, checking progress. I'm unsure about the status of this PR (besides a few outstanding comments and a rebase). @Zoxc do you have any update? Are you specifically waiting on a feedback? Thanks!

@rustbot author

Zoxc · 2023-10-07T06:50:00Z

@oli-obk The problem with that approach is that we end up having that reveal option on anything that could use generator sizes, including type_of and type checking.

I don't think the method in this PR is a suitable solution however, so I'm going to close it.

This comment has been minimized.

Sign in to view

Don't allow constant eval to access generator layouts

0adc84b

Zoxc force-pushed the hide-gen-sizes branch from d4036a9 to 0adc84b Compare August 29, 2023 21:25

This comment has been minimized.

Sign in to view

rustbot assigned oli-obk Aug 29, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 29, 2023

RalfJung reviewed Aug 30, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2023

Zoxc closed this Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEST] Don't allow constant eval to access generator layouts #115359

[TEST] Don't allow constant eval to access generator layouts #115359

Zoxc commented Aug 29, 2023

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 29, 2023

rustbot commented Aug 29, 2023

RalfJung Aug 30, 2023

RalfJung Aug 30, 2023

RalfJung Aug 30, 2023

RalfJung Aug 30, 2023

RalfJung Aug 30, 2023

Zoxc Aug 31, 2023

RalfJung Aug 31, 2023 •

edited

Loading

Zoxc Aug 31, 2023

oli-obk commented Aug 30, 2023

Zoxc commented Aug 30, 2023

oli-obk commented Aug 30, 2023

Zoxc commented Aug 30, 2023

oli-obk commented Aug 30, 2023

RalfJung commented Aug 30, 2023

oli-obk commented Sep 1, 2023

RalfJung commented Sep 1, 2023

bors commented Sep 13, 2023

apiraino commented Oct 5, 2023

Zoxc commented Oct 7, 2023

[TEST] Don't allow constant eval to access generator layouts #115359

[TEST] Don't allow constant eval to access generator layouts #115359

Conversation

Zoxc commented Aug 29, 2023

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 29, 2023

rustbot commented Aug 29, 2023

RalfJung Aug 30, 2023

Choose a reason for hiding this comment

RalfJung Aug 30, 2023

Choose a reason for hiding this comment

RalfJung Aug 30, 2023

Choose a reason for hiding this comment

RalfJung Aug 30, 2023

Choose a reason for hiding this comment

RalfJung Aug 30, 2023

Choose a reason for hiding this comment

Zoxc Aug 31, 2023

Choose a reason for hiding this comment

RalfJung Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Zoxc Aug 31, 2023

Choose a reason for hiding this comment

oli-obk commented Aug 30, 2023

Zoxc commented Aug 30, 2023

oli-obk commented Aug 30, 2023

Zoxc commented Aug 30, 2023

oli-obk commented Aug 30, 2023

RalfJung commented Aug 30, 2023

oli-obk commented Sep 1, 2023

RalfJung commented Sep 1, 2023

bors commented Sep 13, 2023

apiraino commented Oct 5, 2023

Zoxc commented Oct 7, 2023

RalfJung Aug 31, 2023 •

edited

Loading