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

ReplaceJumpWithRet #1159

Merged
merged 4 commits into from
Sep 8, 2024
Merged

ReplaceJumpWithRet #1159

merged 4 commits into from
Sep 8, 2024

Conversation

Hecate2
Copy link
Contributor

@Hecate2 Hecate2 commented Sep 5, 2024

If a JMP or JMP_L jumps to a RET, replace the JMP with RET

@Hecate2
Copy link
Contributor Author

Hecate2 commented Sep 5, 2024

This is the first time I add new instructions to the optimized contract, and the added RET is not binded to source codes with debuginfo. For now this may not be a big problem, but in the future we may need to bind new instructions to source codes.
EDIT: this problem has been solved in the following commit 21c640f , but may not be the greatest solution.

@shargon
Copy link
Member

shargon commented Sep 5, 2024

This make harder the method detection, currently we only have one ret for each method

@Jim8y
Copy link
Contributor

Jim8y commented Sep 5, 2024

This make harder the method detection, currently we only have one ret for each method

do we have limitation on using ret? i dont know that.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Sep 5, 2024

This make harder the method detection, currently we only have one ret for each method

Are we now really restricting the count of RET and detecting methods using the only RET? I can write a robust method detection if needed. I can also abandon this PR and (slowly) write something else to ensure only a single RET exists in a method.

@roman-khimov
Copy link

Go compiler can generate as many RETs for a method as needed and typical Go style even encourages that. Method boundaries are usually tracked with manifests, it's just more reliable.

@@ -22,7 +22,7 @@ namespace Neo.Optimizer
{
static class Reachability
{
[Strategy(Priority = int.MaxValue)]
[Strategy(Priority = int.MaxValue - 16)]
Copy link
Contributor

Choose a reason for hiding this comment

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

a brief comment to explain what 16 means here would be great

Copy link
Contributor Author

@Hecate2 Hecate2 Sep 6, 2024

Choose a reason for hiding this comment

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

It means nothing. Just reserving spaces for other optimizers. We are not even using the Strategy attribute now. In the future we may sort all the [Strategy] methods and execute them (cyclically) ordered by Priority.

@Jim8y
Copy link
Contributor

Jim8y commented Sep 6, 2024

o

@shargon Hey shargon, any further detail about this?

@shargon
Copy link
Member

shargon commented Sep 6, 2024

It complicate the reverse engineering but maybe we should take care more avoid optimization instead of reversing

@Jim8y Jim8y merged commit 25278fc into neo-project:master Sep 8, 2024
4 checks passed
@Jim8y Jim8y deleted the delete-jmp-to-ret branch September 8, 2024 07:05
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.

4 participants