Repository navigation
[ObjCRuntime] Improve super calls - Fixes #25362 - #25376
Conversation
Replace this.SuperHandle with a stack-allocated ObjCSuper struct passed by pointer to objc_msgSendSuper calls. Add GC.KeepAlive (this) after each super call to prevent premature collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add unsafe { } blocks around code that takes the address of
__objc_super__ in files where the enclosing method is not already
marked unsafe.
Files modified:
- src/CoreImage/CIVector.cs
- src/CoreImage/CIFilter.cs (2 locations)
- src/UIKit/UIPasteboard.cs (2 locations)
- src/Foundation/NSObject2.cs (3 locations)
- src/Foundation/NSHttpCookie.cs
- src/GameController/GCMouse.cs (2 locations)
- src/AppKit/NSBitmapImageRep.cs
- src/CoreAnimation/CALayer.cs
Files already in unsafe context (no changes needed):
- src/Foundation/NSInputStream.cs
- src/Foundation/NSThread.cs
- src/Foundation/NSUuid.cs
- src/AppKit/NSOpenGLPixelFormat.cs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
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.
|
/review |
|
✅ .NET for Apple Platforms PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully modernizes super call handling by replacing heap-allocated objc_super structures (via NSObject.SuperHandle) with stack-allocated ObjCSuper structs. The changes are well-executed and consistent across both generated and handwritten code.
✅ Strengths
- Correct memory management: All super call sites properly create
ObjCSuperon the stack and includeGC.KeepAlive (this)to prevent premature collection - Consistent pattern: The bgen generator correctly emits the new pattern with
unsafeblocks, stack allocation, and GC safety - Binary layout correctness: The
ObjCSuperstruct uses[StructLayout(LayoutKind.Sequential)]with the correct field order (receiver,classHandle) matching the nativeobjc_superlayout - Backward compatibility:
NSObject.SuperHandleis preserved (mentioned in PR description as simplifiable in XAMCORE_5_0) - Proper P/Invoke signatures: All
objc_msgSendSupervariants correctly updated to takeObjCSuper*instead ofIntPtr
🎯 Findings
- 3 suggestions for minor improvements to API design, code organization, and documentation
Key Changes Validated
- ✅
ObjCSuperstruct correctly implementsobjc_superlayout - ✅ All handwritten super calls (11 files) updated consistently
- ✅ Generator correctly emits unsafe blocks with stack allocation
- ✅
GC.KeepAlive (this)present after all super calls - ✅ Return values properly captured before
GC.KeepAlivewhere needed - ✅ Test expectations updated for size changes
- ✅ CI checks passing (many completed, remainder in progress)
The implementation is sound and ready for merge after considering the minor suggestions above.
Generated by .NET for Apple Platforms PR Reviewer for issue #25376 · ● 7.1M
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.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #7966a34] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [PR Build #7966a34] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) Unable to create gist: Response status code does not indicate success: 502 (Bad Gateway). (raw diff) - Please review changes) Pipeline on Agent |
✅ [PR Build #7966a34] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #7966a34] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 175 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Store the super handle in a ConditionalWeakTable. * Allocate native memory for it. * Cache that native memory in a ConditionalWeakTable. * Free it when the NSObject is freed (but not before). * Obsolete the SuperHandle property in .NET 11, and remove it in XAMCORE_5_0. This is less performant, but any performance issues will be alleviated by the fact that NSObject.GetSuper () will be called much less after #25376. The great advantage is that NSObjectData is now smaller, and fits in the tagged memory returned by `ObjectiveCMarshal.CreateReferenceTrackingHandle`, which will simplify memory management _a lot_ for CoreCLR. Contributes towards #25383. This is a continuation/simplification of #25405.
Summary
Introduces a stack-allocated
ObjCSuperstruct to replace the use ofNSObject.SuperHandleforobjc_msgSendSupercalls. This eliminates the need to allocate and manage unmanaged memory for theobjc_superstruct on every super call.Changes
New
ObjCSuperreadonly ref struct (src/ObjCRuntime/ObjCSuper.cs): A[StructLayout(LayoutKind.Sequential)]struct withReceiverandClassHandlefields matching the nativeobjc_superlayout.Updated bgen code generation: P/Invoke declarations for
objc_msgSendSupervariants now takeObjCSuper*instead ofIntPtras the first parameter. Generated call sites create a stack-allocatedObjCSuperand pass&__objc_super__instead ofthis.SuperHandle.Updated handwritten super calls in 11 source files (CIFilter, CIVector, CALayer, UIPasteboard, NSInputStream, NSUuid, NSThread, NSHttpCookie, GCMouse, NSOpenGLPixelFormat, NSBitmapImageRep) plus NSObject2.cs.
NSObject.SuperHandleis preserved for backward compatibility (can be simplified/removed in XAMCORE_5_0).Fixes #25362
🤖 Pull request created by Copilot