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

Enforce that bitvectors have non-negative indices always #723

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Conversation

Alasdair
Copy link
Collaborator

@Alasdair Alasdair commented Sep 24, 2024

This commit changes the type system so that in order for bitvector('n) to be well-formed, it must be the case that 'n >= 0. To avoid making this change extremely painful, it adds a new kind Nat, which can be used in types like:

forall ('n : Nat). int('n) -> bool

The trick is then to update kind inference so

forall 'n. bits('n) -> bool

now infers Nat rather than Int for 'n. Other than taking part this extra kind-inference Nat is desugared into Int and >= 0 constraint, so there is no significant difference in how the type system works.

This means that the vast majority of Sail code should work as is, including the Sail RISC-V spec with only very minor changes. It is still a breaking change as previously we allowed

let x : list(bits(-1)) = [||]

which is now forbidden. It worked previously as bits with a negative index was treated as an uninhabited type, so while it couldn't be constructed, it could still appear in type signatures like above.

Adding this extra rule requires some refactoring of the typing context, so now we always check that unexpanded types are wellformed, rather than only checking wellformedness of expanded types.

@Alasdair
Copy link
Collaborator Author

Should not be merged before riscv/sail-riscv#559, as this change makes undefined have stricter constraints for bitvectors.

Copy link

github-actions bot commented Sep 24, 2024

Test Results

   10 files  ±0     22 suites  ±0   0s ⏱️ ±0s
  701 tests ±0    701 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 181 runs   - 1  2 180 ✅  - 1  1 💤 ±0  0 ❌ ±0 

Results for commit 6799dfb. ± Comparison against base commit 5df9e7f.

♻️ This comment has been updated with latest results.

@tobiasgrosser
Copy link
Contributor

Should not be merged before riscv/sail-riscv#559, as this change makes undefined have stricter constraints for bitvectors.

This is merged already. 👍

@Alasdair
Copy link
Collaborator Author

I checked and everything is fine for sail-arm (at least the 9.4 model) and sail-riscv, but sail-cheri-riscv is pinned to an older commit of sail-riscv which doesn't typecheck with this change. I need to figure out the best way to proceed there.

I could make this a flag, either opt-in or opt-out, or we could submit a PR to that repository either with a quick patch fix to the version it's using or by doing a full update.

@tobiasgrosser
Copy link
Contributor

I checked and everything is fine for sail-arm (at least the 9.4 model) and sail-riscv, but sail-cheri-riscv is pinned to an older commit of sail-riscv which doesn't typecheck with this change. I need to figure out the best way to proceed there.

I could make this a flag, either opt-in or opt-out, or we could submit a PR to that repository either with a quick patch fix to the version it's using or by doing a full update.

@Alasdair, I am happy with either of these choices. Do you favour one in particular?

This requires some refactoring to the typing context, so now we always
check that unexpanded types are well-formed.
@Alasdair Alasdair force-pushed the bvnat branch 3 times, most recently from c8fb564 to 834ae04 Compare October 7, 2024 07:22
@Alasdair
Copy link
Collaborator Author

Alasdair commented Oct 7, 2024

I've added a flag for now, if the CI is happy I should be good to merge it.

Copy link
Contributor

@tobiasgrosser tobiasgrosser left a comment

Choose a reason for hiding this comment

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

Nice.

@Alasdair Alasdair merged commit e7148e2 into sail2 Oct 9, 2024
9 checks passed
@Alasdair Alasdair deleted the bvnat branch October 9, 2024 16:01
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.

2 participants