[HandleSafety] Recognize GC.KeepAlive with equivalent (non-identical) loads. - #26366
[HandleSafety] Recognize GC.KeepAlive with equivalent (non-identical) loads.#26366rolfbjarne wants to merge 2 commits into
Conversation
… loads.
Some members were already safe - they fetch a handle from a field and then call
`GC.KeepAlive` on that same field, e.g.:
DoSomethingWith (this.field.GetHandle ());
GC.KeepAlive (this.field);
The test failed to recognize this because `field` is a field, so the compiler
emits two separate `ldfld field` instructions, and `IsKeptAlive` only matched
the handle-fetch load instruction by reference identity (which worked for the
`dup` pattern the compiler emits for local variables, but not for fields).
Improve `IsKeptAlive` to also match a `GC.KeepAlive` argument that's loaded by a
different but provably equivalent instruction (same field of the same object,
same argument, same local, etc.). This is sound: it never claims two loads are
equivalent unless it's certain.
This resolves the following known failures:
* AddressBook.ABMultiValueEntry`1.get_Identifier
* AddressBook.ABMultiValueEntry`1.get_Label
* AddressBook.ABMultiValueEntry`1.get_Value
* AddressBook.ABMultiValueEntry`1.set_Label
* AddressBook.ABMultiValueEntry`1.set_Value
* Foundation.NSFastEnumerator`1.Fetch
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd86cf03-297a-43e8-ae1f-3c0b22bb67c4
There was a problem hiding this comment.
Pull request overview
Improves the Cecil-based handle-safety analysis used by the test suite to recognize GC.KeepAlive patterns where the kept-alive object is reloaded via a different but equivalent IL load instruction (notably repeated ldfld for fields). This makes previously “known failing” members pass the HandleSafety check.
Changes:
- Extend
IsKeptAliveto matchGC.KeepAlivearguments via a new “equivalent load” analysis (not just instruction identity). - Add equivalence logic for common load opcodes and for
ldfldwhen both loads target the same field on equivalent source objects. - Remove several entries from
HandleSafety.KnownFailures.csthat should now be recognized as safe.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/cecil-tests/HandleSafety.cs | Enhances GC.KeepAlive detection by recognizing provably equivalent IL loads (incl. repeated ldfld patterns). |
| tests/cecil-tests/HandleSafety.KnownFailures.cs | Drops known-failure entries that should now pass with the improved equivalence matching. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
✅ 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 |
🔥 [CI Build #1573996] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 44 tests failed, 159 tests passed. Failures❌ dotnettests tests (iOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (macOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (tvOS)1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS)19 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (tvOS)19 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ msbuild tests1 tests failed, 1 tests passed.Failed tests
Html Report (VSDrops) Download ❌ windows tests1 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ Tests on macOS Tahoe (26) tests1 tests failed, 4 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: 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 |
Some members were already safe - they fetch a handle from a field and then call
GC.KeepAliveon that same field, e.g.:The test failed to recognize this because
fieldis a field, so the compileremits two separate
ldfld fieldinstructions, andIsKeptAliveonly matchedthe handle-fetch load instruction by reference identity (which worked for the
duppattern the compiler emits for local variables, but not for fields).Improve
IsKeptAliveto also match aGC.KeepAliveargument that's loaded by adifferent but provably equivalent instruction (same field of the same object,
same argument, same local, etc.). This is sound: it never claims two loads are
equivalent unless it's certain.
This resolves the following known failures:
Copilot-Session: dd86cf03-297a-43e8-ae1f-3c0b22bb67c4