Skip to content

[Xamarin..Android.Tools.Aidl] Handle out and inout Parameters in CodeGenerator properly #5726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 26, 2021
Merged

[Xamarin..Android.Tools.Aidl] Handle out and inout Parameters in CodeGenerator properly #5726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ec8c09
modified stub and proxy function generation to handle out/inout param…
mfranke-moba Mar 10, 2021
bc5dca0
Merge remote-tracking branch 'origin/main' into out_parameter_handling
jonpryor Mar 25, 2021
b5e0366
[Xamarin.Android.Tools.Aidl-Tests] update expected test output
jonpryor Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions src/Xamarin.Android.Tools.Aidl/CSharpCodeGenerator.cs
View file
Open in desktop
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"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this change/removal is why data.WriteLong (arg0); is removed in the unit test diff.

I think this is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was necessary because before for primitive input parameters like in void Authenticate (long operationId) the input parameter after function execution was written back to the reply Parcel which is only reqired for out and inout parameters.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…ditto here: is int[][] possible?

A quick gander at the AIDL documentation suggests it isn't possible. However, it does suggest that List:

must be one of the supported data types in this list or one of the other AIDL-generated interfaces or parcelables you've declared

which implies that a List<List<List<String>>> may be viable…and I have no idea what we'd do with that. (Wouldn't impact type.ArrayDimension, though!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I understand the AIDL dokumentation is that multidimensional arrays like int[][] are not supported. At the moment the CSharpCodeGenerator or maybe the AidlParser just ignores higher dimensions and int[][] is treated like int[]. However throwing some NotSupportedException might not be wrong.

For List I think - but not 100% sure - you can put any supported object in. And one object can be a List which can contain other Lists and so on. I don't know why (maybe a bug) but the Android Studio does not support parametrization like List<List<List<String>>>>, just List<String>. So you can use List and put Lists in but List<List>> does not work!?

The CSharpCodeGenerator might not cover all cases for Lists correct and should definetely be unit tested also in this perspect.

return "if (" + arg + " == null) { " + parcel + ".WriteInt(-1); } else { " + parcel + ".WriteInt(" + arg + ".Length); }";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that .WriteInt(-1) doesn't show up anywhere in our current expected test output, we're going to need a new unit test which ensures this code path is hit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfranke-moba : please add a unit test which causes this line to be triggered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already covered by Test11. The WriteInt(-1) will be written for out array parameters like out byte[]. In this case no byte array is written with WriteXyzArray like for in byte[] or inout byte[] as no data has to be transmitted, just the size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had missed that. :-(

} 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
Open in desktop
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