Skip to content

Commit

Permalink
[Xamarin..Android.Tools.Aidl] inout+inout array Parameters (#5726)
Browse files Browse the repository at this point in the history 
Fixes: #4717
Fixes: #5079

Context: https://cs.android.com/android/platform/superproject/+/af1f0005723416a36e025fded7eccb2fd8c777de:system/tools/aidl/generate_java_binder.cpp;l=358
Context: https://cs.android.com/android/platform/superproject/+/af1f0005723416a36e025fded7eccb2fd8c777de:system/tools/aidl/generate_java_binder.cpp;l=486
Context: https://cs.android.com/android/platform/superproject/+/af1f0005723416a36e025fded7eccb2fd8c777de:system/tools/aidl/generate_java_binder.cpp;l=646

`Xamarin.Android.Tools.Aidl.CSharpCodeGenerator` did not properly
support `inout` or `out` parameter marshaling of array types.

For `inout` arrays:

	// AIDL
	interface Test {
	    void inoutArray (inout byte [] args);
	}

the emitted `OnTransact()` method needs to write the "output" value
to the `reply` parameter, *not* the `data` parameter:

	// generated C#
	public interface ITest : Android.OS.IInterface {
	    void InoutArray (byte[] args)
	}
	abstract partial class ITestStub : Binder, ITest {
	    protected override bool OnTransact (int code, Parcel data, Parcel reply, int flags)
	    {
	        switch (code) {
	        case TransactionInoutArray: {
	            data.EnforceInterface (descriptor);
	            byte [] arg0 = default (byte []);
	            arg0 = data.CreateStringArray ();
	            this.InoutArray (arg0);
	            reply.WriteNoException ();
	            data.WriteByteArray (arg0);         // SHOULD USE `reply`, not `data`!
	            return true;
	            }
	        }
	    }
	}

Similar-yet-more-so, with `out` array parameters:

	// AIDL
	interface Test {
	    void outArray (out byte [] args);
	}

the `OnTransact()` method *also* needs to support `args` being `null`:

	// generated C#
	public interface ITest : global::Android.OS.IInterface {
	    void OutArray (byte[] args)
	}
	abstract partial class ITestStub : Binder, ITest {
	    protected override bool OnTransact (int code, Parcel data, Parcel reply, int flags)
	    {
	        switch (code) {
	        case TransactionOutArray: {
	            data.EnforceInterface (descriptor);
	            byte [] arg0 = default (byte []);
	            int arg0_length = data.ReadInt();   // NEEDED
	            if (arg0_length < 0) {              // NEEDED
	                arg0 = null;                    // NEEDED
	            }                                   // NEEDED
	            else {                              // NEEDED
	                arg0 = new byte [arg0_length];	// NEEDED
	            }                                   // NEEDED
	            this.OutArray (arg0);
	            reply.WriteNoException ();
	            reply.WriteByteArray (arg0);        // PREVIOUSLY used data, not reply!
	            return true;
	            }
	        }
	    }
	}

*Plus* the `Proxy` implementation needs to support `null`:

	partial class ITestStub {
	    public class Proxy : Java.Lang.Object, ITest {
	        public void OutArray (byte[] args)
	        {
	            var __data  = Parcel.Obtain ();
	            var __reply = Parcel.Obtain ();
	            try {
	                __data.WriteInterfaceToken (descriptor);
	                if (args == null) {                 // NEEDED
	                    __data.WriteInt (-1);           // NEEDED
	                } else {                            // NEEDED
	                    __data.WriteInt(args.Length);   // NEEDED
	                }                                   // NEEDED
	                remote.Transact (ITestStub.TransactionOutArray, __data, __reply, 0);
	                …
	        }
	    }
	}

This is comparable to the Java code the Android Studio produces.
  • Loading branch information
@mfranke-moba
mfranke-moba authored Mar 26, 2021
1 parent cc3d123 commit 9cfa5f3
Show file tree
Hide file tree
Showing 7 changed files with 295 additions and 758 deletions.
35 changes: 30 additions & 5 deletions src/Xamarin.Android.Tools.Aidl/CSharpCodeGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,20 @@ protected override bool OnTransact (int code, global::Android.OS.Parcel data, gl
w.WriteLine ("\t\t\t\t{0} {1} = default ({0});", ToOutputTypeName (name_cache.ToCSharp (a.Type)), "arg" + i);
if (a.Modifier == null || a.Modifier.Contains ("in"))
w.WriteLine ("\t\t\t\t{0}", GetCreateStatements (a.Type, "data", "arg" + i));
else if (a.Modifier != null && a.Modifier.Contains ("out")) {
if (a.Type.ArrayDimension > 0) {
w.WriteLine (@" int {0}_length = data.ReadInt();
if ({0}_length < 0) {{
{0} = null;
}}
else {{
{0} = new {1}[{0}_length];
}}", "arg" + i, ToOutputTypeName (name_cache.ToCSharp (a.Type)).Replace ("[]", ""));
}
else {
w.WriteLine ("\t\t\t\t{0} = new {1}();", "arg" + i, ToOutputTypeName (name_cache.ToCSharp (a.Type)));
}
}
}
string args = String.Join (", ", (from i in Enumerable.Range (0, method.Arguments.Length) select "arg" + i).ToArray ());
if (isVoidReturn)
Expand All @@ -289,8 +303,8 @@ protected override bool OnTransact (int code, global::Android.OS.Parcel data, gl
w.WriteLine ("\t\t\t\t{0}", GetWriteStatements (method.ReturnType, "reply", "result", "global::Android.OS.ParcelableWriteFlags.ReturnValue"));
for (int i = 0; method.Arguments != null && i < method.Arguments.Length; i++) {
var a = method.Arguments [i];
if (a.Modifier == null || a.Modifier.Contains ("out"))
w.WriteLine ("\t\t\t\t{0}", GetWriteStatements (a.Type, "data", "arg" + i, "global::Android.OS.ParcelableWriteFlags.None"));
if (a.Modifier != null && a.Modifier.Contains ("out"))
w.WriteLine ("\t\t\t\t{0}", GetWriteStatements (a.Type, "reply", "arg" + i, "global::Android.OS.ParcelableWriteFlags.ReturnValue"));
}
w.WriteLine ("\t\t\t\treturn true;");
w.WriteLine ("\t\t\t\t}");
Expand Down Expand Up @@ -331,13 +345,16 @@ public string GetInterfaceDescriptor ()
if (!isOneWay)
w.WriteLine ("\t\t\t\tglobal::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();");
if (hasReturn)
w.WriteLine ("{0} __result = default ({0});", ToOutputTypeName (name_cache.ToCSharp (method.ReturnType)));
w.WriteLine ("\t\t\t\t{0} __result = default ({0});", ToOutputTypeName (name_cache.ToCSharp (method.ReturnType)));
w.WriteLine (@"
try {
__data.WriteInterfaceToken (descriptor);");
foreach (var arg in method.Arguments)
foreach (var arg in method.Arguments) {
if (arg.Modifier == null || arg.Modifier.Contains ("in"))
w.WriteLine ("\t\t\t\t\t" + GetWriteStatements (arg.Type, "__data", SafeCSharpName (arg.Name), "global::Android.OS.ParcelableWriteFlags.None"));
else if (arg.Modifier != null && arg.Modifier.Contains ("out") && arg.Type.ArrayDimension > 0)
w.WriteLine ("\t\t\t\t\t" + GetWriteOutStatements (arg.Type, "__data", SafeCSharpName (arg.Name)));
}
w.WriteLine ("\t\t\t\t\tremote.Transact ({1}Stub.Transaction{0}, __data, {2}, 0);",
method.Name,
type.Name,
Expand Down Expand Up @@ -613,7 +630,15 @@ string GetWriteStatements (TypeName type, string parcel, string arg, string parc
return String.Format ("{1}.WriteStrongBinder (((({0} != null)) ? ({0}.AsBinder ()) : (null)));", arg, parcel);
}
}


string GetWriteOutStatements (TypeName type, string parcel, string arg)
{
if (type.ArrayDimension > 0) {
return "if (" + arg + " == null) { " + parcel + ".WriteInt(-1); } else { " + parcel + ".WriteInt(" + arg + ".Length); }";
} else
return "";
}

// FIXME: should this be used?
string GetCreatorName (TypeName type)
{
Expand Down
21 changes: 6 additions & 15 deletions tests/Xamarin.Android.Tools.Aidl-Tests/TestData/FaceService.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
long arg0 = default (long);
arg0 = data.ReadLong ();
this.Authenticate (arg0);
data.WriteLong (arg0);
return true;
}

Expand All @@ -132,7 +131,6 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
int [] arg2 = default (int []);
arg2 = data.CreateIntArray ();
this.Enroll (arg0, arg1, arg2);
data.WriteInt (arg1);
return true;
}

Expand All @@ -151,7 +149,6 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
var result = this.GenerateChallenge (arg0);
reply.WriteNoException ();
reply.WriteLong (result);
data.WriteInt (arg0);
return true;
}

Expand All @@ -172,8 +169,6 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
var result = this.GetFeature (arg0, arg1);
reply.WriteNoException ();
reply.WriteInt (result ? 1 : 0);
data.WriteInt (arg0);
data.WriteInt (arg1);
return true;
}

Expand All @@ -190,7 +185,6 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
int arg0 = default (int);
arg0 = data.ReadInt ();
this.Remove (arg0);
data.WriteInt (arg0);
return true;
}

Expand Down Expand Up @@ -229,9 +223,6 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
int arg3 = default (int);
arg3 = data.ReadInt ();
this.SetFeature (arg0, arg1, arg2, arg3);
data.WriteInt (arg0);
data.WriteInt (arg1 ? 1 : 0);
data.WriteInt (arg3);
return true;
}

Expand Down Expand Up @@ -321,7 +312,7 @@ namespace Com.Android.Internal.Util.Custom.Faceunlock
global::Android.OS.Parcel __data = global::Android.OS.Parcel.Obtain ();

global::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();
int __result = default (int);
int __result = default (int);

try {
__data.WriteInterfaceToken (descriptor);
Expand All @@ -343,7 +334,7 @@ int __result = default (int);
global::Android.OS.Parcel __data = global::Android.OS.Parcel.Obtain ();

global::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();
long __result = default (long);
long __result = default (long);

try {
__data.WriteInterfaceToken (descriptor);
Expand All @@ -366,7 +357,7 @@ long __result = default (long);
global::Android.OS.Parcel __data = global::Android.OS.Parcel.Obtain ();

global::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();
int __result = default (int);
int __result = default (int);

try {
__data.WriteInterfaceToken (descriptor);
Expand All @@ -388,7 +379,7 @@ int __result = default (int);
global::Android.OS.Parcel __data = global::Android.OS.Parcel.Obtain ();

global::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();
bool __result = default (bool);
bool __result = default (bool);

try {
__data.WriteInterfaceToken (descriptor);
Expand All @@ -412,7 +403,7 @@ bool __result = default (bool);
global::Android.OS.Parcel __data = global::Android.OS.Parcel.Obtain ();

global::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();
int __result = default (int);
int __result = default (int);

try {
__data.WriteInterfaceToken (descriptor);
Expand Down Expand Up @@ -472,7 +463,7 @@ int __result = default (int);
global::Android.OS.Parcel __data = global::Android.OS.Parcel.Obtain ();

global::Android.OS.Parcel __reply = global::Android.OS.Parcel.Obtain ();
int __result = default (int);
int __result = default (int);

try {
__data.WriteInterfaceToken (descriptor);
Expand Down
Loading

0 comments on commit 9cfa5f3

Please sign in to comment.