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

cannon: Rework RMW ops for 64-bit compatibility #12419

Merged
merged 18 commits into from
Oct 17, 2024

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented Oct 10, 2024

Description

Rework RMW ops for 64-bit compatibility.

Changes include:

  • State.LLAddress now holds the requested address rather than the effective address (which has the lower bits zeroed out). It is necessary to retain some of the less significant bits in order to ensure that LL and SC operations are targeting the same 32-bit value when executing on 64-bit architectures.
  • bool State.LLReservationActive has been changed to uint8 State. LLReservationStatus. This allows us to store the type of reservation that was made (LL operations load a 32-bit value, LLD operations load a 64-bit value) so that the SC (32-bit) and SCD (64-bit) operations only succeed if a matching reservation has been made.

Tests

  • Updated existing ll, sc tests to work for 32- and 64-bit architectures
  • Added 64-bit specific tests

Metadata
Fixes #12242

@mbaxter mbaxter force-pushed the issue-12242/rmw-64bit branch from 003444b to b36309c Compare October 14, 2024 17:30
@mbaxter mbaxter force-pushed the issue-12242/rmw-64bit branch from b36309c to 4b307c3 Compare October 14, 2024 17:31
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 80.32787% with 24 lines in your changes missing coverage. Please review.

Project coverage is 64.86%. Comparing base (9d9dc32) to head (c0f9463).
Report is 36 commits behind head on develop.

Files with missing lines Patch % Lines
cannon/mipsevm/testutil/arch.go 56.52% 9 Missing and 1 partial ⚠️
cannon/mipsevm/multithreaded/mips.go 75.00% 5 Missing and 1 partial ⚠️
cannon/mipsevm/testutil/mips.go 0.00% 4 Missing and 1 partial ⚠️
cannon/mipsevm/multithreaded/state.go 50.00% 0 Missing and 2 partials ⚠️
cannon/mipsevm/testutil/state.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12419      +/-   ##
===========================================
+ Coverage    64.60%   64.86%   +0.25%     
===========================================
  Files           52       54       +2     
  Lines         4371     4460      +89     
===========================================
+ Hits          2824     2893      +69     
- Misses        1372     1391      +19     
- Partials       175      176       +1     
Flag Coverage Δ
cannon-go-tests 64.86% <80.32%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cannon/mipsevm/exec/mips_instructions.go 74.18% <100.00%> (+1.96%) ⬆️
...non/mipsevm/multithreaded/testutil/expectations.go 97.40% <100.00%> (+0.12%) ⬆️
cannon/mipsevm/multithreaded/testutil/mutators.go 79.59% <100.00%> (ø)
cannon/mipsevm/multithreaded/testutil/thread.go 99.18% <100.00%> (+0.02%) ⬆️
cannon/mipsevm/testutil/memory.go 100.00% <100.00%> (ø)
cannon/mipsevm/testutil/state.go 78.26% <0.00%> (+4.34%) ⬆️
cannon/mipsevm/multithreaded/state.go 54.24% <50.00%> (ø)
cannon/mipsevm/testutil/mips.go 85.06% <0.00%> (-2.86%) ⬇️
cannon/mipsevm/multithreaded/mips.go 93.48% <75.00%> (-0.48%) ⬇️
cannon/mipsevm/testutil/arch.go 56.52% <56.52%> (ø)

... and 1 file with indirect coverage changes

@ethereum-optimism ethereum-optimism deleted a comment from KADEFA2210 Oct 14, 2024
cannon/mipsevm/exec/mips_instructions.go Show resolved Hide resolved
cannon/mipsevm/memory/memory.go Outdated Show resolved Hide resolved
cannon/mipsevm/testutil/mips.go Show resolved Hide resolved
@mbaxter mbaxter requested a review from Inphi October 14, 2024 20:40
@mbaxter mbaxter marked this pull request as ready for review October 14, 2024 20:40
@mbaxter mbaxter requested review from a team as code owners October 14, 2024 20:40
cannon/mipsevm/multithreaded/mips.go Show resolved Hide resolved
cannon/mipsevm/memory/memory.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

LGTM! I appreciate the test for backwards-compatibility for 32-bit instructions on cannon64.
Just a couple nits.

cannon/mipsevm/tests/evm_multithreaded_test.go Outdated Show resolved Hide resolved
cannon/mipsevm/tests/evm_multithreaded_test.go Outdated Show resolved Hide resolved
cannon/mipsevm/memory/memory.go Outdated Show resolved Hide resolved
@mbaxter mbaxter added this pull request to the merge queue Oct 17, 2024
Merged via the queue into ethereum-optimism:develop with commit cc0959d Oct 17, 2024
58 checks passed
@mbaxter mbaxter deleted the issue-12242/rmw-64bit branch October 17, 2024 14:06
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
)

* cannon: Use 4-byte alignment for memory.SetUint32

* cannon: For now, bypass EVM check for 64-bit tests

* cannon: Rework rmw ops for Cannon64 compatibility

TODO: Fix MIPS 64-bit tests

* cannon: Add some comments

* cannon: Make TestEVM_MT_LL 64-bit compatible

* cannon: Make SC tests 64-bit compatible

* cannon: Clean up unused test param

* cannon: Add 64-bit tests of ll/sc ops, fix sign extension

* cannon: Add tests for lld, scd ops

* cannon: Rework test utils

* cannon: Update state field LLReservationActive to LLReservationStatus

Use the status field to track what type of reservation was made.

* cannon: Update MIPS2 version, run semver lock

* cannon: Rename go var to match solidity

* cannon: Tweak test descriptions

* Revert "cannon: Use 4-byte alignment for memory.SetUint32"

This reverts commit ce9abdd.

* cannon: Rework 64-bit compatibility in tests

Also, clean-up pc setting

* cannon: Add test cases that clear unaligned LLAddress reservation

* Fix testcase descriptions

Co-authored-by: Inphi <[email protected]>

---------

Co-authored-by: Inphi <[email protected]>
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.

ll/sc and lld/scd 64-bit emulation
2 participants