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

Rewrite rest of the RTS in Rust #2210

Merged
merged 95 commits into from
Jan 19, 2021
Merged

Rewrite rest of the RTS in Rust #2210

merged 95 commits into from
Jan 19, 2021

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Dec 23, 2020

Previously 1461f56 ported the GC to Rust. This patch ports rest of the RTS to
Rust and removes all C code.

Note that this is not 100% direct port. I rewrote some parts, and implemented
new tests. Notable changes:

  • Float functions that were just wrappers of musl/libc float functions are
    removed. Compiler now directly calls musl/libc float functions. Only float
    function left in the RTS is float_fmt.

  • We now use Rustc core's char functions instead of musl's. This allows us to
    handle cases where upper/lowercase of a character is multiple characters. For
    backwards compatibility we return the input chracter in these cases for now.

  • Closure table is slightly refactored:

    • FREE_SLOT is no longer shifted, it holds the next free slot directly
    • The magic value FULL is gone, we check if the table is full by comparing
      FREE_SLOT with table's length field.
  • Added a dummy return value to wctomb stub. Previously it wasn't returning
    anything even though it should return an int. (we should probably panic in
    this function)

  • (s)leb128 encoding/decoding code is slightly refactored to use binary
    patterns instead of hexadecimal numbers for checking specific bits. For
    example we now use 0b1000_0000 to check the highest order bit, instead of
    0x80 etc.

  • utf8_valid.c is completely gone. We now use Rust core's str::from_utf8 to
    validate UTF-8 strings.

  • Lots of debug functions added. Notably, calling debug::dump_heap prints the
    entire heap using ic0.debug_print. Debug functions are only compiled in
    debug mode. (when Rust code is built without --release)

  • Rust arithmetic operations have overflow and underflow checking in debug
    mode, so all arithmetic in the RTS is checked for those in debug mode now. We
    now use add_wrapping etc. methods in specific places to explicitly allow
    overflows and underflows.

  • All methods that previously had a *const receiver now have a *mut
    receiver. This is because of the rustc bug 80258 which doesn't allow
    passing *mut pointer to a method that expects *const, and having *const
    methods requires a lot of casts in use sites.

    (Note that *mut vs. *const pointers in Rust are not like &mut and &,
    unlike references, in pointers one can be coerced into the other. My
    understanding is *mut and *const exist for similar reasons they exist in
    C, to hint about mutability of the pointed memory.)

New test suite for the RTS

We now have a test suite implemented in Rust where we can use the entire
standard library and other Rust libraries. All existing tests are ported
directly. AFL tests are ported as quickcheck tests. New quickcheck tests added
for text functions (iterators, concatenation, text-to-blob etc.).

The test suite runs on WASI. This makes it more realistic (it uses the same
libtommath.a as the real RTS) and easier to run on darwin etc.

A previous version of this PR had the test suite running as a 32bit binary, i.e.
using cross-compilation. This worked, but caused issues with CI, hence
Joachim changed that. Old comments follow:

The test suite currently runs on native, but it needs to be compiled to a
32-bit platform as the RTS only supports 32-bit platforms, and it would be a
lot of work to make it work on 64-bit platforms. This is checked in run time
and the test suite fails (returns 1) with this meesage when run on 64-bit:

Motoko RTS only works on 32-bit architectures

An easy way to run the test suite on x86/64 platforms is with:

TOMMATHSRC=... cargo run --target=i686-unknown-linux-gnu

TOMMATHSRC should be the value of nix expression nixpkgs.sources.libtommath.
I didn't update nix files yet to run the tests on CI yet.

Differences in the RTS code when compiled to be linked with moc-generated code
and compiled for testing:

  • The test suite runs on native, so we compile the RTS code to i686 when
    testing, instead of wasm32.

    Because both platforms are 32-bit this shouldn't cause too much changes, but
    there can still be subtle changes in how LLVM decides to lay out struct
    fields. For example, this C struct and its Rust equivalent:

    struct Test {
        uint32_t a;
        uint64_t b;
    };
    

    by default is 4 words on wasm32, but 3 words on i686. A change in this patch
    is we now make sure that all our sturcts are packed (with no padding between
    fields) by using #[repr(packed)] attributes.

  • When compiled to native the RTS uses a different allocator, where
    alloc_words is just malloc, and we never free allocated objects.

    The GC module is not compiled when generating native.

    This could potentially cause OOM issues if we run longer/more tests, as
    32-bit platforms have 4G memory, but so far things work fine.

    In the future If memory limit becomes a problem we can allocate a (say, 1G)
    region in each test and free it afterwards.

    Finally, we could make the GC compile to native. I think this is not too
    difficult to do, but I decided to not invest time into this so far as this
    patch already took too long.

Notes on using quickcheck

I found the Rust quickcheck library a bit lacking in some aspects. For example,
as far as I can see, there's no way to specify number of iterations for
individual tests. All we can do is setting the global number with an env
variable:

QUICKCHECK_TESTS=100 cargo run --target=i686-unknown-linux-gnu

