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.

Sign up for GitHub

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

Use evmone for testing. #7010

Merged

chriseth merged 13 commits into develop from useevmone

Aug 8, 2019

Contributor

chriseth commented Jun 26, 2019

No description provided.

chriseth requested review from axic and chfast

June 26, 2019 16:43

chriseth force-pushed the useevmone branch from f0db3d6 to 649c07a Compare

June 26, 2019 16:49

chriseth commented

View reviewed changes

test/ExecutionFramework.cpp Outdated

+ evmc::result call(const evmc_message& msg) noexcept final
+ {
+ // TODO: Improve C API for result creation.

Contributor Author

chriseth Jun 26, 2019

I guess most of the tests are failing because this is not yet implemented.

chriseth force-pushed the useevmone branch 2 times, most recently from 39ff71c to 6b43716 Compare

June 26, 2019 21:22

stackenbotten commented Jun 26, 2019

There was an error when running chk_coding_style for commit 6b43716743cec6d8abb17ebc3605f4ee6083b79b:

Error: Trailing whitespace found:
 evmc/CHANGELOG.md:30: Disallow implicit conversion from C   `evmc::result` to `evmc_result` 
 evmc/CHANGELOG.md:31: causing unintendent premature releasing of resources. 
 evmc/CHANGELOG.md:44:- Added: 
 evmc/CHANGELOG.md:94: Integration of EVMC as a CMake subproject is easier because 
 evmc/cmake/cable/README.md:57:Start by including this file in your main `CMakeLists.txt` from the Cable 
 evmc/cmake/cable/README.md:58:submodule/subtree or any other location. The `bootstrap.cmake` must be included 
 evmc/docs/Host_Guide.md:10:The implementation can be done in object-oriented manner. 
 evmc/docs/Host_Guide.md:13:Moreover, each of the methods has a pointer to ::evmc_context 
 evmc/docs/Host_Guide.md:14:as a parameter. The context is owned entirely by the Host allowing a Host instance 
 evmc/docs/Host_Guide.md:22: function in particular VM implementation. The EVMC recommends to name the 
 evmc/docs/Host_Guide.md:24: Invoking the create function will give you the VM instance (::evmc_instance). 
 evmc/docs/Host_Guide.md:26: 
 evmc/docs/Host_Guide.md:27:2. If you are interested in loading VMs dynamically (i.e. to use DLLs) 
 evmc/docs/Host_Guide.md:29: 
 evmc/docs/Host_Guide.md:30:3. The ::evmc_instance contains information about the VM like 
 evmc/docs/Host_Guide.md:33: 
 evmc/docs/Host_Guide.md:39: 
 evmc/docs/Host_Guide.md:42:

Please check that your changes are working as intended.

stackenbotten commented Jun 26, 2019

There was an error when running chk_coding_style for commit df93960d23a2ac9ff09d1bf96a61e72f10b1ae26:

Error: Trailing whitespace found:
 evmc/CHANGELOG.md:30: Disallow implicit conversion from C   `evmc::result` to `evmc_result` 
 evmc/CHANGELOG.md:31: causing unintendent premature releasing of resources. 
 evmc/CHANGELOG.md:44:- Added: 
 evmc/CHANGELOG.md:94: Integration of EVMC as a CMake subproject is easier because 
 evmc/cmake/cable/README.md:57:Start by including this file in your main `CMakeLists.txt` from the Cable 
 evmc/cmake/cable/README.md:58:submodule/subtree or any other location. The `bootstrap.cmake` must be included 
 evmc/docs/Host_Guide.md:10:The implementation can be done in object-oriented manner. 
 evmc/docs/Host_Guide.md:13:Moreover, each of the methods has a pointer to ::evmc_context 
 evmc/docs/Host_Guide.md:14:as a parameter. The context is owned entirely by the Host allowing a Host instance 
 evmc/docs/Host_Guide.md:22: function in particular VM implementation. The EVMC recommends to name the 
 evmc/docs/Host_Guide.md:24: Invoking the create function will give you the VM instance (::evmc_instance). 
 evmc/docs/Host_Guide.md:26: 
 evmc/docs/Host_Guide.md:27:2. If you are interested in loading VMs dynamically (i.e. to use DLLs) 
 evmc/docs/Host_Guide.md:29: 
 evmc/docs/Host_Guide.md:30:3. The ::evmc_instance contains information about the VM like 
 evmc/docs/Host_Guide.md:33: 
 evmc/docs/Host_Guide.md:39: 
 evmc/docs/Host_Guide.md:42:

Please check that your changes are working as intended.

stackenbotten commented Jun 26, 2019

There was an error when running chk_coding_style for commit cf03187111f120c07f256f639edc762a898e4364:

Error: Trailing whitespace found:
 evmc/CHANGELOG.md:30: Disallow implicit conversion from C   `evmc::result` to `evmc_result` 
 evmc/CHANGELOG.md:31: causing unintendent premature releasing of resources. 
 evmc/CHANGELOG.md:44:- Added: 
 evmc/CHANGELOG.md:94: Integration of EVMC as a CMake subproject is easier because 
 evmc/cmake/cable/README.md:57:Start by including this file in your main `CMakeLists.txt` from the Cable 
 evmc/cmake/cable/README.md:58:submodule/subtree or any other location. The `bootstrap.cmake` must be included 
 evmc/docs/Host_Guide.md:10:The implementation can be done in object-oriented manner. 
 evmc/docs/Host_Guide.md:13:Moreover, each of the methods has a pointer to ::evmc_context 
 evmc/docs/Host_Guide.md:14:as a parameter. The context is owned entirely by the Host allowing a Host instance 
 evmc/docs/Host_Guide.md:22: function in particular VM implementation. The EVMC recommends to name the 
 evmc/docs/Host_Guide.md:24: Invoking the create function will give you the VM instance (::evmc_instance). 
 evmc/docs/Host_Guide.md:26: 
 evmc/docs/Host_Guide.md:27:2. If you are interested in loading VMs dynamically (i.e. to use DLLs) 
 evmc/docs/Host_Guide.md:29: 
 evmc/docs/Host_Guide.md:30:3. The ::evmc_instance contains information about the VM like 
 evmc/docs/Host_Guide.md:33: 
 evmc/docs/Host_Guide.md:39: 
 evmc/docs/Host_Guide.md:42:

Please check that your changes are working as intended.

stackenbotten commented Jun 26, 2019

There was an error when running chk_coding_style for commit 512418a9ec7d7cc6b1d977afc239cc57025779bf:

Error: Trailing whitespace found:
 evmc/CHANGELOG.md:30: Disallow implicit conversion from C   `evmc::result` to `evmc_result` 
 evmc/CHANGELOG.md:31: causing unintendent premature releasing of resources. 
 evmc/CHANGELOG.md:44:- Added: 
 evmc/CHANGELOG.md:94: Integration of EVMC as a CMake subproject is easier because 
 evmc/cmake/cable/README.md:57:Start by including this file in your main `CMakeLists.txt` from the Cable 
 evmc/cmake/cable/README.md:58:submodule/subtree or any other location. The `bootstrap.cmake` must be included 
 evmc/docs/Host_Guide.md:10:The implementation can be done in object-oriented manner. 
 evmc/docs/Host_Guide.md:13:Moreover, each of the methods has a pointer to ::evmc_context 
 evmc/docs/Host_Guide.md:14:as a parameter. The context is owned entirely by the Host allowing a Host instance 
 evmc/docs/Host_Guide.md:22: function in particular VM implementation. The EVMC recommends to name the 
 evmc/docs/Host_Guide.md:24: Invoking the create function will give you the VM instance (::evmc_instance). 
 evmc/docs/Host_Guide.md:26: 
 evmc/docs/Host_Guide.md:27:2. If you are interested in loading VMs dynamically (i.e. to use DLLs) 
 evmc/docs/Host_Guide.md:29: 
 evmc/docs/Host_Guide.md:30:3. The ::evmc_instance contains information about the VM like 
 evmc/docs/Host_Guide.md:33: 
 evmc/docs/Host_Guide.md:39: 
 evmc/docs/Host_Guide.md:42:

Please check that your changes are working as intended.

chriseth force-pushed the useevmone branch from 6f3fae7 to 9ab1168 Compare

June 26, 2019 22:18

stackenbotten commented Jun 26, 2019

There was an error when running chk_coding_style for commit 6f3fae70358710d03b40924b3527c53340a5e828:

Error: Trailing whitespace found:
 evmc/CHANGELOG.md:30: Disallow implicit conversion from C   `evmc::result` to `evmc_result` 
 evmc/CHANGELOG.md:31: causing unintendent premature releasing of resources. 
 evmc/CHANGELOG.md:44:- Added: 
 evmc/CHANGELOG.md:94: Integration of EVMC as a CMake subproject is easier because 
 evmc/cmake/cable/README.md:57:Start by including this file in your main `CMakeLists.txt` from the Cable 
 evmc/cmake/cable/README.md:58:submodule/subtree or any other location. The `bootstrap.cmake` must be included 
 evmc/docs/Host_Guide.md:10:The implementation can be done in object-oriented manner. 
 evmc/docs/Host_Guide.md:13:Moreover, each of the methods has a pointer to ::evmc_context 
 evmc/docs/Host_Guide.md:14:as a parameter. The context is owned entirely by the Host allowing a Host instance 
 evmc/docs/Host_Guide.md:22: function in particular VM implementation. The EVMC recommends to name the 
 evmc/docs/Host_Guide.md:24: Invoking the create function will give you the VM instance (::evmc_instance). 
 evmc/docs/Host_Guide.md:26: 
 evmc/docs/Host_Guide.md:27:2. If you are interested in loading VMs dynamically (i.e. to use DLLs) 
 evmc/docs/Host_Guide.md:29: 
 evmc/docs/Host_Guide.md:30:3. The ::evmc_instance contains information about the VM like 
 evmc/docs/Host_Guide.md:33: 
 evmc/docs/Host_Guide.md:39: 
 evmc/docs/Host_Guide.md:42:

Please check that your changes are working as intended.

stackenbotten commented Jun 26, 2019

There was an error when running chk_coding_style for commit 9ab1168906e804229953befef9e8f1eb45071a15:

Error: Trailing whitespace found:
 evmc/CHANGELOG.md:30: Disallow implicit conversion from C   `evmc::result` to `evmc_result` 
 evmc/CHANGELOG.md:31: causing unintendent premature releasing of resources. 
 evmc/CHANGELOG.md:44:- Added: 
 evmc/CHANGELOG.md:94: Integration of EVMC as a CMake subproject is easier because 
 evmc/cmake/cable/README.md:57:Start by including this file in your main `CMakeLists.txt` from the Cable 
 evmc/cmake/cable/README.md:58:submodule/subtree or any other location. The `bootstrap.cmake` must be included 
 evmc/docs/Host_Guide.md:10:The implementation can be done in object-oriented manner. 
 evmc/docs/Host_Guide.md:13:Moreover, each of the methods has a pointer to ::evmc_context 
 evmc/docs/Host_Guide.md:14:as a parameter. The context is owned entirely by the Host allowing a Host instance 
 evmc/docs/Host_Guide.md:22: function in particular VM implementation. The EVMC recommends to name the 
 evmc/docs/Host_Guide.md:24: Invoking the create function will give you the VM instance (::evmc_instance). 
 evmc/docs/Host_Guide.md:26: 
 evmc/docs/Host_Guide.md:27:2. If you are interested in loading VMs dynamically (i.e. to use DLLs) 
 evmc/docs/Host_Guide.md:29: 
 evmc/docs/Host_Guide.md:30:3. The ::evmc_instance contains information about the VM like 
 evmc/docs/Host_Guide.md:33: 
 evmc/docs/Host_Guide.md:39: 
 evmc/docs/Host_Guide.md:42:

Please check that your changes are working as intended.

chfast reviewed

View reviewed changes

test/ExecutionFramework.cpp Outdated Show resolved Hide resolved

test/ExecutionFramework.cpp Outdated Show resolved Hide resolved

test/ExecutionFramework.cpp Outdated Show resolved Hide resolved

test/ExecutionFramework.cpp Outdated Show resolved Hide resolved

test/ExecutionFramework.cpp Outdated Show resolved Hide resolved

test/ExecutionFramework.cpp Outdated Show resolved Hide resolved

test/ExecutionFramework.cpp Outdated

@@ @@ -50,25 55,294 @@ string getIPCSocketPath() @@
  return ipcPath;
 }
+evmc::vm& getVM()
+{
+ static unique_ptr<evmc::vm> theVM;

Member

chfast Jun 27, 2019

You can just do static evmc::vm theVM = ...;

Contributor Author

chriseth Jun 27, 2019

but then I cannot try different locations for libevmone.so

Contributor Author

chriseth commented Jun 27, 2019

The way evmc is included is still not satisfactory for me. Since it is "just a wrapper", I'm thinking of stripping everything that is not needed and just adding the 4 header files and single .c file into the project.

chriseth force-pushed the useevmone branch 3 times, most recently from d8e102b to 05e68c5 Compare

June 27, 2019 20:47

Contributor Author

chriseth commented Jun 27, 2019

Test failure count is down to 36.

One is a weird failure in the auction registrar.

Then we have a bunch of gas cost test failures - those are probably still quite some work. I would suspect this is related to https://github.com/ethereum/solidity/pull/7010/files#diff-e4cd21bbaea28071e286e4adfd6ec79aR97

Finally, we have some failures related to the precompiled contracts. We probably do not want to implement everything, so this boils down to implementing some crude "simulated" version of each precompile and modifying the test expectations, I would assume.

Anyone feel free to continue working on this until Monday :)

chriseth force-pushed the useevmone branch from 05e68c5 to be7892f Compare

