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

Make unused states of Reserved unrepresentable #3754

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Vanille-N
Copy link
Contributor

In the previous TB update we discovered that the existence of Reserved + !ty_is_freeze + protected is undesirable.

This has the side effect of making Reserved { conflicted: true, ty_is_freeze: false } unreachable.
As such it is desirable that this state would also be unrepresentable.

This PR eliminates the unused configuration by changing

enum PermissionPriv {
    Reserved { ty_is_freeze: bool, conflicted: bool },
    ...
}

into

enum PermissionPriv {
    ReservedFrz { conflicted: bool },
    ReservedIM,
    ...
}

but this is not the only solution and Reserved(Activable | Conflicted | InteriorMut) could be discussed.
In addition to making the unreachable state not representable anymore, this change has the nice side effect of enabling foreign_read to no longer depend explicitly on the protected flag.

Currently waiting for

  • @JoJoDeveloping to confirm that this is the same representation of Reserved as what is being implemented in simuliris,
  • @RalfJung to approve that this does not introduce too much overhead in the trusted codebase.

@JoJoDeveloping
Copy link
Contributor

confirm that this is the same representation of Reserved as what is being implemented in simuliris

Indeed, I can confirm:

image

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem obvious that merging the two states would make things so much simpler, so -- fine for me to split up the states like this.

@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 22, 2024
@Vanille-N
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jul 22, 2024
@Vanille-N
Copy link
Contributor Author

It was a good idea to add the assert!(!protected), it led me to discover a few more cases of exhaustive tests exploring too many initial states.

@RalfJung
Copy link
Member

Looks good, thanks. :)

Please squash the commits.

@RalfJung
Copy link
Member

Sorry, I missed this one since GH doesn't send notifications on force pushes.^^ Please always post a message in the PR when it is now waiting on me to do something again. :)

@bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2024

📌 Commit ae183fd has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 16, 2024

⌛ Testing commit ae183fd with merge eeb6e96...

@bors
Copy link
Contributor

bors commented Aug 16, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing eeb6e96 to master...

@bors bors merged commit eeb6e96 into rust-lang:master Aug 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants