Skip to content

Convert ManagedThread to Rust#2812

Merged
sporksmith merged 10 commits intoshadow:mainfrom
sporksmith:mthread
Mar 31, 2023
Merged

Convert ManagedThread to Rust#2812
sporksmith merged 10 commits intoshadow:mainfrom
sporksmith:mthread

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Mar 28, 2023

Probably easiest to review a commit at a time.

The first commit tries to do the "dumbest" conversion possible, while minimizing design changes. It includes several FIXMEs for things that are fixed in follow-up commits in this PR.

There's still room for more improvements and refactoring, but I think this is a good-enough point to merge.

@sporksmith sporksmith self-assigned this Mar 28, 2023
@github-actions github-actions bot added Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable labels Mar 28, 2023
@sporksmith sporksmith force-pushed the mthread branch 3 times, most recently from 8c01fa0 to af8c5aa Compare March 30, 2023 22:15
@sporksmith
Copy link
Copy Markdown
Contributor Author

@sporksmith sporksmith marked this pull request as ready for review March 30, 2023 22:29
@sporksmith sporksmith requested a review from stevenengler March 30, 2023 22:32
This has several issues to be fixed in follow-up commits in this PR,
called out with "FIXME" comments.
This is the shortest route to avoiding double mutable borrows of the
ManagedThread itself.
(This was missing in the C code)
Spawning a thread requires an owned copy of argv; pass ownership all the
way through the stack instead of copying "on-demand".
We currently emulate all the supported operations on these, and ought to
emulate any additional operations we later want to support, if any.
The shmem allocator doesn't support such large alignments. The safe Rust
wrapper panics if the allocation doesn't happen to be aligned enough.

It wouldn't have been giving us this alignment before either; we just
didn't have the alignment validation when going through the C API.

I already suspected the performance difference in the PR this was likely
to have been some other artifact; it seems even more likely now.
Removing it.
Also propagates this backwards a bit for some Thread APIs to take
`&ProcessContext`.

On one hand, this means passing along more "global" state than strictly
necessary. OTOH I think this will help limit the pain of having to
propagate fine-grained requiredments backwards through call stacks when
we need one of these objects in some deeply nested function.

I still left finer-grained parameters for non-pub APIs, since it's not
as painful to refactor those within a single module.
This fixes a FIXME in `Thread` where we were getting the current
`Process` and `Host` from the `Worker`. This propagates passing them
explicitly instead, using e.g. `ThreadContext`.
@sporksmith sporksmith enabled auto-merge March 31, 2023 18:14
@sporksmith sporksmith merged commit ffd6349 into shadow:main Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants