Initial work on closing GC holes in bindings #22345

rolfbjarne · 2025-03-10T08:31:09Z

Generate GC.KeepAlive where necessary in bgen
Add analyzer to flag missing GC.KeepAlive calls
Update manual bindings to fix errors from the analyzer

Contributes to #10146

Original PR #22337 from @filipnavara.

- Add analyzer to flag missing GC.KeepAlive calls - Update manual bindings to fix errors from the analyzer

dalexsoto

Thanks a lot for your contribution as always, is there any chance you can provide some tests for this?

filipnavara · 2025-03-10T22:56:11Z

Thanks a lot for your contribution as always, is there any chance you can provide some tests for this?

I will submit a test for the analyzer soon (along with few more fixes). There's not much else that can be tested easily. I run a part of the tests under CoreCLR with DOTNET_GCStress=2 but that's more of a smoke test than systematic testing.

Improve NativeObjectHandleAnalyzer detection of INativeObject, handle generic parameters

…22355)

…e-gchole

filipnavara · 2025-03-13T21:59:47Z

I'll fix the analyzer tests tomorrow. I made the dumb mistake of using Class in the test cases (chosen at random) and then making an exclusion specifically for it...

…hrowOnNull (#22395) Replace usages of Runtime.ThrowOnNull with GetNonNullHandle extension method Update analyzer tests

vs-mobiletools-engineering-service2 · 2025-03-14T15:00:29Z

✅ [CI Build #e325b09] Build passed (Build packages) ✅

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:01:52Z

✅ [PR Build #e325b09] Build passed (Detect API changes) ✅

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:09:51Z

✅ [CI Build #e325b09] Build passed (Build macOS tests) ✅

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:23:34Z

💻 [CI Build #e325b09] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:24:16Z

💻 [CI Build #e325b09] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:24:25Z

💻 [CI Build #e325b09] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:27:59Z

✅ API diff for current PR / commit

.NET ( No breaking changes )

iOS: vsdrops gist ( No breaking changes )
tvOS: vsdrops gist ( No breaking changes )
MacCatalyst: vsdrops gist ( No breaking changes )
macOS: vsdrops gist ( No breaking changes )

❗ API diff vs stable (Breaking changes)

.NET ( ❗ Breaking changes ❗ )

iOS: vsdrops gist ( ❗ Breaking changes ❗ )
tvOS: vsdrops gist ( ❗ Breaking changes ❗ )
MacCatalyst: vsdrops gist ( ❗ Breaking changes ❗ )
macOS: vsdrops gist ( ❗ Breaking changes ❗ )

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) Unable to create gist: Response status code does not indicate success: 422 (Unprocessable Entity). (raw diff) - Please review changes)

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T15:43:13Z

💻 [CI Build #e325b09] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

vs-mobiletools-engineering-service2 · 2025-03-14T16:31:43Z

🚀 [CI Build #e325b09] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 115 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 2 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 5 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 4 tests passed. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 44 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 8 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ windows: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

src/rgen/Microsoft.Macios.Bindings.Analyzer/NativeObjectHandleAnalyzer.cs

rolfbjarne · 2025-03-14T14:44:15Z

src/bgen/Generator.cs

@@ -1713,7 +1713,7 @@ void GenerateTrampolinesForQueue (TrampolineInfo [] queue)
 			string cast_a = "", cast_b = "";


This code doesn't look right from a first glance, so why wouldn't it be flagged by the analyzer?

macios/src/bgen/Generator.cs

Line 590 in ad60117

returnformat = "return NSArray.FromNSObjects({0}).Handle;";

I'm afraid the analyzer is not run on the bgen generated code, only on the manual bindings. We should probably look into doing that. Perhaps @mandel-macaque has an idea where should that be wired?

Let's add it in a diff PR, we can merge it with this and then see what complains. It looks like we would need to fix both bgen and the new rgen implementation.

I agree we should tackle that in a follow up PR. The PR is big as it is and fixing the manual bindings is likely the harder part.

rolfbjarne

All looks good to me, we can analyze the code from bgen in a separate PR.

✅

dalexsoto

LGTM 👍

vs-mobiletools-engineering-service2 · 2025-03-17T15:57:59Z

💻 [CI Build #e325b09] Tests on macOS arm64 - Mac Sequoia (15) passed 💻

✅ All tests on macOS arm64 - Mac Sequoia (15) passed.

Pipeline on Agent
Hash: e325b094b881cc37a4d07d33142c12863a0ff63f [PR build]

mandel-macaque

Lets land it and then update our build to include analyzer for the generated code and fix any issues in bgen/rgen.

rolfbjarne · 2025-03-17T16:33:51Z

@filipnavara thanks a lot for your great work!

filipnavara and others added 5 commits March 8, 2025 01:09

- Generate GC.KeepAlive where necessary in bgen

e6deaa5

- Add analyzer to flag missing GC.KeepAlive calls - Update manual bindings to fix errors from the analyzer

Auto-format source code

c0205a4

Update FIXME comments

2d21451

Make CALayerDelegate GC safe

60dc257

Fix NSUrlCredential constructor

b606d11

rolfbjarne requested review from dalexsoto and mandel-macaque as code owners March 10, 2025 08:31

rolfbjarne mentioned this pull request Mar 10, 2025

Initial work on closing GC holes in bindings #22337

Closed

This comment has been minimized.

Sign in to view

dalexsoto reviewed Mar 10, 2025

View reviewed changes

filipnavara and others added 5 commits March 11, 2025 00:01

Add test for NativeObjectHandleAnalyzer

9d44977

Improve NativeObjectHandleAnalyzer detection of INativeObject, handle generic parameters

Fix more cases found by the updated analyzer

5982b06

Auto-format source code

1601b3e

[dev/filipnavara/handle-gchole] Add analyzer tests, fix more usages (#…

4560da2

…22355)

Merge remote-tracking branch 'origin/main' into dev/filipnavara/handl…

86e7303

…e-gchole

This comment has been minimized.

Sign in to view

[dev/filipnavara/handle-gchole] Fix analyzer tests, replace Runtime.T…

e325b09

…hrowOnNull (#22395) Replace usages of Runtime.ThrowOnNull with GetNonNullHandle extension method Update analyzer tests

This comment has been minimized.

Sign in to view

rolfbjarne commented Mar 17, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rolfbjarne commented Mar 17, 2025

View reviewed changes

dalexsoto approved these changes Mar 17, 2025

View reviewed changes

mandel-macaque approved these changes Mar 17, 2025

View reviewed changes

rolfbjarne merged commit 8d78327 into main Mar 17, 2025
47 checks passed

rolfbjarne deleted the dev/filipnavara/handle-gchole branch March 17, 2025 16:33

		@@ -1713,7 +1713,7 @@ void GenerateTrampolinesForQueue (TrampolineInfo [] queue)
		string cast_a = "", cast_b = "";

Initial work on closing GC holes in bindings #22345

Initial work on closing GC holes in bindings #22345

Uh oh!

Conversation

rolfbjarne commented Mar 10, 2025

Uh oh!

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.

dalexsoto left a comment

Choose a reason for hiding this comment

Uh oh!

filipnavara commented Mar 10, 2025

Uh oh!

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.

filipnavara commented Mar 13, 2025

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

✅ [CI Build #e325b09] Build passed (Build packages) ✅

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

✅ [PR Build #e325b09] Build passed (Detect API changes) ✅

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

✅ [CI Build #e325b09] Build passed (Build macOS tests) ✅

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

💻 [CI Build #e325b09] Tests on macOS M1 - Mac Monterey (12) passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

💻 [CI Build #e325b09] Tests on macOS X64 - Mac Sonoma (14) passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

💻 [CI Build #e325b09] Tests on macOS M1 - Mac Ventura (13) passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

✅ API diff for current PR / commit

❗ API diff vs stable (Breaking changes)

ℹ️ Generator diff

Uh oh!

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Mar 14, 2025

💻 [CI Build #e325b09] Windows Integration Tests passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Mar 14, 2025

🚀 [CI Build #e325b09] Test results 🚀

Test results

Tests counts

Uh oh!

Uh oh!

rolfbjarne Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

filipnavara Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandel-macaque Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

filipnavara Mar 17, 2025 •

edited

Loading

filipnavara Mar 17, 2025 •

edited

Loading