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

vm: missing tests for CALL* instructions #833

Merged
merged 2 commits into from
Apr 9, 2020
Merged

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Apr 7, 2020

Add missing tests for:

  • CALL* VM instructions
  • Bit and numeric instructions

@AnnaShaleva AnnaShaleva self-assigned this Apr 7, 2020
@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #833 into master will increase coverage by 0.67%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   67.68%   68.36%   +0.67%     
==========================================
  Files         141      141              
  Lines       13175    13175              
==========================================
+ Hits         8918     9007      +89     
+ Misses       3849     3756      -93     
- Partials      408      412       +4     
Impacted Files Coverage Δ
pkg/vm/context.go 69.14% <0.00%> (+4.25%) ⬆️
pkg/vm/vm.go 86.70% <0.00%> (+8.55%) ⬆️

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 b2c767e...9f0bc2a. Read the comment docs.

pkg/vm/vm_test.go Outdated Show resolved Hide resolved
pkg/vm/vm_test.go Outdated Show resolved Hide resolved
pkg/vm/vm_test.go Outdated Show resolved Hide resolved
pkg/vm/vm_test.go Outdated Show resolved Hide resolved
Added tests for:
  - bit operatoins: AND, OR, XOR
  - numeric operations: BOOLOR, MIN, MAX, WITHIN, NEGATE
@AnnaShaleva AnnaShaleva added vm VM tasks/bugs/issues test Unit tests labels Apr 7, 2020
@AnnaShaleva AnnaShaleva changed the title vm: missing tests for CALL instructions vm: missing tests for CALL* instructions Apr 7, 2020
@AnnaShaleva AnnaShaleva force-pushed the test/vm_calls branch 2 times, most recently from 97f4188 to f44e621 Compare April 7, 2020 18:33
pkg/vm/vm_test.go Outdated Show resolved Hide resolved
@AnnaShaleva AnnaShaleva force-pushed the test/vm_calls branch 2 times, most recently from 800436c to ad39973 Compare April 8, 2020 11:09
@AnnaShaleva AnnaShaleva marked this pull request as ready for review April 8, 2020 11:10
}

func TestCALLI(t *testing.T) {
prog := []byte{byte(opcode.PUSHBYTES2), byte(opcode.Opcode(uint16(1))), byte(opcode.Opcode(uint16(1) >> 8)), // little-endian
Copy link
Member

Choose a reason for hiding this comment

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

byte(opcode.Opcode(uint16(1))) is just a 1, really. And it makes no sense doing uint16(1) >> 8 as it's just a 0.

byte(opcode.TOALTSTACK), byte(opcode.DUPFROMALTSTACK),
byte(opcode.JMPIF), byte(0x4), byte(0), byte(opcode.RET),
byte(opcode.FROMALTSTACK), byte(opcode.DEC),
byte(opcode.CALLI), byte(0), byte(1), 0xF8, 0xFF} // -8 -> JMP to TOALTSTACK)
Copy link
Member

Choose a reason for hiding this comment

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

This won't jump to TOALTSTACK, rather it will jump to JMPIF.

And I'd suggest something like this to test RV and stack isolation properly.

  1. Check that alt stack is clean (it must panic on FROMALTSTACK):
PUSH1
DUP
TOALTSTACK
JMP 0x2 /* to CALLI */
FROMALTSTACK
RET
CALLI {0, 0, -2} /* to FROMALTSTACK */
RET
  1. Check parameter and return values copying:
PUSH3
PUSH2
PUSH1
JMP 0x2 /* to CALLI */
SUB /* 2 - 1 */
RET
CALLI {1, 2, -4}  /* to SUB */
MUL /* 3 * 1 */
RET

It should run to completion and leave 3 on estack. Failing to copy pcount elements it would fail on SUB, doing it in wrong order it would compute -1 instead on 1, not returning rvcount elements it would fail on MUL, so it seems to be a nice test to me.

Estack isolation can also be checked with DEPTH as with CALLE tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I finally got how it works!

Copy link
Member Author

Choose a reason for hiding this comment

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

Added also DEPTH check

@AnnaShaleva AnnaShaleva force-pushed the test/vm_calls branch 2 times, most recently from abb49f8 to 5752c19 Compare April 9, 2020 08:16
@roman-khimov roman-khimov merged commit 54a9581 into master Apr 9, 2020
@roman-khimov roman-khimov deleted the test/vm_calls branch April 9, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Unit tests vm VM tasks/bugs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants