ChildPidWatcher improvements #2823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Component: Main
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`.
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.
These work on Shadow 1.x configurations and topologies. They or the tests would need to be updated to keep up with the Shadow 3.x changes. It's probably not worth doing so. Users who need to migrate from a 1.x shadow can still get these from 2.x Shadow releases, and then migrate from 2.x to 3.x the same way as other users (TBD whether there will be new tools).
One fix is in a log statement, and the other is in `AllocdMem` which we only ever use as a `AllocdMem<u8>`, so neither of these currently cause incorrect behaviour.
One fix is in a log statement, and the other is in `AllocdMem` which we only ever use as an `AllocdMem<u8>`, so neither of these currently cause incorrect behaviour.
This is a planned breaking change for the Shadow 3.0 release. See shadow#2496 and shadow#2447 (comment). This forces users to explicitly use "./" to specify a binary in the current directory.
`UntypedForeignPtr` should only be used in C code and maps to `ForeignPtr<()>` in rust. This commit changes all existing rust uses of `ForeignPtr` to `ForeignPtr<()>`, allowing us to start changing the generic type in the future.
Used the following command: ``` rg 'TypedArrayForeignPtr' --files-with-matches | xargs sed -i 's/TypedArrayForeignPtr/ForeignArrayPtr/g' ```
Realized a better way of preventing type inference without requiring a second generic.
Follow-up to shadow#2813. This makes `ForeignPtr` typed. There aren't many places where we need untyped pointers in Shadow, so making pointers typed should be the default. This also means that for a lot of pointers, we can define their type directly in the syscall handler. Pointer types should now be self-documenting, and it should be harder to accidentally access the pointers as the wrong type. An `UntypedForeignPtr` was added for compatibility with the C code. - added a generic type to `ForeignPtr` - added `UntypedForeignPtr` for C code which maps to `ForeignPtr<()>` in rust code - renamed `TypedArrayForeignPtr` to `ForeignArrayPtr` The intermediate commits have a lot of casts between pointer types so that each commit would build and run, but the final code should have only a few pointer type conversions. There are still some things that can be cleaned up, but I think this gets most of the code in.
This also renames it to `read` and removes the UB from the `MaybeUninit::uninit().assume_init()` (part of shadow#2555).
stevenengler
approved these changes
Apr 3, 2023
We wrap the fd in a `File` to ensure its closed on Drop. Technically this was a file descriptor leak, but it wouldn't have mattered in practice in Shadow since we only create ChildPidWatcher, and only drop it when exiting the Shadow process.
We were already closing this in the right place, but this makes ownership clearer and will help avoid forgetting to close it in future changes.
We were manually managing shared ownership using a raw fd. Make this more explicit by wrapping the File object in an Arc.
github-actions
bot
added
Component: Build
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FileandArcto manage file descriptors instead of manually managingRawFds. In particular this fixes a bug that the command notifier descriptor was never getting closed. This wouldn't have affected Shadow, since there would only be one such file, and it lives for the full lifetime of the Shadow process anyway.