[dotnet] Integrate class handle rewriting into static registrar process. by stephen-hawley · Pull Request #18456 · dotnet/macios

stephen-hawley · 2023-06-15T20:01:32Z

Integrate class handle rewriting into static registrar.

Since the class handle optimizations are fairly tightly coupled with the static registrar, it made more sense to me for it to happen in the same process.

In this PR, I refactored the code to

use AssemblyDefinition types instead of a directory of assemblies
eliminate the need for intermediate files and just get the map straight from the registrar
remove the ClassRedirector task
integrate the code to wherever the static registrar is used

Things that I'm concerned about that I don't know about:

this code rewrites over the top of the module. I'm not sure how else to handle this and it's probably wrong. Could use some guidance here
getting the flag into the whole shebang

No tests yet - they'll happen in a subsequent PR after the dust settles here

github-actions · 2023-06-15T20:05:45Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

rolfbjarne · 2023-06-16T11:32:33Z

+    
+      external\ObjCNameIndex.cs
+    


Not sure why you need this if it's not being implemented as an MSBuild task anymore?

rolfbjarne · 2023-06-16T11:34:16Z

 				@(_MonoLibrary -> 'MonoLibrary=%(Identity)')
 				MtouchFloat32=$(MtouchFloat32)
 				Optimize=$(_BundlerOptimize)
+				OptimizeClassHandles=$(OptimizeClassHandles)


You already added an optimization option here:

62634bd

can't that one be used instead of adding another one?

rolfbjarne · 2023-06-16T11:36:43Z

+			if (Target.App.OptimizeClassHandles) {
+				var rewriter = new Rewriter (assemblies, typeMap);
+				rewriter.Process ();
+			}
 		}


This chunk of code is duplicated in a few places, would it make sense to move it inside the StaticRegistrar.Generate method so that there's only one copy of it?

That's a good idea - I didn't do it initially because I was worried about exception handling (which this code needs).

…-hawley/xamarin-macios into reroute-class-redirector

rolfbjarne

There's also a test failure in a few tests:

error MM2362: The linker step 'Registrar' failed during processing: Unable to find ObjCRuntime.Runtime/ClassHandles type in

rolfbjarne · 2023-06-19T09:29:21Z

-			if (!string.IsNullOrEmpty (type_map_path)) {
-				var doc = CSToObjCMap.ToXDocument (map_dict);
-				doc.Save (type_map_path);
+			if (rewriteClassHandles) {


You can access the optimization flag like this, that would avoid having to pass another extra parameter around:

Suggested change

if (rewriteClassHandles) {

if (App.Optimizations.RewriteClassHandles == true) {

rolfbjarne · 2023-06-19T09:32:26Z

+			if (rewriteClassHandles) {
+				var rewriter = new Rewriter (map_dict, GetAssemblies ());
+				rewriter.Process ();


IIRC the class redirect is .NET only, and not for legacy Xamarin:

Suggested change

if (rewriteClassHandles) {

var rewriter = new Rewriter (map_dict, GetAssemblies ());

rewriter.Process ();

#if NET

if (rewriteClassHandles) {

var rewriter = new Rewriter (map_dict, GetAssemblies ());

rewriter.Process ();

}

#endif

rolfbjarne · 2023-06-19T09:48:14Z

+				var module = assem.MainModule;
 				PatchClassPtrUsage (classMap, module);
-				module.Write (ToOutputFileName (path));
+				module.Write ();


Don't write out the assembly, the linker will do that later.

However, it's important to tell the linker that the assembly was changed, and that's done by changing the action:

https://github.com/xamarin/xamarin-macios/blob/cb34bfe86cba310293d45004a5799c68e60901bd/tools/dotnet-linker/AppBundleRewriter.cs#L1092-L1094

https://github.com/xamarin/xamarin-macios/blob/cb34bfe86cba310293d45004a5799c68e60901bd/tools/dotnet-linker/AppBundleRewriter.cs#L1103

Note that you should only save the assembly if you actually modified it.

rolfbjarne · 2023-06-21T10:35:50Z

+			} catch (Exception e) {
+				// if this throws, no changes are made to the assemblies
+				// so it's safe to log it on the far side.
+				return e.Message;
+			}


Any reason it's not a failure to fail to apply the class handle optimization? It should be easy enough for customers to turn it off if need be, and just printing a log message if something goes wrong feels like nobody will notice if it ever breaks.

A couple reasons: if this throws an exception, it means that we've done something horribly, horribly wrong. It means that someone removed the ClassHandles types or its InitializeClassHandles method, or NativeHandle or any of a number of things that need to be in place for the rest of the code to work. And while this seems like a "Holy cow, we should stop this immediately!" error - it actually isn't. The rest of the process won't run and no changes will have been made to the assemblies and the user is left with a build without NativeHandle.

rolfbjarne · 2023-06-21T10:45:08Z

 		internal static unsafe InitializationOptions* options;

 #if NET
+		[Preserve (AllMembers = true)]


Could you maybe just add this class + method dynamically in the rewriter? That way it would only be present when needed.

One thing that's missing is a call from Initialize that will run that code. It needs to be there and it's best if it's just empty.

If the method is called from somewhere, then you won't need the Preserve attribute.

vs-mobiletools-engineering-service2 · 2023-06-22T16:30:38Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-06-22T16:32:47Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: 51a9207f9e4ec8bfd3137f89e95c2760c409279a [PR build]

vs-mobiletools-engineering-service2 · 2023-06-22T16:48:07Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

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

.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)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 51a9207f9e4ec8bfd3137f89e95c2760c409279a [PR build]

vs-mobiletools-engineering-service2 · 2023-06-22T16:57:55Z

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

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

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-06-22T16:59:04Z

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-06-22T19:50:35Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 235 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 35 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 51a9207f9e4ec8bfd3137f89e95c2760c409279a [PR build]

…ss. (#18456) Integrate class handle rewriting into static registrar.

Integrate class handle rewriting into static registrar process.

441d4d4

stephen-hawley added not-notes-worthy Ignore for release notes dotnet An issue or pull request related to .NET labels Jun 15, 2023

stephen-hawley requested review from emaf, mauroa and rolfbjarne as code owners June 15, 2023 20:01

Auto-format source code

4a395e2