Skip to content
This repository has been archived by the owner on May 11, 2020. It is now read-only.

exec: fix div instruction #186

Merged
merged 9 commits into from
Mar 13, 2020
Merged

exec: fix div instruction #186

merged 9 commits into from
Mar 13, 2020

Conversation

laizy
Copy link
Member

@laizy laizy commented Feb 11, 2020

@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #186 into master will decrease coverage by 0.73%.
The diff coverage is 40.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
- Coverage   69.77%   69.03%   -0.74%     
==========================================
  Files          49       51       +2     
  Lines        5495     5910     +415     
==========================================
+ Hits         3834     4080     +246     
- Misses       1318     1467     +149     
- Partials      343      363      +20
Impacted Files Coverage Δ
exec/vm.go 80.13% <0%> (-3.14%) ⬇️
wasm/module.go 72.88% <100%> (ø) ⬆️
wasm/types.go 46.15% <100%> (+0.32%) ⬆️
wasm/index.go 58.55% <33.33%> (-3.32%) ⬇️
exec/call.go 76.92% <50%> (-5.69%) ⬇️
exec/num.go 98.67% <60%> (-1.33%) ⬇️
exec/memory.go 84.42% <66.66%> (-1.9%) ⬇️
wast/write.go 70.15% <0%> (ø) ⬆️
wast/scanner.go 55.64% <0%> (ø)
wast/token.go 65% <0%> (ø)
... and 1 more

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 8dd99d5...b4f9a91. Read the comment docs.

@laizy
Copy link
Member Author

laizy commented Feb 13, 2020

@sbinet ,review?

Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

thanks (and apologies for the belated answer)

could we try to add a test for that?
either update our copy (?) of the testsuite or add a dedicated .wasm file.

thanks again.

@laizy
Copy link
Member Author

laizy commented Feb 13, 2020

No rush to merge. we have build a test runner for the spec testsuite in our forked repo,
and found some other issues. We will submit the fixes with the test runner code soon.

# Conflicts:
#	exec/memory.go
# Conflicts:
#	.gitignore
#	wasm/calibration.go
# Conflicts:
#	tests/spectestcase
#	wasm/index.go
@sbinet
Copy link
Contributor

sbinet commented Feb 26, 2020

ping?

zxh0 and others added 2 commits March 3, 2020 16:28
# Conflicts:
#	exec/num_test.go
@laizy
Copy link
Member Author

laizy commented Mar 3, 2020

@sbinet sorry for late reply. Have appended commits for test runner and some bug fix.

@@ -0,0 +1,3 @@
[submodule "tests/spectestcase"]
path = tests/spectestcase
url = https://github.com/ontio/testsuite
Copy link
Member Author

Choose a reason for hiding this comment

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

Our forked testsuite disabled all testcases related to floating-point operation and I do not have enough time to check that part. We can replace it with https://github.com/WebAssembly/testsuite and fix the floating-point operation in other PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems we've grown too many "testsuite" datasets over time...

perhaps not for this PR, but it would be great to streamline a bit all of those...

@@ -0,0 +1,3 @@
[submodule "tests/spectestcase"]
path = tests/spectestcase
url = https://github.com/ontio/testsuite
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems we've grown too many "testsuite" datasets over time...

perhaps not for this PR, but it would be great to streamline a bit all of those...

exec/memory.go Show resolved Hide resolved
exec/vm.go Outdated Show resolved Hide resolved
exec/vm.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
tests/run_testcase.sh Outdated Show resolved Hide resolved
tests/run_testcase.sh Outdated Show resolved Hide resolved
tests/spec_test_runner.go Show resolved Hide resolved
}
}

func main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't we pre-generate the .json files (via a go generate command) and make this run as part of the usual testing.T infrastructure?

wasm/module.go Show resolved Hide resolved
wasm/module.go Outdated Show resolved Hide resolved
Co-Authored-By: Sebastien Binet <binet@cern.ch>
@sbinet sbinet merged commit dc6758c into go-interpreter:master Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants