anza-xyz / move

Move compiler targeting llvm supported backends
https://discord.gg/wFgfjG9J
Apache License 2.0
108 stars 34 forks source link

Allow rbpf-tests to use move stdlib #289

Closed brson closed 1 year ago

brson commented 1 year ago

This adds functions to test_common.rs to locate the source code of, and build the bytecode for, the move standard library; and adjusts rbpf-tests.rs to build and link it into test cases.

For now, tests need the test directive // use-stdlib to link to the standard library. In the future all tests could be linked to stdlib. I have not modified move-ir-tests to link to stdlib, but it would be easy to do if desired.

There are two reasons that stdlib is opt-in:

1) The test case struct-fxpt32.move errors with duplicate symbols during linking. This is probably because our symbol naming is too simple, and this test has some names that are the same as in std.

2) The move stdlib is recompiled, both bytecode and object files, for each test case. This is for simplicity as there are multiple concurrency concerns when compiling the move stdlib to a shared location. I felt the code was getting too complex when attempting to share stdlib across test cases, and it doesn't take that long to compile anyway.

The test harness assigns the stdlib to addres 0x1 by convention. There is no mechanism here for using other dependencies.

Closes https://github.com/solana-labs/move/issues/196

nvjle commented 1 year ago

This adds functions to test_common.rs to locate the source code of, and build the bytecode for, the move standard library; and adjusts rbpf-tests.rs to build and link it into test cases.

For now, tests need the test directive // use-stdlib to link to the standard library. In the future all tests could be linked to stdlib. I have not modified move-ir-tests to link to stdlib, but it would be easy to do if desired.

There are two reasons that stdlib is opt-in:

1. The test case `struct-fxpt32.move` errors with duplicate symbols during linking. This is probably because our symbol naming is too simple, and this test has some names that are the same as in std.

Opt-in makes sense for testing since some of the tests themselves are sourced from move-stdlib.

For struct-fxpt32.move in particular-- this test was literally copied from move-stdlib-- so it certainly has completely overlapping names as move-stdlib/sources/fixed_point32.move (other than the module address). The point of the test was to feed more code to the then very immature compiler and begin building stdlib one file at a time. It is still useful to keep these for regression testing. Not to mention, thus far, stdlib tests are the only "real" code we feed through the compiler.

But yes, for function names, we could (probably should) prepend the module address to the symbol names, with a slight decrease in IR readability.

2. The move stdlib is recompiled, both bytecode and object files, for each test case. This is for simplicity as there are multiple concurrency concerns when compiling the move stdlib to a shared location. I felt the code was getting too complex when attempting to share stdlib across test cases, and it doesn't take that long to compile anyway.

This also has the benefit (like struct-fxpt32 above) of ensuring we get additional regression testing for every single build of the compiler.

The test harness assigns the stdlib to addres 0x1 by convention. There is no mechanism here for using other dependencies.

Closes #196

Thanks for doing this!