Secondly, it seems not possible to specify the seed, for individual tests, or
globally. This means even without any relevant changes a test may fail on CI
randomly (assuming there are bugs). When a test fails quickcheck tries to
shrink the argument and then print the arguments, so we can copy that from the
CI logs and make it a unit test, which is good. But random behavior on CI may
not be ideal.

I haven't compared proptest API to quickcheck to see if it solves any of these
problems.

Cycle and binary size changes

CI reports

In terms of gas, 3 tests improved and the mean change is -3.7%.
In terms of size, 3 tests regressed and the mean change is +56.0%.

The size changes are for the most part the static LOWERCASE_TABLE from https://doc.rust-lang.org/src/core/unicode/unicode_data.rs.html#568

Future cleanup

The following things could be done as future cleanup

  • Don't commit bindgen-generated tommath bindings, run bindgen in build time.
  • Build and link tommath via Rust build script (build.rs) instead of manually in a Makefile, as we did in the tests when they were built manually.
  • Or: Build and link libtommath.a using nix, and treat it more like a “normal system library”, so that we get nix caching.

Native uses of alloc_array are duplicated in the native test files.
Those will be removed when the tests are ported to Rust.
This also changes how we store the next free index a little bit. The
constant `FULL` is gone, when the next free location is equal to the
array length that's how we know the table is full now. Also, `FREE_SLOT`
no longer next free location shifted left, it holds the next free
location directly (not shifted).
@osa1
Copy link
Contributor Author

osa1 commented Dec 23, 2020

The last commit solves the issue with generating rlib for the RTS. I was thinking more hacky solution before but I think it's not too bad. We just introduce another Cargo.toml and only build it to rlib.

@osa1
Copy link
Contributor Author

osa1 commented Dec 23, 2020

Could you solve this with feature flags?

I don't think that's possible. We need to define panic_handler (a top-level definition) only in some cases. The only way I know to do this is with feature flags.

Alternatively we could handle it at Makefile or in a build script to add/remove the definition but that's worse I think.

@osa1
Copy link
Contributor Author

osa1 commented Dec 23, 2020

A lot of redundant coercions in the RTS will be gone once rust-lang/rust#80258 is fixed.

- Introduced `as_blah` methods to convert a SkewedPtr or *Obj to other
  object types. These methods check the type in debug mode.

- Improve error reporting, we know show details in assertion failures

- Temporarily link with debug runtime to enable sanity checking by
  default for now
@nomeata
Copy link
Collaborator

nomeata commented Jan 18, 2021

What is the status here? Of the current TODOs, namely

  • Don't commit bindgen-generated tommath bindings, run bindgen in build time.
  • Run RTS tests on CI
  • Build and link tommath via Rust build script (build.rs) instead of manually in a Makefile, as we do in the tests.

I think the first one does not need to block merging this one, the last one is also not so pressing. @osa1, do you need help with the second one? Should I do that? If so, on this branch directly or another?

@osa1
Copy link
Contributor Author

osa1 commented Jan 19, 2021

@nomeata if you could help with (2) that would be great indeed. I'm currently busy with NNS stuff.

Two things about (2):

  • We need to export TOMMATHSRC the same way we do when building the RTS (it should point to the same version we use in the RTS).
  • We need to pass a 32-bit --target parameter when running the tests (otherwise the test program will just fail saying that it only works on 32-bit). Locally I've been using cargo run --target=i686-unknown-linux-gnu. I guess we can do the same on CI on Linux. For Darwin I'm not sure what 32-bit targets exist.

@nomeata
Copy link
Collaborator

nomeata commented Jan 19, 2021

Note to myself: Had to install gcc-multilib to build and run the test suite on my Debian machine (outside of nix)

@nomeata
Copy link
Collaborator

nomeata commented Jan 19, 2021

Ok, got it to work on Linux. I may have to skip running the tests on drawin…

@nomeata
Copy link
Collaborator

nomeata commented Jan 19, 2021

Now blocked on CI infrastructure building i686-linux programs. Infra alerted in https://dfinity.slack.com/archives/CL7Q2RXUM/p1611053732053200, they’ll work on it.

this is easier to build on all platforms, compared to building for 32bits.

It also means we are testing things closer to what we really run.
@nomeata
Copy link
Collaborator

nomeata commented Jan 19, 2021

By running the rts tests as a WASI program, we avoid problems with cross-building for 32bits, plus our tests are “more realistic”.

My changes lost the ability to easily run the test suite natively, e.g. for debugging. But it should be able to recover that: Either a simple branch in build.rs to use the old code, or extend Makefile to build a native libtommath.a. Maybe there is also a better, more flexible way for the rust code to pick up the libtommath.a library?

Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Let’s get that in and iterate on master.

@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Jan 19, 2021
@mergify mergify bot merged commit 8ba767c into master Jan 19, 2021
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 19, 2021
@nomeata nomeata deleted the osa1/rts_rust_port branch January 19, 2021 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants