Refactor CTFE and const-prop machines to share common code

[vakaras]

Miri, CTFE, and const-prop are implemented by using a MIR interpreter that can be customized by implementing the Machine trait. Currently, the CTFE and const-prop machines have quite a bit of common code that is duplicated. It would be nice to reduce the code duplication.

One idea that could be used to reduce the code duplication would be to add a new trait CommonMachine and put all the common implementation into it:

pub trait CommonMachine<'mir, 'tcx>: Sized {

    /// Borrow the current thread's stack.
    fn stack(
        ecx: &'a InterpCx<'mir, 'tcx, Self>,
    ) -> &'a [Frame<'mir, 'tcx, (), ()>];

    /// Mutably borrow the current thread's stack.
    fn stack_mut(
        ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
    ) -> &'a mut Vec<Frame<'mir, 'tcx, (), ()>>;

    // Other methods that require custom implementations for the CTFE and const-prop machines.
}

impl<'mir, 'tcx, T: CommonMachine<'mir, 'tcx>> super::machine::Machine<'mir, 'tcx> for T {

    type MemoryKind = !;
    type PointerTag = ();
    type ExtraFnVal = !;

    type FrameExtra = ();
    type MemoryExtra = ();
    type AllocExtra = ();

    #[inline(always)]
    fn enforce_alignment(_memory_extra: &Self::MemoryExtra) -> bool {
        false
    }

    /// Borrow the current thread's stack.
    fn stack(
        ecx: &'a InterpCx<'mir, 'tcx, Self>,
    ) -> &'a [Frame<'mir, 'tcx, (), ()>] {
        CommonMachine::stack(ecx)
    }

    /// Mutably borrow the current thread's stack.
    fn stack_mut(
        ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
    ) -> &'a mut Vec<Frame<'mir, 'tcx, (), ()>> {
        CommonMachine::stack_mut(ecx)
    }

    // TODO: other methods
}

[RalfJung]

I propose ConstMachine or so as trait name.

I wonder if there is a way to also share the stack field between the two? Though given how simple these two getters are it's probably not worth it...

[samrat]

Hi, I am interested in working on this issue.

[vakaras]

Hi, I am interested in working on this issue.

@samrat Perfect! If you have any questions, just post them here.

[LeSeulArtichaut]

@rustbot assign @samrat

[samrat]

@vakaras Since the methods you reference in the issue description are added in #70598, I'm curious-- is there a specific reason to do this refactoring on top of that PR? I was planning on working off of master otherwise.

[vakaras]

@samrat I would suggest for you to work on top of master because it is not clear if that PR will land before you finish your changes.

[samrat]

Hi, I don't have things working quite yet. But wanted to ask a few questions and get some feedback on what I have so far(samrat@1f1820c). (I have called the common trait ConstMachine as @RalfJung suggested)

This is my understanding of what needs to be done as part of the refactoring:

• const_prop and const_eval machines will implement ConstMachine. There will be an impl so anything that implements the ConstMachine trait also implements the interpret::Machine trait.
• ConstMachine will have default implementation fn's for common code. Otherwise just the signatures.
• For fn's that has to differ between const_prop and const_eval, the ConstMachine impl's will differ between the two.

Questions:

• Introducing ConstMachine brings in quite a bit of duplication in the form of fn signatures that exist in both interpret::Machine and ConstMachine: samrat@1f1820c#diff-86bbb578ec97cf2c4fb01cd5d62c44ccR14 The common code that I can hoist up to default ConstMachine fn's seem to be just one- or two-liners. Is there anyway I can avoid this, or is this something we'll have to live with?

• What do we do about fn's that has a default interpret::Machine fn but only one of const_prop or const_eval need to have a custom implementation of? Currently I've dealt with this by having the default ConstMachine fn be a copy of the interpret::Machine fn like so: samrat@1f1820c#diff-86bbb578ec97cf2c4fb01cd5d62c44ccR108

