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

Fix match AccessType #687

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Fix match AccessType #687

merged 1 commit into from
Jan 15, 2025

Conversation

trdthg
Copy link
Contributor

@trdthg trdthg commented Jan 14, 2025

-                            Read()          => false,
+                            Read(_)         => false,

I don't know why the previous one could compile, it should be wrong, but after replacing ext_access_type with newtype, the compiler detected the error correctly.

union AccessType ('a : Type) = {
  Read      : 'a,
  Write     : 'a,
  ReadWrite : ('a, 'a),
  Execute   : unit
}

@Timmmm
Copy link
Collaborator

Timmmm commented Jan 14, 2025

Ah interesting. So the reason this wasn't an issue before is that Sail treats unit union variants specially:

union Foo = {
  A : unit,
  B : unit,
  C : unit,
  D : int,
}

function test(f : Foo) -> int =
  match f {
    // These are all ok for unit
    A(()) => 0,
    B() => 1,
    C(_) => 2,
    // But int requires _.
    D(_) => 3,
  }

So Read() would only work if ext_access_type is unit. Extended versions of the model are allowed to change it, and then Read() wouldn't work... except that the only extended version of the model that really exists and uses this is CHERI and that also replaces the entire riscv_vmem_pte.sail file too.

So yeah, we should change the Read() to Read(_) anyway, but I don't see any reason to use newtype?

Copy link

github-actions bot commented Jan 14, 2025

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit a9b6c0c. ± Comparison against base commit 70edcb0.

♻️ This comment has been updated with latest results.

@trdthg trdthg changed the title Use newtype for ext_access_type Fix match AccessType Jan 14, 2025
@trdthg
Copy link
Contributor Author

trdthg commented Jan 14, 2025

done

@jordancarlin jordancarlin added the will be merged Scheduled to be merged in a few days if nobody objects label Jan 15, 2025
@Timmmm Timmmm merged commit 6adb553 into riscv:master Jan 15, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will be merged Scheduled to be merged in a few days if nobody objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants