Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 22, 2025

At least I think the old logic was wrong? Cc @JoJoDeveloping

The old logic was based on this comment:

/// For all non-accessed locations in the RangeMap (those that haven't had an
/// implicit read), their SIFA must be weaker than or as weak as the SIFA of
/// `default_perm`.

This should have said "must be at least as strong as", right? The invariant is that "the SIFA at each child must be stronger than that at the parent", so having stronger things in the new node should always be fine.

We should definitely have a test for this, if possible (but it may be that this doesn't actually lead to any bugs due to the implicit read fixing things up).

@rustbot
Copy link
Collaborator

rustbot commented Sep 22, 2025

Thank you for contributing to Miri!
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Sep 22, 2025
@RalfJung
Copy link
Member Author

RalfJung commented Sep 22, 2025

I think this is the actual assertion we needed before I added the max earlier today:

// Accessed nodes have the SIFA fixed up as part of the access. For everything else,
// let's ensure default_strongest_idempotent has the minimum SIFA so the fixup
// below does the right thing.
if !perm.accessed {
    assert!(default_strongest_idempotent <= perm.idempotent_foreign_access);
}

This assertion also passes. The thing we checked instead (default_strongest_idempotent >= perm.idempotent_foreign_access) seems just entirely pointless, but it also passed since for non-accessed nodes we in fact always have default_strongest_idempotent == perm.idempotent_foreign_access (because if a non-accessed node exists, then outside_perm is the same as nonfreeze_perm).

for (Range { start, end }, &perm) in
inside_perms.iter(Size::from_bytes(0), inside_perms.size())
{
assert!(perm.permission.is_initial());
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add an assert here that perm.idempotent_foreign_access == perm.permission.strongest_idempotent_foreign_access(protected)? I do not fully recall what sets perm.idempotent_foreign_access initially and since it's "far away" from this function it might be easy to miss that this is used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The constructor sets it and it's a private field so nobody outside this files can mess with it, but sure.

@JoJoDeveloping
Copy link
Contributor

Good catch!

@RalfJung RalfJung force-pushed the sifa branch 4 times, most recently from df00dee to 60b9bc1 Compare September 22, 2025 16:07
@RalfJung RalfJung enabled auto-merge September 22, 2025 16:09
@RalfJung RalfJung added this pull request to the merge queue Sep 22, 2025
Merged via the queue into rust-lang:master with commit 645c5fc Sep 22, 2025
13 checks passed
@RalfJung RalfJung deleted the sifa branch September 22, 2025 17:04
@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants