Fix ewasm polyfills & enable corresponding tests. #9508

aarlt · 2020-07-26T20:10:36Z

Replaced by #10319, #10323, #10368, #10369, #10406.

test/cmdlineTests/evm_to_wasm_break/output

libyul/backends/wasm/EVMToEwasmTranslator.cpp

axic · 2020-10-29T10:59:35Z

Rebased in order to get the changes from #10135.

axic · 2020-10-29T11:01:45Z

We may want to clarify what memory model we are following with the EVM->Wasm translator. I assume we are trying to keep the memory matching with EVM aka. numbers stored are in big-endian order.

This means we need to byteswap balances, but can just copy addresses and storage, as those are not treated as numbers in ewasm host functions.

leonardoalt · 2020-10-29T16:09:00Z

@axic @aarlt do we need to discuss that in the round or are you confident that's the way to proceed?

chriseth · 2020-10-29T17:36:05Z

inline assembly should "just work". mload(x) and mload(add(x, 1)) should return the same thing on ewasm and on evm.

axic · 2020-10-30T18:10:35Z

I assume we are trying to keep the memory matching with EVM aka. numbers stored are in big-endian order.

inline assembly should "just work". mload(x) and mload(add(x, 1)) should return the same thing on ewasm and on evm.

Agreed.

I think in this case this PR may need quite a bit of work to only swap the correct things. It may be swapping sload/store right now which masks the problem.

aarlt · 2020-11-02T23:08:12Z

I think in this case this PR may need quite a bit of work to only swap the correct things. It may be swapping sload/store right now which masks the problem.

I added a test test/libsolidity/semanticTests/various/endianess.sol to check the behaviour of mload/mstore and sload/sstore. For mload/mstore ewasm is behaving exactly as in the EVM case. For sload/sstore I checked the "endianess" by adding debug output in evmone and hera. It turns out that the implementation is correct.

axic · 2020-11-02T23:18:17Z

libyul/backends/wasm/EVMToEwasmTranslator.cpp

+	z3 := bswap64(i64.load(i32.add(pos, 16:i32)))
+	z4 := bswap64(i64.load(i32.add(pos, 24:i32)))
+}
+function mload_address(pos:i32) -> z2, z3, z4 {


We should start adding comments. Is this expecting to read 32 or 20 bytes from pos?

This would read 24 bytes from pos. Yeah, we really need to add more comments in that file.

axic · 2020-11-02T23:19:46Z

libyul/backends/wasm/EVMToEwasmTranslator.cpp

+	z4 := bswap64(i64.load(i32.add(pos, 24:i32)))
+}
+function mload_address(pos:i32) -> z2, z3, z4 {
+	z2 := i64.and(bswap64(i64.load(pos)), 0x00000000ffffffff)


Why not just read 32-bits here? (And adjust the offset in the caller too)

Suggested change

z2 := i64.and(bswap64(i64.load(pos)), 0x00000000ffffffff)

z2 := bswap32(i32.load(pos))

This would only work, if we do something like bswap32(i64.extend_i32_u(i32.load(i32.add(pos, 4:i32)))). I think i64.and(bswap64(i64.load(pos)), 0x00000000ffffffff) is better readable.

I don't really get it. As long as you only put in 20 bytes of the address in the memory and pos refers to that offset there is no extension needed. Why would bswap32 need i64 inputs?

bswap32 is just defined like this - function bswap32(x) -> y. Maybe this can just be changed, I didn't tried that.

I just changed that. I think it is better now.

axic · 2020-11-02T23:31:02Z

I think what needs to be done is documenting how this is supposed to operate above the polyfill:

memory layout
size and endianness of types (address, balance, storage)
etc.

aarlt · 2020-11-17T02:21:13Z

libyul/backends/wasm/polyfill/eei.h

+	// address need to be encoded as a little endian unsigned integer.
+	// balance is of type u128 represented as a 16 bytes long
+	// little endian unsigned integer in memory.
+	eth.getExternalBalance(12:i32, 32:i32)


Following the hera implementation, eth.getExternalBalance(..) get the balance as big endian (evmc_uint256be), where this value is written to the defined offset via storeUint128, that uses storeMemoryReverse - resulting in little endianness.

aarlt · 2020-11-17T02:37:50Z

libyul/backends/wasm/polyfill/eei.h

+	// little endian unsigned integer in memory.
+	eth.getExternalBalance(12:i32, 32:i32)
+
+	z3 := i64.load(40:i32)


I don't really understand why this actually works.

If z1, z2, z3, z4 is big endian, where the data is currently encoded as little endian, should'nt this be z3 := bswap64(i64.load(40)) and z4 := bswap64(i64.load(32)) - or, if that data is already stored as big endian, shouldn't this be z3 := i64.load(32) and z4 := i64.load(40).

Both variants are not working.

I don't understand why the i64's need to get loaded "reversely" z3 := i64.load(40) and z4:= i64.load(32) without the need of using bswap64.

chriseth · 2020-11-17T15:49:28Z

libyul/backends/wasm/polyfill/eei.h

+	// eth.getAddress: Gets address of currently executing account and stores it
+	// in memory at the given offset. This address is encoded as a 160 bit number,
+	// represented as a 20 bytes long little endian unsigned integer in memory.


stores little-endian encoded 160 bit number

chriseth · 2020-11-17T15:51:48Z

libyul/backends/wasm/polyfill/eei.h

+
+	// mload_address takes the little endian unsigned integer from memory and converts
+	// it to an big endian unsigned integer (evm is big endian).
+	z2, z3, z4 := mload_address(0:i32)


This adds 4 again - could either use 4 here or 0 above.

chriseth · 2020-11-17T15:54:00Z

libyul/backends/wasm/polyfill/bitwise.h

+	lo := i64.and(x, 0xffffffff)
+}
+
+// xor(x, y): bitwise “xor” of x and y


Please do not use non-ascii characters

axic · 2020-11-25T19:10:30Z

Is this PR still needed?

axic · 2020-11-30T15:25:07Z

@aarlt can we close this?

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch from a3cd729 to 20ade27 Compare July 26, 2020 20:20

aarlt marked this pull request as draft July 26, 2020 21:07

axic reviewed Jul 27, 2020

View reviewed changes

test/cmdlineTests/evm_to_wasm_break/output Show resolved Hide resolved

aarlt force-pushed the evmc-vms-evm-ewasm-ci branch 7 times, most recently from edd351a to d3eb416 Compare July 27, 2020 23:41

chriseth reviewed Jul 29, 2020

View reviewed changes

libyul/backends/wasm/EVMToEwasmTranslator.cpp Outdated Show resolved Hide resolved

chriseth reviewed Jul 29, 2020

View reviewed changes

libyul/backends/wasm/EVMToEwasmTranslator.cpp Outdated Show resolved Hide resolved

chriseth reviewed Jul 29, 2020

View reviewed changes

libyul/backends/wasm/EVMToEwasmTranslator.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch 3 times, most recently from c0b6db3 to 90e3deb Compare July 31, 2020 14:04

aarlt force-pushed the evmc-vms-evm-ewasm-ci branch from d3eb416 to f496857 Compare July 31, 2020 14:37

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch 2 times, most recently from 5cd0a49 to 36193b2 Compare July 31, 2020 15:33

axic reviewed Jul 31, 2020

View reviewed changes

libyul/backends/wasm/EVMToEwasmTranslator.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch from 215b340 to 614734e Compare July 31, 2020 23:18

aarlt force-pushed the evmc-vms-evm-ewasm-ci branch 4 times, most recently from 05269ce to cdddda1 Compare August 3, 2020 15:07

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch from 614734e to 71958e6 Compare August 3, 2020 15:08

aarlt force-pushed the evmc-vms-evm-ewasm-ci branch from cdddda1 to 88af39d Compare August 3, 2020 15:41

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch from 71958e6 to 0c462ac Compare August 3, 2020 15:41

aarlt mentioned this pull request Aug 3, 2020

Add support for ewasm evmc vm. #9143

Merged

3 tasks

aarlt force-pushed the evmc-vms-evm-ewasm-ci branch from 88af39d to f66a09a Compare August 3, 2020 16:24

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch 2 times, most recently from 521e631 to d7e23f1 Compare November 2, 2020 22:42

axic reviewed Nov 2, 2020

View reviewed changes

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch 4 times, most recently from 8fe45ba to 0b45a88 Compare November 9, 2020 20:27

aarlt added 4 commits November 16, 2020 21:16

Fix ewasm polyfills & enable tests.

f62cbff

Fix windows build.

4a5ab2a

Change bswap32 & bswap16 input&output type to i32.

2564c68

Refactor polyfill: split into different files & add comments.

162ee37

aarlt force-pushed the evmc-vms-evm-ewasm-tests branch from 0b45a88 to 162ee37 Compare November 17, 2020 02:16

aarlt commented Nov 17, 2020

View reviewed changes

chriseth reviewed Nov 17, 2020

View reviewed changes

aarlt marked this pull request as draft November 17, 2020 17:58

aarlt closed this Nov 30, 2020

axic deleted the evmc-vms-evm-ewasm-tests branch November 30, 2020 23:17

	z2 := i64.and(bswap64(i64.load(pos)), 0x00000000ffffffff)
	z2 := bswap32(i32.load(pos))

Fix ewasm polyfills & enable corresponding tests. #9508

Fix ewasm polyfills & enable corresponding tests. #9508

Uh oh!

Conversation

aarlt commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

axic commented Oct 29, 2020

Uh oh!

axic commented Oct 29, 2020

Uh oh!

leonardoalt commented Oct 29, 2020

Uh oh!

chriseth commented Oct 29, 2020

Uh oh!

axic commented Oct 30, 2020

Uh oh!

aarlt commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Nov 25, 2020

Uh oh!

axic commented Nov 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aarlt commented Jul 26, 2020 •

edited

Loading

axic Nov 2, 2020 •

edited

Loading

axic Nov 3, 2020 •

edited

Loading

aarlt Nov 17, 2020 •

edited

Loading