Implement minimal socket shim #3426
Conversation
There was a problem hiding this comment.
Thanks for the PR!
However, as-is I don't think landing this would be very useful. Does it even make sense to support socket alone? As you said we can't really write any tests, so this doesn't actually make any program work with Miri. Likely this entire function will have to be rewritten as we discover how sockets can actually be implemented.
I think we need a plan for what the support for socket + bind + connect looks like. Once we have a plan we can land it step-by-step, but it makes no sense to start going without a plan for where we want to go. In particular, clearly the Socket type can't just be a unit struct. We need a sketch for what that type should look like. Then we can land this in multiple PRs, but at least there will be a perspective for where we are going.
If we land this as-is it will just be another useless stub next to socketpair, a half-finished construction site without any plan or vision or idea for what to do next -- that's not really useful for anything. I don't like the existing construction site, I regret that we landed it. I don't want us to add another one.
src/shims/unix/linux/fd.rs
Outdated
|
|
||
| let _domain = this.read_scalar(domain)?.to_i32()?; | ||
| let _type_ = this.read_scalar(type_)?.to_i32()?; | ||
| let _protocol = this.read_scalar(protocol)?.to_i32()?; |
There was a problem hiding this comment.
Unless we intend to support all values here, we should throw_unsupported! for socket types we do not support.
| @@ -88,6 +88,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { | |||
| let ptr = this.mremap(old_address, old_size, new_size, flags)?; | |||
| this.write_scalar(ptr, dest)?; | |||
| } | |||
| "socket" => { | |||
There was a problem hiding this comment.
socket is a standard Unix/POSIX function, why is this in the "linux" module?
Don't copy the existing socketpair, it was landed with a "we'll fix the issues later" attitude but "later" never happened.
|
☔ The latest upstream changes (presumably #3441) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I have opened an issue to track this: #3449. I think blocking sockets are actually highly non-trivial to implement. |
|
So to be clear I think before any implementation work is done, we need a clear description of which part of the socket API is intended to be implemented (TCP/UDP/..., blocking/non-blocking, which shims are involved). And since this will be non-trivial to implement, we also need a description of what the implementation plan is -- which new state is needed in Miri, how do we handle waking up the right threads when the sockets they are blocked on get ready, things like that. I laid out some of the complications in #3449. Adding sockets is not an easy project. And with projects of this scale it's not a good idea to just go implement something -- that has a pretty high risk that we'll ask you to re-write it entirely as we see flaws with the fundamental structure of the approach, and it also makes review much harder when we have to reverse engineer the high-level approach from the code. @ZoeS17 what is your goal with this PR? Is there a motivation for wanting to land just the |
|
☔ The latest upstream changes (presumably #3451) made this pull request unmergeable. Please resolve the merge conflicts. |
Agreed.
My intent was originally to land this as a shim without supporting sockets at all but given what has been said in the #3449 I agree that it seems foolish to continue as a "hollow" implementation. I originally come upon the desire to pick up this work because I was attempting to run miri against a program I was building that utilizes sockets to connect to IRC. Since obviously that needs a deeper implementation then I originally understood I'm up for closing this PR with an understanding that I'm willing to admit I may be out of my depth here. |
Ah, yes makes sense. I'm afraid that's a pretty hard problem and quite far off currently, unfortunately. I'll close this PR then. Thanks anyway! |
I vaguely followed socketpair as to implementation details. I attempted to make a test too but there's not exactly a good way, that I can figure out, besides simply checking that an file descriptor gets made. I dd so for both the libc crate and the version I implemented.
Free feel to ask question and/or adjust if needed.