xamarin-macios icon indicating copy to clipboard operation
xamarin-macios copied to clipboard

Published 20 hours ago • verified publisher icon xamarin

[Generator] Allow the generator to write calls that will take arrays of structures.

Open mandel-macaque opened this issue 5 years ago • 56 comments

Initially, the generator will try to use the following code when it sees a parameter that is an array of structures:

Binding code

[Export ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:")]
void ConvertSparsePixelRegions (MTLRegion [] pixelRegions, MTLRegion [] tileRegions, MTLSparseTextureRegionAlignmentMode mode, nuint numRegions);

Generated code

var nsa_pixelRegions = NSArray.FromNSObjects (pixelRegions);
var nsa_tileRegions = NSArray.FromNSObjects (tileRegions);

With the change, the generator will write something like:

if (pixelRegions == null)
	throw new ArgumentNullException ("pixelRegions");
if (tileRegions == null)
	throw new ArgumentNullException ("tileRegions");

var nsa_pixelRegionsHandle = GCHandle.Alloc (pixelRegions, GCHandleType.Pinned);
IntPtr nsa_pixelRegions = nsa_pixelRegionsHandle.AddrOfPinnedObject ();
var nsa_tileRegionsHandle = GCHandle.Alloc (tileRegions, GCHandleType.Pinned);
IntPtr nsa_tileRegions = nsa_tileRegionsHandle.AddrOfPinnedObject ();

try {
	if (IsDirectBinding) {
		global::api0.Messaging.void_objc_msgSend_IntPtr_IntPtr_int_nuint (this.Handle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), nsa_pixelRegions, nsa_tileRegions, (int)mode, numRegions);
	} else {
		global::api0.Messaging.void_objc_msgSendSuper_IntPtr_IntPtr_int_nuint (this.SuperHandle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), nsa_pixelRegions, nsa_tileRegions, (int)mode, numRegions);
	}
} finally {
	nsa_pixelRegionsHandle.Free ();
	nsa_tileRegionsHandle.Free ();

}

This allows to stop using manual bindings for those cases in which we are taking a struct[] which there are a few.

Fixes: https://github.com/xamarin/xamarin-macios/issues/6994

mandel-macaque avatar Dec 03 '19 22:12 mandel-macaque

The generated code would be simpler with the array type in the P/Invoke declaration:

[DllImport (...)]
static extern void_objc_msgSend_MTLRegionArray_MTLRegionArray_int_nuint (IntPtr handle, IntPtr sel, MTLRegion[] pixelRegions, MTLRegion[] tileRegions, int mode, nuint numRegions);

and then the generated code would become:

if (pixelRegions == null)
	throw new ArgumentNullException ("pixelRegions");
if (tileRegions == null)
	throw new ArgumentNullException ("tileRegions");

if (IsDirectBinding) {
	global::api0.Messaging.void_objc_msgSend_IntPtr_IntPtr_int_nuint (this.Handle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), pixelRegions, tileRegions, (int)mode, numRegions);
} else {
	global::api0.Messaging.void_objc_msgSendSuper_IntPtr_IntPtr_int_nuint (this.SuperHandle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), pixelRegions, tileRegions, (int)mode, numRegions);
}

rolfbjarne avatar Dec 03 '19 22:12 rolfbjarne

I was about to suggest fixed instead of allocating (and free'ing) two GGHandle but @rolfbjarne suggestion is even simpler :)

spouliot avatar Dec 03 '19 22:12 spouliot

~Might be worth annotating arrays with [In] - where const is in native, the runtime will not marshal back any changes done to the array.~ NEVERMIND, that's just COM interop

Therzok avatar Dec 04 '19 00:12 Therzok

I wonder if you can use MemoryMarshal APIs to safe the headache of pinning, etc.

Therzok avatar Dec 04 '19 00:12 Therzok

Build failure ✅ Build succeeded ✅ API Diff (from stable) ✅ API Diff (from PR only) (no change) ✅ Generator Diff (no change) 🔥 Test run failed 🔥

Test results

1 tests failed, 87 tests passed.

Failed tests

  • monotouch-test/watchOS 32-bits - simulator/Debug: Crashed

monojenkins avatar Dec 04 '19 01:12 monojenkins

Agreed, I think Rolf suggestion is simpler

dalexsoto avatar Dec 04 '19 15:12 dalexsoto

Build failure ✅ Provisioning succeeded ✅ Build succeeded ✅ API Diff (from stable) ✅ API Diff (from PR only) (no change) ✅ Generator Diff (no change) 🔥 Test run failed 🔥

Test results

1 tests failed, 89 tests passed.

Failed tests

  • monotouch-test/watchOS 32-bits - simulator/Debug: Failed Known issue: HE0038)

monojenkins avatar Jun 12 '20 16:06 monojenkins

:warning: Your code has been reformatted. :warning:

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).

github-actions[bot] avatar Nov 07 '23 23:11 github-actions[bot]

:fire: [PR Build] Build failed :fire:

Build failed for the job 'Build packages'

Pipeline on Agent Hash: e27dd3f382755387efbbb632fa5c546a8a46511a [PR build]

vs-mobiletools-engineering-service2 avatar Nov 07 '23 23:11 vs-mobiletools-engineering-service2

:fire: [PR Build] Build failed :fire:

Build failed for the job 'Build macOS tests'

Pipeline on Agent Hash: e27dd3f382755387efbbb632fa5c546a8a46511a [PR build]

vs-mobiletools-engineering-service2 avatar Nov 07 '23 23:11 vs-mobiletools-engineering-service2

:fire: [PR Build] Build failed :fire:

Build failed for the job 'Detect API changes'

Pipeline on Agent Hash: e27dd3f382755387efbbb632fa5c546a8a46511a [PR build]

vs-mobiletools-engineering-service2 avatar Nov 07 '23 23:11 vs-mobiletools-engineering-service2

:fire: Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire

Pipeline on Agent Hash: e27dd3f382755387efbbb632fa5c546a8a46511a [PR build]

vs-mobiletools-engineering-service2 avatar Nov 07 '23 23:11 vs-mobiletools-engineering-service2

:warning: Your code has been reformatted. :warning:

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).

github-actions[bot] avatar Nov 08 '23 00:11 github-actions[bot]

:computer: [CI Build] Windows Integration Tests passed :computer:

:white_check_mark: All Windows Integration Tests passed.

Pipeline on Agent Hash: 973285831ce1f7ef7a29f6dc2cab36c581cab474 [PR build]

vs-mobiletools-engineering-service2 avatar Nov 08 '23 01:11 vs-mobiletools-engineering-service2

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

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

Pipeline on Agent Hash: [PR build]

vs-mobiletools-engineering-service2 avatar Nov 08 '23 01:11 vs-mobiletools-engineering-service2

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

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

Pipeline on Agent Hash: [PR build]

vs-mobiletools-engineering-service2 avatar Nov 08 '23 01:11 vs-mobiletools-engineering-service2

:books: [PR Build] Artifacts :books:

Packages generated

View packages

Pipeline on Agent Hash: [PR build]

vs-mobiletools-engineering-service2 avatar Nov 08 '23 01:11 vs-mobiletools-engineering-service2

:white_check_mark: 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)

:white_check_mark: API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

:information_source: Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent Hash: 973285831ce1f7ef7a29f6dc2cab36c581cab474 [PR build]

vs-mobiletools-engineering-service2 avatar Nov 08 '23 01:11 vs-mobiletools-engineering-service2

:rocket: [CI Build] Test results :rocket:

Test results

:white_check_mark: All tests passed on VSTS: simulator tests.

:tada: All 235 tests passed :tada:

Tests counts

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

Pipeline on Agent Hash: 973285831ce1f7ef7a29f6dc2cab36c581cab474 [PR build]

vs-mobiletools-engineering-service2 avatar Nov 08 '23 04:11 vs-mobiletools-engineering-service2

This is an old PR :) Things have changed a bit since then, and most importantly the new detail now is that we only want to create blittable P/Invokes, and arrays aren't blittable. We'll have to do something like Sebastien suggested instead:

[DllImport (...)]
static unsafe extern void_objc_msgSend_MTLRegionArray_MTLRegionArray_int_nuint (IntPtr handle, IntPtr sel, MTLRegion* pixelRegions, MTLRegion* tileRegions, int mode, nuint numRegions);

if (pixelRegions is null)
	throw new ArgumentNullException (nameof (pixelRegions));
if (tileRegions is null)
	throw new ArgumentNullException (nameof (tileRegions));

fixed (MTLRegion* pixelRegionsPtr = &pixelRegions[0]) {
    fixed (MTLRegion* tileRegionsPtr = &tileRegions[0]) {
        if (IsDirectBinding) {
        	global::api0.Messaging.void_objc_msgSend_IntPtr_IntPtr_int_nuint (this.Handle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), pixelRegionsPtr, tileRegionsPtr, (int)mode, numRegions);
        } else {
        	global::api0.Messaging.void_objc_msgSendSuper_IntPtr_IntPtr_int_nuint (this.SuperHandle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), pixelRegionsPtr, tileRegionsPtr, (int)mode, numRegions);
        }
    }
}

