[tools/msbuild] Add InlineDlfcnMethodsStep as an opt-in custom linker step - #24888
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in build+linker pipeline to inline ObjCRuntime.Dlfcn symbol lookups into direct __Internal P/Invokes, with post-trimming symbol discovery (ILTrim + NativeAOT) and native C code generation to keep only surviving symbol references.
Changes:
- Introduces
InlineDlfcnMethodsStep+GenerateInlinedDlfcnNativeCodeStepand correspondingInlineDlfcnMethodsMSBuild property wiring. - Adds post-trimming MSBuild tasks/targets to detect surviving inlined symbols (ILTrim scanning + NativeAOT unresolved-symbol parsing) and generate/compile native glue code.
- Extends Mach-O parsing to read unresolved symbols and adds/adjusts tests and test variations for the new modes.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/mtouch/Errors.resx | Adds new MX2254–MX2257 warning strings for the inlining step. |
| tools/mtouch/Errors.designer.cs | Regenerated resource accessors for new MX225x entries. |
| tools/linker/MonoTouch.Tuner/ProcessExportedFields.cs | Collects [Field] symbol names for compatibility mode. |
| tools/linker/MonoTouch.Tuner/ListExportedSymbols.cs | Skips adding required field symbols when dlfcn inlining is enabled. |
| tools/dotnet-linker/Steps/InlineDlfcnMethodsStep.cs | New custom linker step that rewrites Dlfcn calls into generated helpers + P/Invokes. |
| tools/dotnet-linker/Steps/GenerateInlinedDlfcnNativeCodeStep.cs | Emits a symbol list file for later post-trimming MSBuild steps. |
| tools/dotnet-linker/LinkerConfiguration.cs | Adds InlineDlfcnMethods configuration and symbol tracking collections. |
| tools/dotnet-linker/AppBundleRewriter.cs | Adds type/method references used by the new inlining step. |
| tools/common/MachO.cs | Adds LC_SYMTAB parsing and unresolved-symbol extraction for Mach-O/static libraries. |
| tests/xharness/Jenkins/TestVariationsFactory.cs | Adds new CI variations to exercise inline-dlfcn modes (incl. NativeAOT). |
| tests/monotouch-test/ObjCRuntime/DlfcnTest.cs | Adds tests for [Field]-backed constants and adjusts behavior under strict mode. |
| tests/monotouch-test/Foundation/AttributedStringTest.cs | Removes legacy field-absence test that would conflict with new behavior. |
| tests/monotouch-test/CoreFoundation/ProxyTest.cs | Removes legacy field-absence test that would conflict with new behavior. |
| tests/common/test-variations.csproj | Adds inline-dlfcn-methods-compat and inline-dlfcn-methods-strict test variations. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/PostTrimmingProcessing.cs | Generates native C glue for surviving symbols and outputs compile/link items. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/ComputeNativeAOTSurvivingNativeSymbols.cs | Filters NativeAOT unresolved symbols to determine surviving inlined-dlfcn symbols. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/CompileNativeCode.cs | Ensures output directory exists before compiling native files. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/CollectUnresolvedNativeSymbols.cs | New task to extract unresolved symbols from a NativeAOT static library. |
| msbuild/Xamarin.MacDev.Tasks/Tasks/CollectPostILTrimInformation.cs | New task to scan trimmed assemblies and cache surviving inlined-dlfcn P/Invokes. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Wires property defaults, custom linker steps, and post-trimming targets into the SDK. |
| docs/code/native-symbols.md | New doc explaining the native symbol pipeline and InlineDlfcnMethods modes. |
| docs/building-apps/build-properties.md | Documents the new InlineDlfcnMethods MSBuild property and defaults. |
| .github/copilot-instructions.md | Adds MSBuild guidance (DeviceSpecificIntermediateOutputPath) to repo copilot instructions. |
Files not reviewed (1)
- tools/mtouch/Errors.designer.cs: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
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.
This comment has been minimized.
This comment has been minimized.
1 similar comment
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.
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.
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.
Code Review Summary
This is a well-architected feature that adds build-time symbol resolution to replace runtime dlsym calls. The implementation demonstrates solid engineering with:
✅ Excellent incremental build support with per-assembly caching
✅ Correct thread-safety using volatile acquire/release semantics
✅ Comprehensive error handling with detailed warnings for unsupported patterns
✅ Clean separation between ILTrim and NativeAOT paths
✅ Proper framework availability checks for simulator builds
Review Findings
I've left 2 inline comments with suggestions for defensive improvements:
- Symbol name validation (PostTrimmingProcessing.cs): Consider validating that symbol names are valid C identifiers before generating C code
- Documentation enhancement (InlineDlfcnMethodsStep.cs): The thread-safety implementation is correct, but additional comments would help future maintainers understand why the cached field doesn't need volatile
No Blocking Issues
The code is production-ready. The suggestions are defensive improvements rather than bug fixes. The memory model and threading aspects are correctly implemented.
Testing
The PR includes appropriate test coverage for the new functionality, including tests for string constants, and existing tests were updated where the optimization changes behavior (e.g., GeometryTest).
Great work on this optimization! 🚀
Generated by .NET for Apple Platforms PR Reviewer for issue #24888 · ● 1.4M
| il.Append (il.Create (OpCodes.Brtrue, loadCachedFieldInstruction)); | ||
|
|
||
| // Get__{symbolName}_Cached = xamarin_Dlfcn_{symbolName}_Native (); | ||
| il.Append (il.Create (OpCodes.Call, GetOrCreatePInvokeMethod (callingMethod, symbolName))); |
There was a problem hiding this comment.
Consider clarifying thread-safety guarantees in comment
The current comment (lines 149-153) explains why the initialized field needs volatile semantics, which is correct. However, it might be helpful to also explain why the cached field write/read don't need to be volatile:
// The initialized field must use volatile semantics to prevent ARM64 store
// reordering: without it, another thread could see Initialized=true before
// the Cached value is written, and return a stale IntPtr.Zero.
// Note that we don't care if we call the P/Invoke more than once, that's
// a safe operation, we just don't want to call it every time.
//
// The cached field doesn't need volatile because:
// - The volatile write to Initialized (line 165) provides a release fence,
// ensuring all prior stores (including this one) are visible.
// - The volatile read of Initialized (line 155) provides an acquire fence,
// ensuring this thread sees all stores before the release.This would make the correctness of the double-checked locking pattern more explicit for future maintainers.
…cn namespace The InlineDlfcnMethodsStep now checks for [Field] attributes on the calling method (or its property), and if the attribute has a second string argument (the library name), uses that as the namespace for the Dlfcn type instead of the calling type's namespace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ [PR Build #9a4cb3e] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #9a4cb3e] 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) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [PR Build #9a4cb3e] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #9a4cb3e] 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 |
|
/review |
|
/review |
|
✅ .NET for Apple Platforms PR Reviewer completed successfully! Code review complete - PR already merged. High-quality implementation with comprehensive testing and documentation. No critical issues found, only minor suggestions for improvement. |
|
✅ .NET for Apple Platforms PR Reviewer completed successfully! Code review complete - PR #24888 is already merged with all CI checks passing. No blocking issues found. The implementation follows repository patterns for linker steps, MSBuild tasks, and post-trimming processing. The feature is well-documented and tested. |
Add a new custom linker step that replaces runtime
Dlfcn.dlsymcalls withdirect native symbol lookups via P/Invokes to
__Internal. This improvesstartup performance and app size, and ensures the native linker can see symbol
references that would otherwise only be resolved at runtime.
Two modes are supported, controlled by the
InlineDlfcnMethodsMSBuildproperty:
strict: inlines all calls toObjCRuntime.DlfcnAPIs.compatibility: only inlines calls that reference symbols from[Field]attributes.
Post-trimming MSBuild targets ensure native code is only generated for symbols
that survive trimming (ILTrim or NativeAOT):
_CollectPostILTrimInformation: scans trimmed assemblies for survivinginlined dlfcn P/Invokes, with per-assembly caching for incremental builds.
_CollectPostNativeAOTTrimInformation: reads unresolved symbols from theNativeAOT object file or static library and filters for inlined dlfcn symbols.
_PostTrimmingProcessing: generates native C code for surviving symbols andadds it to the native link inputs.
Extends
MachO.cswith LC_SYMTAB parsing to read unresolved symbols from bothMach-O object files and static libraries.
Contributes towards #17693.