-
Notifications
You must be signed in to change notification settings - Fork 516
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
[Foundation] Don't dispose tagged pointers. Fixes #21425. #21446
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -573,6 +573,9 @@ | |||
<!-- Set default ValidateObjectPointers value --> | |||
<_ValidateObjectPointers Condition="'$(_ValidateObjectPointers)' == ''">false</_ValidateObjectPointers> | |||
|
|||
<!-- Set default DisposeTaggedPointers value --> | |||
<_DisposeTaggedPointers Condition="'$(_DisposeTaggedPointers)' == ''">true</_DisposeTaggedPointers> |
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.
Isn't the default supposed to be false
? How does the test pass? (Or, am I just misreading 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 was just a quick test run on CI to see if doing it this way would break any test, the PR isn't complete at all.
|
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
✅ API diff for current PR / commit.NET (No breaking changes)✅ API diff vs stable.NET (No breaking changes)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
💻 [CI Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 99 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Objective-C has the concept of a tagged pointer, which is not really a pointer, but instead a pointer-like value that has encoded the constant value inside itself.
More information about tagged pointers can be found online:
The important part for us is that there's no need for reference counting tagged pointers - they're constant values, and there's nothing allocated, nor does anything need to be freed either. Another point is that these tagged pointers can pop up at any time from Objective-C, and that causes a problem:
This PR attempts to fix this by detecting a tagged pointer, and in that case we don't disconnect the managed instance from the native pointer, because the native pointer will never point to freed memory (it'll always be a valid object).
The unfortunate part is that detecting a tagged pointer is not possible with public API, we have to look at the actual bits, and this is considered an internal implementation detail by Apple.
To make this as safe as possible:
TODO:
Fixes #21425.