June 27, 2019 21:05

Contributor Author

chriseth commented Jun 27, 2019

Another reason for the gas test failures is that EVMHost maybe has to compute the data gas. @chfast - do you have any insight into this?

Member

chfast commented Jun 27, 2019

Another reason for the gas test failures is that EVMHost maybe has to compute the data gas. @chfast - do you have any insight into this?

Host is responsible for counting refunds, unfortunately.

Contributor Author

chriseth commented Jun 28, 2019

@chfast is there a complete implementation somewhere?

chriseth mentioned this pull request

Investigate evmc / evmone for semantic tests #7007

Closed

Member

chfast commented Jul 3, 2019

@chfast is there a complete implementation somewhere?

In Aleth:

SSTORE: https://github.com/ethereum/aleth/blob/master/libevm/ExtVMFace.cpp#L65-L83
There should be also one for SELFDESTRUCT, but I cannot find it around ExtVMFace.

In geth:

Yes, we already know this should be handled by the VM, but that requires the ABI break so it will not be changed in EVMC 6.

chriseth mentioned this pull request

Switch to evmc/evmone for testing #7083

Closed

chriseth force-pushed the useevmone branch 3 times, most recently from 2fc4208 to 94c7643 Compare

July 11, 2019 21:07

Contributor Author

chriseth commented Jul 11, 2019