and then we'll also probably need to check the length of the arrays to ensure length>0 before getting the address of the first item.

rolfbjarne avatar Nov 08 '23 07:11 rolfbjarne

:warning: Your code has been reformatted. :warning:

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).

github-actions[bot] avatar Nov 09 '23 17:11 github-actions[bot]

The approach I took to get the indentations correct is a little rough and hoping the upcoming refactoring work will introduce a better overall solution :(

Here's what the generator produces now with this PR:

public unsafe extern static void void_objc_msgSend_MTLRegionArray_MTLRegionArray_int_UIntPtr (IntPtr receiver, IntPtr selector, MTLRegion* arg1, MTLRegion* arg2, int arg3, UIntPtr arg4);

public virtual void ConvertSparsePixelRegions (MTLRegion[] pixelRegions, MTLRegion[] tileRegions, MTLSparseTextureRegionAlignmentMode mode, nuint numRegions)
{
	if (pixelRegions is null)
		ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (pixelRegions));
	if (tileRegions is null)
		ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (tileRegions));
	if (pixelRegions.Length > 0) {
		fixed (MTLRegion* pixelRegionsPtr = &pixelRegions[0]) {
			if (tileRegions.Length > 0) {
				fixed (MTLRegion* tileRegionsPtr = &tileRegions[0]) {
					if (IsDirectBinding) {
						global::api0.Messaging.void_objc_msgSend_MTLRegionArray_MTLRegionArray_int_UIntPtr (this.Handle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), pixelRegionsPtr, tileRegionsPtr, (int)mode, (UIntPtr) numRegions);
					} else {
						global::api0.Messaging.void_objc_msgSendSuper_MTLRegionArray_MTLRegionArray_int_UIntPtr (this.SuperHandle, Selector.GetHandle ("convertSparsePixelRegions:toTileRegions:alignmentMode:numRegions:"), pixelRegionsPtr, tileRegionsPtr, (int)mode, (UIntPtr) numRegions);
					}
				}
			}
		}
	}
}

A discrepancy I noticed is that UIntPtr is being used instead of nint and not sure which value we want.

dustin-wojciechowski avatar Nov 09 '23 18:11 dustin-wojciechowski

:computer: [CI Build] Windows Integration Tests passed :computer:

:white_check_mark: All Windows Integration Tests passed.

Pipeline on Agent Hash: 45a641597714c77d0bc7d36606e70d2fc0972d42 [PR build]

vs-mobiletools-engineering-service2 avatar Nov 09 '23 19:11 vs-mobiletools-engineering-service2

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

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

Pipeline on Agent Hash: [PR build]

vs-mobiletools-engineering-service2 avatar Nov 09 '23 19:11 vs-mobiletools-engineering-service2

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

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

Pipeline on Agent Hash: [PR build]

vs-mobiletools-engineering-service2 avatar Nov 09 '23 19:11 vs-mobiletools-engineering-service2

:books: [PR Build] Artifacts :books:

Packages generated

View packages

Pipeline on Agent Hash: [PR build]

vs-mobiletools-engineering-service2 avatar Nov 09 '23 19:11 vs-mobiletools-engineering-service2

:white_check_mark: 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)

:white_check_mark: API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

:information_source: Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent Hash: 45a641597714c77d0bc7d36606e70d2fc0972d42 [PR build]

vs-mobiletools-engineering-service2 avatar Nov 09 '23 19:11 vs-mobiletools-engineering-service2

:rocket: [CI Build] Test results :rocket:

Test results

:white_check_mark: All tests passed on VSTS: simulator tests.

:tada: All 235 tests passed :tada:

Tests counts

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

Pipeline on Agent Hash: 45a641597714c77d0bc7d36606e70d2fc0972d42 [PR build]

vs-mobiletools-engineering-service2 avatar Nov 09 '23 21:11 vs-mobiletools-engineering-service2

@dustin-wojciechowski I agree on waiting on cleaning the generator to deal with the indentation better, I have added a new test case in which we return an IntPtr[] and it fails as you can see in the generated code: https://gist.github.com/mandel-macaque/1937910665e7d5c8693d153157db14fd

@rolfbjarne should we agree that in this branch too?

Ps: I cannot/should not approve my own pr.

mandel-macaque avatar Nov 09 '23 23:11 mandel-macaque

:warning: Your code has been reformatted. :warning:

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).

github-actions[bot] avatar Nov 09 '23 23:11 github-actions[bot]