• Currently I have things compiling with the new trait. However, it seems I've broken const_prop as I get an error when trying to build libstd:

error: this operation will panic at runtime
   --> src/libstd/sys/unix/thread.rs:322:25
    |
322 |         let remainder = (stackaddr as usize) % PAGE_SIZE;
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to calculate the remainder with a divisor of zero
    |
    = note: `#[deny(unconditional_panic)]` on by default

It seems with my change, the interpreter now thinks the PAGE_SIZE const is zero. Do you have any advice on how I can go about debugging this?

[vakaras]

ConstMachine will have default implementation fn's for common code.

Also, ConstMachine can have more concrete types because most of the associated types are instantiated in the same way in both machines.

Introducing ConstMachine brings in quite a bit of duplication in the form of fn signatures that exist in both interpret::Machine and ConstMachine: samrat/rust@1f1820c#diff-86bbb578ec97cf2c4fb01cd5d62c44ccR14 The common code that I can hoist up to default ConstMachine fn's seem to be just one- or two-liners. Is there anyway I can avoid this, or is this something we'll have to live with?

I have not anticipated that we can achieve only so little reuse here; sorry, I should have checked this before opening the issue. One alternative maybe would be to put all shared code into the Machine trait so that we can avoid the duplication caused by adding the additional ConstMachine trait. However, I am not sure whether there are any serious drawbacks of this approach. @RalfJung: What is your opinion about this?

It seems with my change, the interpreter now thinks the PAGE_SIZE const is zero. Do you have any advice on how I can go about debugging this?

This looks really weird. I do not see how your changes could have led to this. However, you could use the approaches described here for figuring out what is being done to the PAGE_SIZE macro. For example, you could try getting a backtrace for the error or look at the logs produced by the compiler (note that to get the logs you need to add debug-assertions=yes to your config.toml).

[RalfJung]

Thanks a lot @samrat for giving this a shot!

One alternative maybe would be to put all shared code into the Machine trait so that we can avoid the duplication caused by adding the additional ConstMachine trait.

I don't think we can do that -- that code only works for particular values of these associated consts. IIRC we already have all assoc-type-independent code that can be shared in the Machine trait. (Well, except for the enforce_* methods. I guess we could move those.)

But I also did not expect this much duplication, this is helping much less than I expected. :/

I wonder if there is a way to use specialization to have less duplication here?

[RalfJung]

I think your current patch can be improved, but even with that, there is a lot of extra duplication of signatures, when avoiding signature duplication was the entire point of this exercise. Maybe it's just not worth it, at least with the techniques Rust currently offers.

[samrat]

Thanks @RalfJung and @vakaras. I'll table my efforts on this issue for now then-- do let me know if there are any other ideas to move forward with the issue.

[RalfJung]

That's okay, thanks for trying!

[vakaras]

@samrat Thank you for trying! And sorry for not checking properly how much benefit we can get here.

@RalfJung Shall we close this issue as undoable or would you like to keep it open?

[RalfJung]

I asked on Zulip if someone has an idea.

[RalfJung]

One rather crude but definitely working approach (suggested by @hanna-kruppe) would be to use a macro to share this code... not sure if @oli-obk would like that though.^^

[hanna-kruppe]

Let the record show that I am also skeptical whether this is a good idea, despite pointing out the possibility. I'm not very confident since I'm not familiar with any of the code in question, but this seems like one of those cases where LOC reduction is "penny-wise, pound-foolish" -- it does not seem to help prevent future bugs (only trivial logic and compiler-checked signatures are duplicated), while maintenance becomes harder if some previously-common code has to change in one impl but not in the other.

[RalfJung]

This issue is actually motivated by the maintenance burden that we currently have when changing the signatures of methods that are shared by both instances of the trait. Specifically, anything to do with pointers typically has a trivial implementation we want to use everywhere in the compiler, but because pointers might be tagged, we cannot just share that code. So specifically for this I feel like the macro would help reduce maintenance burden.

But I might be underestimating how annoying it would be to maintain that macro.