Migrate most of Process from C to Rust#2631
Merged
sporksmith merged 27 commits intoshadow:mainfrom Jan 6, 2023
Merged
Conversation
21339f8 to
618b9af
Compare
Codecov ReportBase: 68.15% // Head: 68.01% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2631 +/- ##
==========================================
- Coverage 68.15% 68.01% -0.14%
==========================================
Files 201 201
Lines 29502 29852 +350
Branches 5794 5814 +20
==========================================
+ Hits 20107 20304 +197
- Misses 4784 4930 +146
- Partials 4611 4618 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
618b9af to
3175dda
Compare
Contributor
Author
|
Started a tor benchmark: https://github.com/shadow/benchmark/actions/runs/3762181499 Update: Results: https://github.com/shadow/benchmark-results/tree/master/tor/2022-12-23-T00-36-14 Looks like no change, as expected. |
3175dda to
6aaca6b
Compare
Contributor
|
|
stevenengler
reviewed
Jan 5, 2023
stevenengler
approved these changes
Jan 6, 2023
We could make accessors instead, but I think it'd just be unnecessary boilerplate.
Using it directly this way skips the runtime checking that Process does to ensure there are no incompatible borrows from the MemoryManager. I think the primary reason it was being used here was so that errors writing to memory could be handled instead of panicking. Now that process_flushPtrs supports this, we can use that instead.
The C code was attempting to detect incompatible borrows, but had some gaps. It detected incompatible borrows by the legacy "caching" memory access APIs with each-other, but not with the newer APIs. During this migration I cached the Ref or RefMut of the MemoryManager used to create the memory reference as well as the memory reference itself. This requires a self-referential data structure and is a bit fragile, but should prevent incompatible borrows more reliably than the old code. As a result, I had to add some extra flushes to ensure there weren't outstanding borrows from a syscall handler when running the strace-logging code, which also tries to access memory.
6aaca6b to
d44764f
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Best reviewed as individual commits.
This moves each member of the C Process struct into the Rust struct, and adds corresponding Rust methods. Many
process_methods are now thin wrappers around a_process_method that operates on the Rust process.I ran out of time to do the thread list, but don't foresee any major complications there. The next steps will be roughly:
process_methods in Rust, such that they're all thin wrappers around_process_methods taking the Rust Process._process_*accessors toprocess_*- i.e. they'll have almost the same signature as the methods they're replacing, except the first parameter will be a const pointer to the RustProcess.