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

Add support for std #1069

Open
wants to merge 12 commits into
base: theseus_main
Choose a base branch
from
Open

Conversation

tsoutsman
Copy link
Member

@tsoutsman tsoutsman commented Nov 6, 2023

The CI is too slow to even check out the rust submodule, let alone build the compiler. Probably best to create a separate branch rather than merging into theseus_main.

To upstream this we would have to:

  • Implement the rest of std functionality
  • Publish theseus_ffi and theseus_shim to crates.io

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
.ignore Outdated Show resolved Hide resolved
shim/src/lib.rs Outdated Show resolved Hide resolved
shim/src/lib.rs Outdated Show resolved Hide resolved
unsafe { thread_local_macro::register_dtor(t, dtor) }
}

// TODO: Something better than transmutations?
Copy link
Member Author

Choose a reason for hiding this comment

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

Bump.

Copy link
Member

Choose a reason for hiding this comment

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

yea, can't we use a typical conversion method for this? I know there are existing fat pointer types (pointers w/ metadata) in libcore, perhaps we can leverage that (or just DIY) to restrict the interface and types that can be used to convert to/from FatPointers

Copy link
Member

Choose a reason for hiding this comment

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

Check out this section about strict provenance and the types included therein
https://doc.rust-lang.org/beta/core/ptr/index.html#strict-provenance

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the problem is that the types in core aren't FFI-safe.

yea, can't we use a typical conversion method for this?

I mean the function (at least for moving from a fat pointer to a trait object) is ultimately unsafe. There's nothing guaranteeing that the implementation of the function on the other side of the FFI boundary is really returning a fat pointer corresponding to e.g. Writer.

Makefile Outdated Show resolved Hide resolved
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
///
/// # Safety
///
/// `object_ptr` must point to a valid object, and `dtor` must be the destructor for that object.
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this safety doc correct?

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
@tsoutsman
Copy link
Member Author

@kevinaboos should be ready for review

@kevinaboos
Copy link
Member

@kevinaboos should be ready for review

Great! I'll take a look tonight or tomorrow late afternoon.

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

So i know that in our mtg, you said that if we used something like stabby or abi_stable, we'd have to infect the rest of the codebase with their specific Arc-like types. But I wonder how it would be any different than defining our own types in theseus_ffi -- couldn't we just have the shim and libtheseus layers convert to/from those existing ABI-stable FFI types, regardless of whether they came from our lib or another lib? Seems like we could save the effort (and potential soundness/safety sharp edges) of implementing and maintaining our own FFI types by just using their types for that specific set of crates only.

My other major concern is mostly just the transmutes used everywhere, hopefully we can use existing fat pointer types with explicit to/from conversions instead.

shim/src/lib.rs Outdated Show resolved Hide resolved
shim/src/lib.rs Outdated Show resolved Hide resolved

#[derive(Debug, Clone)]
#[repr(C)]
pub struct FatPointer {
Copy link
Member

Choose a reason for hiding this comment

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

this can impl Send/Sync if and only if it actually comes from types that impl Send/Sync. Here I don't see that being the case yet, as it's always transmuted to/from something.

Instead, it'd be cleaner and safer to use a From/Into impl on limited types that you know are safe to convert to/from fat pointers like this. Given a set of closed conversion methods like that, then it might be okay to impl Send/Safe.

unsafe { thread_local_macro::register_dtor(t, dtor) }
}

// TODO: Something better than transmutations?
Copy link
Member

Choose a reason for hiding this comment

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

yea, can't we use a typical conversion method for this? I know there are existing fat pointer types (pointers w/ metadata) in libcore, perhaps we can leverage that (or just DIY) to restrict the interface and types that can be used to convert to/from FatPointers

unsafe { thread_local_macro::register_dtor(t, dtor) }
}

// TODO: Something better than transmutations?
Copy link
Member

Choose a reason for hiding this comment

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

Check out this section about strict provenance and the types included therein
https://doc.rust-lang.org/beta/core/ptr/index.html#strict-provenance

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.ignore Outdated Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
@@ -39,3 +39,6 @@ github_pages/doc/

# library lock files
/libs/**/Cargo.lock

# TODO: Remove once std is upstreamed
.cargo/
Copy link
Member

Choose a reason for hiding this comment

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

what is this, something that the build of rust generates?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we set the specific path to our custom rustc installation in .cargo/config.toml.

@tsoutsman
Copy link
Member Author

couldn't we just have the shim and libtheseus layers convert to/from those existing ABI-stable FFI types, regardless of whether they came from our lib or another lib?

So not really. E.g. we call app_io::stdout and get an alloc::sync::Arc<dyn Writer>. We can't call stabby::Arc::from_raw(alloc::sync::Arc::into_raw(writer)) because stabby::Arc and sync::Arc could have different layouts. And there's not really much else we could do. We can't get an owned reference to the dyn Writer and so we can't turn it into a stabby trait object and wrap it in a stabby::Arc.

And conceptually that makes sense. writer is layed out in memory as an alloc::sync::Arc<dyn Writer> and there is no way we can interpret it as a stabby::Arc<vtable!(dyn Writer)> because they have different layouts.

Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
Signed-off-by: Klimenty Tsoutsman <klim@tsoutsman.com>
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.

2 participants