- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
[NativeAOT/Apple] Map the thunks using vm_allocate/vm_remap #95914
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
Conversation
…ed for specific executable segment layout
| Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsMap the thunks using vm_allocate/vm_map to avoid need for specific executable segment layout. Fixes #94655 
 | 
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.
Thank you!
| Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos Issue DetailsMap the thunks using vm_allocate/vm_map to avoid need for specific executable segment layout. Fixes #94655 
 | 
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.
LGTM, thank you!
I just left a minor question/comment above.
@filipnavara would it make sense to add some kind of unit test verifying that the app has only a single executable segment?
| do | ||
| { | ||
| ret = vm_remap( | ||
| mach_task_self(), &addr, templateSize, 0, VM_FLAGS_FIXED | VM_FLAGS_OVERWRITE, | 
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.
Just trying to understand the difference here, Mono seems to pass only FALSE in place of flags when calling vm_remap, why are we passing VM_FLAGS_FIXED | VM_FLAGS_OVERWRITE instead (or vice versa should we use the same on Mono side as well)?
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.
We should use this on Mono side as well. Mono uses a loop with vm_allocate(2*size)+vm_deallocate(1*size)+vm_remap. There's a race condition between the vm_deallocate+vm_remap not ending up at the same address. If the race condition is hit then it starts again from the beginning. VM_FLAGS_OVERWRITE gets rid of this race condition by skipping the intermediate reallocation.
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.
/cc: @lambdageek ^
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.
Yea seems like a good idea (I'm not intimately familiar with our infinite trampolines code, but it seems workable).
So instead of the loop we would do a single pass:
- vm_allocate(2*size)returning- addr
- set tpage = addr+size
- vm_remap(&tpage, VM_FLAGS_FIXED | VM_FLAGS_OVERWRITE)
- if success, we're done; if failure, dump core
?
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.
Yep
| 
 I don't think it's necessary. You have to go out of your way to create a second executable segment, it doesn't happen without an explicit effort to do so. | 
| /backport to release/8.0-staging | 
| Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7196873974 | 
| Thanks for review and backport. | 
Map the thunks using vm_allocate/vm_remap to avoid need for specific executable segment layout.
Fixes #94655