Phew. Now the only thing left to do is clean everything up and implement the required test vectors for the snark precompiles.

bshastry force-pushed the useevmone branch from 3a0c5e2 to 44f9466 Compare

August 7, 2019 14:56

Contributor

bshastry commented Aug 7, 2019

Selectively disabled failing check for ASan builds.

chriseth force-pushed the useevmone branch from 44f9466 to 4bdeb13 Compare

August 7, 2019 14:59

Contributor Author

chriseth commented Aug 7, 2019

Also had a local change in the dockerfile that was not pushed upstream.

bshastry reviewed

View reviewed changes

.circleci/docker/Dockerfile.ubuntu1904 Outdated

 tar -xf /tmp/aleth.tar.gz -C /usr

 wget -q -O /tmp/evmone.tar.gz "${EVMONE_URL}"; \

 test "$(shasum /tmp/evmone.tar.gz)" = "$EVMONE_HASH /tmp/evmone.tar.gz"; \

 tar -xf /tmp/evmone.tar.gz -C /usr

Contributor

bshastry Aug 7, 2019

Minor comment: Later in the dockerfile, a static build of evmone is installed at the same destination /usr from source. The subsequent overwriting should not be a problem since the tarball is presumably packed with the shared library version which the static build does not install.

bshastry reviewed

View reviewed changes

.circleci/soltest.sh Outdated

+(
+cd /usr/
+wget -q https://github.com/ethereum/evmone/releases/download/v0.1.0/evmone-0.1.0-linux-x86_64.tar.gz -O - | tar xz
+)

Contributor

bshastry Aug 7, 2019

This can be removed since the docker image should contain evmone now.

bshastry reviewed

View reviewed changes

CMakeLists.txt Outdated

		@@ -13,6 13,7 @@ eth_policy()
		set(PROJECT_VERSION "0.5.11")
		project(solidity VERSION ${PROJECT_VERSION} LANGUAGES C CXX)

Contributor

bshastry Aug 7, 2019

Newline can be removed?

chriseth force-pushed the useevmone branch 2 times, most recently from f0e26ef to 44ff050 Compare

August 7, 2019 15:50

ekpyron reviewed

View reviewed changes

Member

ekpyron left a comment

I haven't checked everything in every detail, but generally this looks good - mainly some minor comments!
The precompile stuff I haven't checked at all, though.

docs/contributing.rst

-To run a basic set of tests that require neither ``aleth`` nor ``libz3``, run
-``./scripts/soltest.sh --no-ipc --no-smt``.
+Some tests require the ``libevmone.so`` library, others require ``libz3``.

Member

ekpyron Aug 8, 2019

Not really related to this PR, but "others require libz3" doesn't really feel correct here... first of all it's not enough to have libz3 at runtime as for libevmone, but it must have been present at build time already, secondly it must be a correct version of z3 and thirdly it could also be cvc4... But we can change this separately, no need to let this block this PR.

docs/contributing.rst

+`Github <https://github.com/ethereum/evmone/releases/download/v0.1.0/evmone-0.1.0-linux-x86_64.tar.gz>`_
+and either place it in the project root path or inside the ``deps`` folder.
+If you do not have libz3 installed on your system, you should disable the SMT tests:

Member

ekpyron Aug 8, 2019

More like "If you did not build against libz3 or cvc4" actually, but yeah, as above.

libdevcore/CommonData.h Show resolved Hide resolved

test/EVMHost.cpp

- result.create_address = message.destination;
- destination.code = bytes(result.output_data, result.output_data result.output_size);
- destination.codeHash = convertToEVMC(keccak256(destination.code));
+ result.gas_left -= eth::GasCosts::createDataGas * result.output_size;

Member

ekpyron Aug 8, 2019

Hm... this logic now probably results in parts of our gas tests basically being tested against our own gas calculation, right? But ok, it's probably fine...

Contributor Author

chriseth Aug 8, 2019

Yep, no way around this, unfortunately.

Contributor Author

chriseth Aug 8, 2019

On the other hand: Most of the gas calculation is done by evmone itself, we mainly only do storage gas and data cas.

Member

chfast Aug 8, 2019

I'm not sure I mentioned it here: this is going to be changed and EVM will calculate these, but we need to break EVMC ABI first.

test/EVMHost.h Outdated Show resolved Hide resolved

test/contracts/AuctionRegistrar.cpp Show resolved Hide resolved

test/libsolidity/SolidityOptimizer.cpp Outdated

+ // we disable it if we are compiling an ASan test binary. The
+ // DISABLE_CHECK_FOR_ASAN_BUILD macro is defined if
+ // -DSANITIZE=address is passed as a compile time cmake option.
+#ifndef DISABLE_CHECK_FOR_ASAN_BUILD

Member

ekpyron Aug 8, 2019

I might have gone with just calling this ASAN_BUILD... and we still want to run the test, even though it takes a long time? We could also move the #ifndef around the whole test to avoid having to wait for it for 60 seconds...

Member

chfast Aug 8, 2019

You don't need to define own macro. There is a way to check if you are building with ASan: https://clang.llvm.org/docs/AddressSanitizer.html#conditional-compilation-with-has-feature-address-sanitizer.

Contributor Author

chriseth Aug 8, 2019

@bshastry noted that this is not cross-compiler. Does asan exist on non-llvm compilers?

Member

chfast Aug 8, 2019

This will work for cross-compilation - the code will be instrumented for the target architecture (if that's a concern).

ASan is also supported with GCC. The same basic flags are used, but the runtime library is different I believe. In case of GCC the __SANITIZE_ADDRESS__ macro is defined: https://gcc.gnu.org/onlinedocs/gcc-4.8.0/cpp/Common-Predefined-Macros.html.

Contributor Author

chriseth Aug 8, 2019

I changed it to use the compiler-internal check for now. It's fine for the test to fail on gcc.

chriseth force-pushed the useevmone branch from 44ff050 to 6d2383e Compare

August 8, 2019 14:29

chriseth and others added 13 commits

August 8, 2019 17:34


Add evmc.

7401fba


Use evmone for testing.

3dec3af


Count gas for transaction and code deposit.

b7d17c5


Precompiles.

22e9155


Removing obsolete but timing-sensitive test.

13e2a6f


Provide path to libevmone.

f692cec


Discover path.

09eeb1a


Construct VM with path.

ac43c3b


Remove mention of IPC.

c4638cc


Remove traces of aleth and IPC.


Fix for ossfuzz.

f3f3cac


Better error message if loading fails.

1258aab


Selectively disable time sensitive check in the constant_optimization…

a20dc62

…_early_exit unit test for address sanitizer builds.

chriseth force-pushed the useevmone branch from 6d2383e to a20dc62 Compare

August 8, 2019 15:39

Contributor Author

chriseth commented Aug 8, 2019

The tests are not run properly, because the docker image currently only contains libevmone.a and not libevmone.so.

Contributor Author

chriseth commented Aug 8, 2019

The above is fixed by #7203

chriseth merged commit 18ffe3b into develop

ekpyron deleted the useevmone branch

August 8, 2019 16:44

ekpyron mentioned this pull request

[Do not merge] Fix nested array copy from calldata to storage #6983

Closed

6 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment