Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Add move-native to workspace, fix formatting etc. #244

Merged
merged 5 commits into from Jul 21, 2023

Conversation

brson
Copy link
Collaborator

@brson brson commented Jul 18, 2023

This makes cargo xfmt and cargo xclippy work with move-native by adding move-native to the workspace. cargo xlint already applies to move-native. CI will now run these on move-native.

The reason move-native was not part of the workspace is because it is compiled as a staticlib. Previously this caused workspace operations like cargo check to attempt to compile move-native as a staticlib for the host platform. Staticlibs are expected to contain necessary lang-items like panic handlers and allocators, but move-native does not try to declare these for platforms other than solana, and this caused the build to fail.

This resolves that problem by not declaring move-native a staticlib in its manifest. Now normal build operations just build it as a rlib, which is allowed to not contain those lang items since it must be linked to other rlibs to be used. As a result of this, to compile move-native correctly for solana one must use a lesser-known command:

cargo rustc --crate-type=staticlib

and this is what rbpf-tests does.

One might try to instead declare the correct lang-items when compiled for various host platforms, but I am not aware of a way to do so only when compiling a staticlib, and not an rlib. And I see it as an uglier solution even if it is possible.

Closes #237

@brson
Copy link
Collaborator Author

brson commented Jul 18, 2023

@jcivlin this changes how move-native must be compiled for production, described in the op, which might affect your work on compiling from source.

@@ -119,14 +118,14 @@ pub struct MoveBorrowedRustVecMut<'mv, T> {

impl<'mv, T> Drop for MoveBorrowedRustVec<'mv, T> {
fn drop(&mut self) {
let rv = mem::replace(&mut self.inner, Vec::new());
let rv = mem::take(&mut self.inner);
mem::forget(rv);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do wee need to call mem::drop(rv) as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. mem::forget takes rv by value so it is no longer in scope to be passed to any other function.

We call forget here because rv is type Vec, which owns its interior buffer pointer, but this Vec was temporarily fabricated from a MoveUntypedVector, which actually owns that buffer. So forgetting the Vec quietly destroys it without running the Vec destructor - so that the original MoveUntypedVector can continue owning that buffer.

It is never required to call mem::drop because it does nothing but run the value's destructor, which would have also happened without passing it to mem::drop. Calling mem::drop is sometimes done for clarity, and sometimes to force a value to be dropped before it would naturally go out of scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only reason to call mem::forget is to not run a destructor.

pub use crate::rt_types::TypeDesc;
pub use crate::rt_types::MOVE_UNTYPED_VEC_DESC_SIZE;
pub use crate::rt_types::MOVE_TYPE_DESC_SIZE;
pub use crate::rt_types::{TypeDesc, MOVE_TYPE_DESC_SIZE, MOVE_UNTYPED_VEC_DESC_SIZE};
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a linter that does this automatically?

Copy link

Choose a reason for hiding this comment

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

is there a linter that does this automatically?

Yes, cargo x fmt does that.

Copy link

@nvjle nvjle left a comment

Choose a reason for hiding this comment

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

Other than needing to fix the clippy error in move-native/src/tests.rs, LGTM.

error: call to `std::mem::drop` with a value that does not implement `Drop`. Dropping such a type only extends its contained lifetimes
   --> language/move-native/src/tests.rs:164:9
    |
164 |         drop(new_element_vec);
    |         ^^^^^^^^^^^^^^^^^^^^^
    |
note: argument has type `rt_types::MoveUntypedVector`
   --> language/move-native/src/tests.rs:164:14
    |
164 |         drop(new_element_vec);
    |              ^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_non_drop
    = note: `-D clippy::drop-non-drop` implied by `-D warnings`

@brson brson merged commit 3cebf06 into anza-xyz:llvm-sys Jul 21, 2023
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] move-native is not formatted correctly
3 participants