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.
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
Implement futex_wait and futex_wake. #1568
Implement futex_wait and futex_wake. #1568
Changes from 8 commits
1ffc5bb
281a538
6c2f36e
69cea1d
1c582e7
c2fa27c
712e800
ee3eb4b
dabd980
422b505
d5b3f54
e64ead2
8113882
c9627b2
924fd56
6628275
5880e7d
6df54c4
9d764c5
dc36988
dfcb46a
c268ee2
68776d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, didn't you say that this point may in fact be dangling? That won't work like this.
deref_operand
requires that the pointer be dereferncable for the given type.I am okay leaving this as a FIXME though. But eventually we should have a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did! I (mis)interpreted your comment above ..
.. as that only
read_scalar()
requires the address to be readable, and thatderef_operand
only does the conversion to a pointer.Anyway, this was the reason I kept the original
addr
around as aTyOp
, separate from thePointer
, such that the FUTEX_WAIT path can still callderef_operand
(orread_scalar_at_offset
, which already doesderef_operand
) on it.What's a good way to do this? Both operations need
addr
asPointer
, but one also needs to dereference the pointer (as i32).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using
force_ptr
now instead ofderef_operand
. But using thatPointer
to read thei32
pointee in FUTEX_WAIT seems tricky.Now I'm using
check_ptr_access
followed bythis.memory.get_raw(addr.alloc_id)?.read_scalar
, but I don't know if that's a proper way to do it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a tricky situation which I do not think we've had before. But
get_raw
feels like going down way too far the stack of abstractions.I think the best way is to delay part of the decision: here at the top, just do
let futex_val = this.read_immediate
. Then also get the ptr value viaforce_ptr(futex_val.to_scalar()?)
. In thefutex_wait
branch, doderef_operand(futex_val.into())?
. At least I think that should work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That only works if I do:
or
in the futex_wait branch. Otherwise it refuses to read a
AtomicI32
asi32
again.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused actually, why does this fail? Without
deref_operand
I think this should work, but I might forget some check somewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or was the actual problem that you passed
0usize
? In that case the value would have been the problem, not the type -- i.e.,0usize as *const ()
would not work either. That is what the last comment that I added is about.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in the example I also used the
usize
for the wait operation, which does dereference it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it doesn't do that with
deref_operand
. So as long as the pointer points to something valid, with your current code, I think it should work fine both with usize and with a pointer type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh,
read_scalar_at_offset
usesderef_operand
internally...