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

DUPN and SWAPN in EOF only + EOF stack validation #788

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented Jan 10, 2024

Closes #538

Please take an extra look if the invoke specializations added are ok and have no undesired side effects, esp. the advanced counterpart. The compiler made me add the latter, even if it's not used.

@pdobacz pdobacz requested review from chfast and gumb0 January 10, 2024 09:25
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83e7a32) 97.91% compared to head (8458134) 97.91%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #788   +/-   ##
=======================================
  Coverage   97.91%   97.91%           
=======================================
  Files         110      110           
  Lines       10644    10670   +26     
=======================================
+ Hits        10422    10448   +26     
  Misses        222      222           
Flag Coverage Δ
blockchaintests 60.37% <0.00%> (+0.44%) ⬆️
statetests 62.35% <41.66%> (+0.13%) ⬆️
statetests-silkpre 25.82% <1.58%> (-0.02%) ⬇️
unittests 95.89% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
lib/evmone/advanced_analysis.hpp 100.00% <ø> (ø)
lib/evmone/advanced_instructions.cpp 100.00% <ø> (ø)
lib/evmone/baseline.cpp 100.00% <100.00%> (ø)
lib/evmone/baseline_instruction_table.cpp 100.00% <ø> (ø)
lib/evmone/eof.cpp 83.87% <100.00%> (+0.11%) ⬆️
lib/evmone/instructions.hpp 100.00% <100.00%> (ø)
test/unittests/eof_validation_test.cpp 100.00% <100.00%> (ø)
test/unittests/evm_eip663_dupn_swapn_test.cpp 100.00% <100.00%> (ø)

lib/evmone/eof.cpp Outdated Show resolved Hide resolved
@chfast
Copy link
Member

chfast commented Jan 10, 2024

@rodiazet please review because some things overlap with #742

@chfast chfast requested a review from rodiazet January 10, 2024 09:45
lib/evmone/advanced_analysis.hpp Outdated Show resolved Hide resolved
lib/evmone/eof.cpp Outdated Show resolved Hide resolved
@pdobacz pdobacz force-pushed the restrict-dupn-swapn branch 4 times, most recently from 76ad926 to 7bd98c5 Compare January 10, 2024 11:11
@chfast
Copy link
Member

chfast commented Jan 10, 2024

The compiler made me add the latter, even if it's not used.

I think the compiler generates these functions anyway, they are just not assigned to the instruction table.

lib/evmone/eof.cpp Outdated Show resolved Hide resolved
lib/evmone/eof.cpp Outdated Show resolved Hide resolved
test/unittests/eof_validation_test.cpp Outdated Show resolved Hide resolved
test/unittests/eof_validation_test.cpp Show resolved Hide resolved
@pdobacz pdobacz force-pushed the restrict-dupn-swapn branch 2 times, most recently from 2745c69 to bcedb55 Compare January 10, 2024 13:08
lib/evmone/instructions.hpp Outdated Show resolved Hide resolved
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

The "Fix" is a bit misleading. Maybe change the commit title to

Change the stack_height_required range to accommodate DUPN/SWAPN

Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

A test for legacy code where the result is EVMC_UNDEFINED_INSTRUCTION is missing.

@pdobacz
Copy link
Collaborator Author

pdobacz commented Jan 11, 2024

The "Fix" is a bit misleading. Maybe change the commit title to

Change the stack_height_required range to accommodate DUPN/SWAPN

I'm planning to squash. The commit was split out for the sake of review back&forth

A test for legacy code where the result is EVMC_UNDEFINED_INSTRUCTION is missing.

👍

@pdobacz
Copy link
Collaborator Author

pdobacz commented Jan 24, 2024

@rodiazet please review because some things overlap with #742

@rodiazet @chfast Folks, I took a look at #742 myself and it seems both PRs add an invoke overload, a different one. I will assume that the conflict will be easily resolvable and go ahead to merge this.

@pdobacz pdobacz merged commit efd64c9 into master Jan 24, 2024
25 checks passed
@pdobacz pdobacz deleted the restrict-dupn-swapn branch January 24, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict DUPN and SWAPN to EOF only
2 participants