Skip to content

Conversation

@filipnavara
Copy link
Member

@filipnavara filipnavara commented Dec 12, 2023

Map the thunks using vm_allocate/vm_remap to avoid need for specific executable segment layout.

Fixes #94655

@ghost ghost added area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member labels Dec 12, 2023
@ghost
Copy link

ghost commented Dec 12, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Map the thunks using vm_allocate/vm_map to avoid need for specific executable segment layout.

Fixes #94655

Author: filipnavara
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@filipnavara filipnavara marked this pull request as ready for review December 12, 2023 17:50
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jkotas jkotas added the os-ios Apple iOS label Dec 12, 2023
@ghost
Copy link

ghost commented Dec 12, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Map the thunks using vm_allocate/vm_map to avoid need for specific executable segment layout.

Fixes #94655

Author: filipnavara
Assignees: -
Labels:

os-ios, community-contribution, area-NativeAOT-coreclr

Milestone: -

@filipnavara filipnavara changed the title [NativeAOT/Apple] Map the thunks using vm_allocate/vm_map [NativeAOT/Apple] Map the thunks using vm_allocate/vm_remap Dec 12, 2023
Copy link
Member

@ivanpovazan ivanpovazan left a 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,
Copy link
Member

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)?

Copy link
Member Author

@filipnavara filipnavara Dec 13, 2023

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc: @lambdageek ^

Copy link
Member

@lambdageek lambdageek Dec 14, 2023

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

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@filipnavara
Copy link
Member Author

would it make sense to add some kind of unit test verifying that the app has only a single executable segment?

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.

@jkotas jkotas merged commit 1e135d1 into dotnet:main Dec 13, 2023
@jkotas
Copy link
Member

jkotas commented Dec 13, 2023

/backport to release/8.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7196873974

@filipnavara
Copy link
Member Author

Thanks for review and backport.

@filipnavara filipnavara deleted the thunks_vm branch December 13, 2023 15:24
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member os-ios Apple iOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NativeAOT] App Store Connect warning ITMS-90999: Invalid executable

4 participants