Update Rust #2761

osa1 · 2021-09-01T13:52:14Z

This PR bumps rustc version and removes xargo. std dependencies are now built
with cargo's new -Zbuild-std parameter.

To work around rust-lang/wg-cargo-std-aware#23, we implement a tool
"vendor-rust-std-deps" that reads Cargo.lock of a Rust toolchain std and
manually vendors all the dependencies.

These dependencies are then merged with the Rust RTS dependencies before the
building the RTS in nix.

RTS README updated with instructions to bump rustc.

New rustc will enable more const functions, new API functions, stabilizations,
and features for other RTS PRs.

This is to remove some of the potential undefined behaviors (UB). It will also remove some of the syntactic noise in #2761. Rust has alignment restrictions for types and fields beyond the hardware limitations. This means even on Wasm (which has no alignment restrictions) we have some restrictions to deal with. Recently the compiler started to check for some of the obvious sources of UB (rust-lang/rust#82523). One of these is when taking a pointer or reference to a `packed` struct. Since `packed` means no padding between fields, the fields may be unaligned, in which case getting a reference to them would be UB. To fix this, this PR removes `packed` attributes and refactors the `Bits64` type to make sure that the layout is as before. It turns out for types other than `Bits64` we don't need `packed`: all fields are word sizes so the compiler does not add any padding. For `Bits64`, we had a `u64` field which is aligned on 64-bit boundary. To avoid this we now split 64-bit payload into two 32-bit fields. A new module `static_checks` added to make sure struct sizes are as expected. Assertions in this module are checked in compile time. No extra work needed to align objects. All objects need 4 bytes alignment (checked with `core::mem::align_of`). The compiler already [aligns static objects][2], and in runtime we only allocate whole words. So both static and dynamic objects are always aligned. Debug mode assertions added in GC to check object alignment. [1]: https://doc.rust-lang.org/stable/reference/ [2]: https://github.com/dfinity/motoko/blob/59ddaa4520793b6e7038b60e3c30ff267d8415f6/src/codegen/compile.ml#L453

This reverts commit 655d5e3.

nomeata

Looks simpler. Great!

osa1 · 2021-09-13T05:38:35Z

We need to add compiler_builtins as a dep so that it will be vendored, but no matter what version of compiler_builtins I add, cargo build tries to build both 0.1.50 and 0.1.49 and fails because apparently you can't link with two different versions of compiler_builtins (unlike other libraries). Without a compiler_builtins dependency it builds 0.1.49 so I think adding compiler_builtins = "0.1.49" should work, but it doesn't. cargo build tries to build 0.1.50 when I do that.

osa1 · 2021-09-13T05:48:32Z

If I use compiler_builtins = "=0.1.49" then it uses 0.1.49 as expected, and I don't see multiple versions of compiler_builtins being built in cargo build output, but it still fails with

error[E0465]: multiple rlib candidates for `compiler_builtins` found
  |
  = note: candidate #1: /home/omer/dfinity/motoko/rts/motoko-rts/target/wasm32-unknown-emscripten/release/deps/libcompiler_builtins-da9a0c66847c05a7.rlib
  = note: candidate #2: /home/omer/dfinity/motoko/rts/motoko-rts/target/wasm32-unknown-emscripten/release/deps/libcompiler_builtins-122ca795d65a1505.rlib

osa1 · 2021-09-13T07:12:20Z

We need to add compiler_builtins as a dep so that it will be vendored

I wonder if this is a cargo vendor bug. I shouldn't need to add deps of deps to my crates for it to work.

osa1 · 2021-09-16T06:39:28Z

I'm convinced that this is a cargo vendor bug and filed an issue to cargo. compiler_builtins is being downloaded from crates.io so it should be vendored.

ggreif · 2021-09-16T08:48:16Z

filed an issue to cargo

Can you add a link here?

osa1 · 2021-09-16T10:05:22Z

Can you add a link here?

rust-lang/cargo#9915

We should merge this soon and start checking object field offsets in the static assertions I've added recently. Here's an interesting bug caused by the compiler reordering fields. In this definition of object headers:

type Tag = u32;

struct Obj {
    tag: Tag,
}

struct Blob {
    header: Obj,
    len: Bytes<u32>,
}

This function:

unsafe extern "C" fn text_size(s: Value) -> Bytes<u32> {
    (s.get_ptr() as *mut Blob).len()
}

is compiled to

  (func $text_size (type 6) (param i32) (result i32)
    local.get 0
    i32.const 5
    i32.add
    i32.load)

The important part is 5: 1 to unskew the pointer, and 4 to skip the header and get the length field. This definition is fine.

Now, if I refactor object header (Obj) to

type Tag = u8;
type GcMetadata = u8;

pub struct Obj {
    pub tag: Tag,
    pub gc_metadata: GcMetadata,
    // Add padding to make it 1 word
    padding: u16,
}

Size of this struct is the same, but that 5 in the Wasm above becomes 1. I thought this is a miscompilation (bug in the compiler), but it turns out the compiler is free to reorder fields, and in this case it places the header in Blob after len field.

Adding repr(C) fixes the issue without introducing potential undefined behaviors (caused by taking addresses of fields). If we had the assertions for object field offsets I wouldn't have to debug this.

godbolt: https://godbolt.org/z/Tj33Tf6c1
simplified godbolt: https://godbolt.org/z/n1hGKET4z

See the node added with this commit and #2761 (comment) for the problem this solves.

osa1 · 2021-09-16T14:38:39Z

A cargo dev confirmed that the problem with cargo vendor mentioned above is a bug, but it doesn't seem like it's going to be fixed soon. We will probably need to work around it somehow.

See the node added with this commit and #2761 (comment) for the problem this solves.

osa1 · 2021-09-23T08:43:56Z

I wonder if we could manually download the packages that cargo vendor fails to vendor, in preBuild step.

URL for downloading package tarballs: https://crates.io/api/v1/crates/<package name>/<version>/download

Vendoring dependencies is causing a lot of problems in #2761 and other PRs, because `cargo vendor` currently has a bug and cannot vendor standard dependencies (deps of alloc and std, see rust-lang/wg-cargo-std-aware#23). For a long time I thought vendoring is a necessity and tried to work around this issue by vendoring dependencies manually, or using https://github.com/nix-community/naersk. However, it turns out vendoring is not necessary, and we can download dependencies in build step just fine. I also don't see any advantages of vendoring the dependencies. So in this PR we remove vendoring. Unblocks #2761.

osa1 · 2021-10-30T12:54:53Z

Python scripts and nix lines for the cargo vendor but are copied from baloo/nixos-firmware@e108c50. I will ask the author for permission on using the code. If they don't give permission I will implement the workaround myself.

I implemented the workaround myself, removed copied code.

osa1 · 2021-10-30T13:59:15Z

This is ready now @crusso @ggreif @nomeata

ggreif

LGTM!

Thanks for doing this! First time in months that I can build the RTS on the Mac with ToT.

nomeata

Great work, thanks for your perseverence!

Didn't look closely at the new rust code, but have some comments about the nix code.

In nix/drun.nix we have instructions on how to use nix-update to automate updating the sha256s. Not in this PR, but maybe I’ll figure out how to do that with all the hashes we have everywhere, and maybe even write a CI check that updates them.

The new code in ./rts/cargo-vendor-tools was written by you, not copied from somewhere else? Would it be worth turning that into a separate independent projects, or better not?

default.nix

osa1 · 2021-10-31T10:29:54Z

The new code in ./rts/cargo-vendor-tools was written by you, not copied from somewhere else? Would it be worth turning that into a separate independent projects, or better not?

