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

Fix unaligned dereferencing (preparation for Rust upgrade) #3981

Merged
merged 22 commits into from
May 24, 2023

Conversation

luc-blaeser
Copy link
Contributor

Analysis:

  • Unaligned pointer dereferencing has undefined behavior in Rust: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
  • Using 64-bit fields in a struct implies that the struct is also 64-bit-aligned. This is however not guaranteed in our heap as objects are only 32-bit aligned.
  • The recent Rust version occasionally checks misaligned pointer dereferencing. This currently fails in Stream.rs and in BitmapIterator.rs for the Compacting GC. However, it is correctly aligned in the incremental GC.
  • Simplification: The implicit padding for 64-bit field alignment depending on the header size and the GC (incremental or not) is now removed.

Intended as a fix for the build errors in #3947.

@luc-blaeser luc-blaeser self-assigned this May 15, 2023
@luc-blaeser
Copy link
Contributor Author

luc-blaeser commented May 15, 2023

Update: I also resolved an issue with the wasm-profiler that showed an error due to an unsupported WASM instruction for number conversion.

@github-actions
Copy link

github-actions bot commented May 15, 2023

Comparing from cf341ee to 759db5f:
In terms of gas, no changes are observed in 4 tests.
In terms of size, 4 tests regressed and the mean change is +0.0%.

@luc-blaeser luc-blaeser marked this pull request as ready for review May 16, 2023 09:06
@luc-blaeser luc-blaeser requested review from ggreif and crusso May 16, 2023 09:06
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM

We'll need to be careful that @matthewhammer's regions PR doesn't introduce more 64-bit object fields.

nix/default.nix Outdated Show resolved Hide resolved
src/codegen/compile.ml Outdated Show resolved Hide resolved
rts/motoko-rts/src/types.rs Outdated Show resolved Hide resolved
rts/motoko-rts/src/stream.rs Outdated Show resolved Hide resolved
rts/motoko-rts/src/types.rs Outdated Show resolved Hide resolved
rts/motoko-rts/src/types.rs Outdated Show resolved Hide resolved
rts/motoko-rts/src/types.rs Outdated Show resolved Hide resolved
src/codegen/compile.ml Show resolved Hide resolved
rts/motoko-rts/src/gc/mark_compact/bitmap.rs Show resolved Hide resolved
luc-blaeser and others added 6 commits May 22, 2023 09:26
Co-authored-by: Claudio Russo <claudio@dfinity.org>
Co-authored-by: Claudio Russo <claudio@dfinity.org>
Keep the current version and prepare upgrade for later
@luc-blaeser luc-blaeser changed the title Fix unaligned dereferencing issue in rustc upgrade Fix unaligned dereferencing issue to prepare future rustc upgrade May 24, 2023
@luc-blaeser
Copy link
Contributor Author

luc-blaeser commented May 24, 2023

Comparing from 66c55a1 to f2aaa0b: In terms of gas, 2 tests regressed, 2 tests improved and the mean change is +0.0%. In terms of size, 4 tests improved and the mean change is -12.4%.

The performance of this PR without Rust compiler upgrade has been remeasured in #3999. No difference compared to master.

@luc-blaeser luc-blaeser deleted the luc/rust-upgrade branch May 24, 2023 07:40
@luc-blaeser luc-blaeser restored the luc/rust-upgrade branch May 24, 2023 07:42
@luc-blaeser luc-blaeser reopened this May 24, 2023
@luc-blaeser luc-blaeser changed the title Fix unaligned dereferencing issue to prepare future rustc upgrade Fix unaligned dereferencing (preparation for Rust upgrade) May 24, 2023
Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

I like how you divide the 64-bit members into two consecutive 32-bit ones, and how that avoids padding. Thanks for doing this!

@luc-blaeser
Copy link
Contributor Author

I like how you divide the 64-bit members into two consecutive 32-bit ones, and how that avoids padding. Thanks for doing this!

Thank you also very much for reviewing!

@luc-blaeser
Copy link
Contributor Author

LGTM

We'll need to be careful that @matthewhammer's regions PR doesn't introduce more 64-bit object fields.

Thank you very much for the review. Yes, we need to be careful about future inserts of 64-bit fields. Once we have moved to new Rust version, the error would probably be reported at runtime.

@luc-blaeser luc-blaeser added the automerge-squash When ready, merge (using squash) label May 24, 2023
@mergify mergify bot merged commit 5ec5b58 into master May 24, 2023
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label May 24, 2023
@mergify mergify bot deleted the luc/rust-upgrade branch May 24, 2023 13:56
@ggreif ggreif mentioned this pull request May 24, 2023
1 task
mergify bot pushed a commit that referenced this pull request May 24, 2023
and a few deps too:
- `wasi-libc`

needs patched toolset from mozilla (PRs submitted: mozilla/nixpkgs-mozilla#309)

TODO:
- [x] `motoko-rts-test` fails in `stream flushing` with unaligned memory access — see #3981
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.

3 participants