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

compiler: rewrite jump targets properly #643

Merged
merged 1 commit into from
Feb 12, 2020
Merged

compiler: rewrite jump targets properly #643

merged 1 commit into from
Feb 12, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Feb 6, 2020

Closes #630 .

Old implementation could view 0x62 byte in
a script as a JMP instruction irregardless of whether it is
a real opcode or a part of a parameter of another instruction.
In this commit instructions are decoded together with parameters
during jump label rewriting.

@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #643 into master will decrease coverage by 0.02%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
- Coverage   65.07%   65.04%   -0.03%     
==========================================
  Files         125      125              
  Lines       10569    10572       +3     
==========================================
- Hits         6878     6877       -1     
- Misses       3413     3416       +3     
- Partials      278      279       +1
Impacted Files Coverage Δ
pkg/vm/context.go 70.93% <0%> (-1.69%) ⬇️
pkg/compiler/codegen.go 86.6% <100%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc9ace6...3e84f2b. Read the comment docs.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

I'm not sure why do you need another Decode function? Just (re)using NewContext() and ctx.Next() should be enough, no?

pkg/compiler/compiler_test.go Outdated Show resolved Hide resolved
@fyrchik
Copy link
Contributor Author

fyrchik commented Feb 10, 2020

I'm not sure why do you need another Decode function? Just (re)using NewContext() and ctx.Next() should be enough, no?

I need to know how much bytes were actually read. Decode has somewhat lower level interface. And it seems cleaner to have it and reuse.

@roman-khimov
Copy link
Member

Hmm... I'm not convinced, I think it's easier to add some NextIP() method to Context and be done with it.

Old implementation could view 0x62 byte in
a script as a JMP instruction irregardless of whether it is
a real opcode or a part of a parameter of another instruction.
In this commit instructions are decoded together with parameters
during jump label rewriting.
@roman-khimov roman-khimov modified the milestone: v0.72.3 Feb 12, 2020
@roman-khimov roman-khimov merged commit bcff9fa into master Feb 12, 2020
@roman-khimov roman-khimov deleted the fix/jump branch February 12, 2020 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler Go smart contract compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compiler: replace jump targets correctly
2 participants