Yeah it's my code. I started implementing it as a reusable package (https://github.com/osa1/cargo-vendor-tools) but then decided to include it in RTS code to give control to Motoko devs. We could still make it a package, but I think it would need to live in dfinity organization and be maintained by the Motoko team.

osa1 · 2021-10-31T10:56:22Z

It seems like there's a intermittent failure in some of the tests. Ubuntu job failed with this after the last commit:

  tailpositions-actor:skipped (no drun)
   [actor1.drun.comp] [actor2.drun.comp] [drun]
  upgrade-hooks:skipped (no drun)
   [upgrade-hook.drun.comp] [upgrade-hook0.drun.comp] [upgrade-hook1.drun.comp] [upgrade-hook2.drun.comp] [drun]
  upgrades:skipped (no drun)
   [upgrade.drun.comp] [upgrade0.drun.comp] [upgrade1.drun.comp] [upgrade2.drun.comp] [drun]
  Some tests failed:
  stable-mem-nat32.mo stable-mem-nat32.mo
  make: *** [Makefile:4: all] Error 1
  make: Leaving directory '/build/test-run-drun-src/run-drun'

The error message is not helpful because it doesn't show the relevant parts. I tried running it a few times locally but it worked fine. I now restarted the job, but it would be good to reproduce the issue and see what's going wrong.

with #2761 adding even more fixed output derivations, this may be useful. If this works, maybe I’ll extract the `run` script here to a script in the repo that works offline just as well.

nomeata · 2021-10-31T12:06:08Z

The error message is not helpful because it doesn't show the relevant parts. I tried running it a few times locally but it worked fine. I now restarted the job, but it would be good to reproduce the issue and see what's going wrong.

Still waiting for Mic92/nix-build-uncached#40 to fix the log display issue :-(

If push comes to shove, we may have to revert #2747. At least for running the tests.

…2871) Usually, when we upgrade some dependencies, we have to manually update the hashes. This involves * changing the hash to something else (e.g. changing one character) * running the build locally * observing the error message with the actual hash * pasting that * commit and push. The `nix-update` tool can automate that. This PR makes Github do that on all pushes (even to feature branches), and if needed, push a fix to that branch. With #2761 adding even more fixed output derivations, this may be more relevant.

osa1 added 2 commits September 1, 2021 16:46

Update rustc, fix some of the warnings

c69d092

Stop using xargo

41c7a93

osa1 mentioned this pull request Sep 1, 2021

Build rustc from source, fix a few issues with the RTS build system #2519

Closed

osa1 added 2 commits September 1, 2021 17:46

Fix formatting

9c0bb84

More syntactic noise to work around unaligned ref warnings

655d5e3

osa1 mentioned this pull request Sep 6, 2021

RTS: remove repr(packed) attributes from RTS structs #2764

Merged

osa1 added 3 commits September 10, 2021 12:22

Merge remote-tracking branch 'origin/master' into osa1/update_rustc

64ee19d

Revert "More syntactic noise to work around unaligned ref warnings"

e131e61

This reverts commit 655d5e3.

Revert more changes

5aa4d24

osa1 force-pushed the osa1/update_rustc branch from b86b821 to 5aa4d24 Compare September 10, 2021 09:28

osa1 requested review from nomeata, crusso and ggreif September 10, 2021 09:31

osa1 marked this pull request as ready for review September 10, 2021 09:31

nomeata approved these changes Sep 10, 2021

View reviewed changes

osa1 added a commit that referenced this pull request Sep 16, 2021

Use repr(C) in RTS struct

7df9955

See the node added with this commit and #2761 (comment) for the problem this solves.

osa1 mentioned this pull request Sep 16, 2021

Use repr(C) in RTS struct #2788

Merged

mergify bot pushed a commit that referenced this pull request Sep 16, 2021

Use repr(C) in RTS struct (#2788)

a71a21b

See the node added with this commit and #2761 (comment) for the problem this solves.

osa1 added 2 commits October 2, 2021 17:28

Merge remote-tracking branch 'origin/master' into osa1/update_rustc

6b1bee6

WIP: Start manually vendoring deps

be1eb43

Merge remote-tracking branch 'origin/master' into osa1/update_rustc

74bf016

osa1 added 8 commits October 30, 2021 15:57

Update rts/README.md

cefe4dc

Update rts/README.md

2aa4eaa

Revert accidental change

bac4043

Revert more changes to keep diff as small as possible

1bc1eee

Remove redundant .gitignore

570bf88

Update cargo-vendor-tools authors

2d8e01d

Minor rewording in vendor-rust-std-deps help

91096be

More comments

3bae8cf

osa1 requested a review from nomeata October 30, 2021 13:58

ggreif approved these changes Oct 30, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into osa1/update_rustc

9fc26e7

nomeata approved these changes Oct 31, 2021

View reviewed changes

default.nix Outdated Show resolved Hide resolved

default.nix Outdated Show resolved Hide resolved

default.nix Outdated Show resolved Hide resolved

default.nix Outdated Show resolved Hide resolved

Remove commented-out deps, redundant nix functions, unused dep

7a56781

nomeata mentioned this pull request Oct 31, 2021

CI: Add a new workflow that tries to automatically update nix hashes #2871

Merged

osa1 added the automerge-squash When ready, merge (using squash) label Oct 31, 2021

mergify bot merged commit 692bdb0 into master Oct 31, 2021

mergify bot deleted the osa1/update_rustc branch October 31, 2021 11:50

mergify bot removed the automerge-squash When ready, merge (using squash) label Oct 31, 2021

osa1 mentioned this pull request Oct 31, 2021

Intermittent test failure on CI (stable-mem-32) #2872

Open

This was referenced Nov 8, 2021

Bump rustc version #2253

Closed

Use cargo's build-std instead of xargo #2251

Closed

yorickvP mentioned this pull request May 23, 2022

[Nix] Frontend packaging MinaProtocol/mina#10077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Rust #2761

Update Rust #2761

osa1 commented Sep 1, 2021 •

edited

Loading

nomeata left a comment

osa1 commented Sep 13, 2021

osa1 commented Sep 13, 2021

osa1 commented Sep 13, 2021

osa1 commented Sep 16, 2021

ggreif commented Sep 16, 2021

osa1 commented Sep 16, 2021 •

edited

Loading

osa1 commented Sep 16, 2021

osa1 commented Sep 23, 2021 •

edited

Loading

osa1 commented Oct 30, 2021

osa1 commented Oct 30, 2021

ggreif left a comment •

edited

Loading

nomeata left a comment

osa1 commented Oct 31, 2021

osa1 commented Oct 31, 2021 •

edited

Loading

nomeata commented Oct 31, 2021

Update Rust #2761

Update Rust #2761

Conversation

osa1 commented Sep 1, 2021 • edited Loading

nomeata left a comment

Choose a reason for hiding this comment

osa1 commented Sep 13, 2021

osa1 commented Sep 13, 2021

osa1 commented Sep 13, 2021

osa1 commented Sep 16, 2021

ggreif commented Sep 16, 2021

osa1 commented Sep 16, 2021 • edited Loading

osa1 commented Sep 16, 2021

osa1 commented Sep 23, 2021 • edited Loading

osa1 commented Oct 30, 2021

osa1 commented Oct 30, 2021

ggreif left a comment • edited Loading

Choose a reason for hiding this comment

nomeata left a comment

Choose a reason for hiding this comment

osa1 commented Oct 31, 2021

osa1 commented Oct 31, 2021 • edited Loading

nomeata commented Oct 31, 2021

osa1 commented Sep 1, 2021 •

edited

Loading

osa1 commented Sep 16, 2021 •

edited

Loading

osa1 commented Sep 23, 2021 •

edited

Loading

ggreif left a comment •

edited

Loading

osa1 commented Oct 31, 2021 •

edited

Loading