[Xamarin.Android.Build.Tasks] Add XA1025 error for Hybrid AOT on armeabi-v7a

[brendanzagaeski]

Context: #1218 (comment)

The undocumented, experimental Hybrid value for the
$(AndroidAotMode) MSBuild property is not currently compatible with
the armeabi-v7a target ABI. Attempting to run an app built with
$(AndroidAotMode)=Hybrid in an armeabi-v7a environment results in a
crash.

Since it is known that this configuration currently produces a crash,
emit a build error for it to improve the visibility of the known issue
and reduce the time users might spend searching for the cause of the
crash.

Example of the current crash:

F libc    : Fatal signal 11 (SIGSEGV), code 2, fault addr 0x913a50c8 in tid 31140 (ppxamarinforms1)
W         : debuggerd: handling request: pid=31140 uid=10146 gid=10146 tid=31140
F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
F DEBUG   : Build fingerprint: 'motorola/perry_metropcs_c/perry:7.1.1/NCQS26.69-64-21/33:user/release-keys'
F DEBUG   : Revision: 'p3b0'
F DEBUG   : ABI: 'arm'
F DEBUG   : pid: 31140, tid: 31140, name: ppxamarinforms1  >>> com.companyname.mobileappxamarinforms1 <<<
F DEBUG   : signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x913a50c8
F DEBUG   :     r0 be9384d4  r1 00000000  r2 be9385a0  r3 9320b1c0
F DEBUG   :     r4 00000000  r5 94167208  r6 00000000  r7 be938584
F DEBUG   :     r8 be938838  r9 ae040008  sl 00000000  fp be9385a0
F DEBUG   :     ip 913a50c8  sp be9384e4  lr 942a8420  pc 913a50c8  cpsr 000f0010
F DEBUG   :
F DEBUG   : backtrace:
F DEBUG   :     #00 pc 000250c8  [anon:libc_malloc:91380000]
F DEBUG   :     #01 pc 0000141c  <anonymous:942a7000>
W ActivityManager: Activity pause timeout for ActivityRecord{7ded4d1 u0 com.companyname.mobileappxamarinforms1/crc64e53dff5578afb8f2.MainActivity t5740}
I ActivityManager: Killing 30471:com.google.android.apps.fireball/u0a145 (adj 906): empty #13
D ConnectivityService: ConnectivityService NetworkRequestInfo binderDied(NetworkRequest [ LISTEN id=624, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ], android.os.BinderProxy@f4c4e10)
D ActivityManager: cleanUpApplicationRecord -- 30471
E ConnectivityService: RemoteException caught trying to send a callback msg for NetworkRequest [ LISTEN id=624, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ]
W         : debuggerd: resuming target 31140
I BootReceiver: Copying /data/tombstones/tombstone_05 to DropBox (SYSTEM_TOMBSTONE)
W ActivityManager:   Force finishing activity com.companyname.mobileappxamarinforms1/crc64e53dff5578afb8f2.MainActivity
I Zygote  : Process 31140 exited due to signal (11)

Other changes:

Update the HybridAOT test to cover the new error.

Correct the BuildIncrementalAot test so that it sets
$(AndroidSupportedAbis). A side effect is that the test cases that
use $(AndroidAotMode)=Full now build successfully. Note that
although those test cases now build successfully, the resulting app
packages abort when run on device because Xamarin.Android requires JIT
compilation:

Unhandled Exception:
System.ExecutionEngineException: Attempting to JIT compile method '(wrapper other) void Java.Interop.JavaVMInterface:PtrToStructure (intptr,object)' while running in aot-only mode.

TODO: Add a build error for $(AndroidAotMode)=Full, likely by
updating error XA3002.

[brendanzagaeski]

One possible reason not to add this error message could be that, as far as I can find, the Hybrid setting for the $(AndroidAotMode) is undocumented, so adding an error message for it might in a small way lead to imprecise impressions about future availability and compatibility of the feature. Hopefully the fact that the message includes the term "experimental" right up front will minimize the risk of incorrect impressions.

[brendanzagaeski]

As currently written, this new error will break builds for projects using armeabi-v7a;arm64-v8a, even though those projects would run successfully on arm64-v8a devices.

The idea is that even though users might currently be using armeabi-v7a;arm64-v8a "successfully," the inclusion of armeabi-v7a in $(AndroidSupportedAbis) gives a false impression that apps built with both ABIs enabled might work on armeabi-v7a devices too, when in fact they will not.

I think this might be an acceptable level of build breakage considering that the Hybrid setting is undocumented and therefore not expected to be used by many projects.

[jonathanpeppers]

There are probably a couple MSBuild tests like this one:
https://github.com/xamarin/xamarin-android/blob/eb89395e672b3131cf4eaf828d5de408b843f1d9/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs#L4292-L4299

You might just add a call to proj.SetAndroidSupportedAbis ("x86") or something so they don't break.

[EDMIStephen]

Thanks for adding this. I am aware the Hybrid AOT is experimental but without it Xamarin Android is not a viable option for enterprise as obfuscation can only do so much. For example dotfuscator cannot obfuscate code pulled in via nuget. I really do hope in the future Hybrid AOT can make it out of experimental.

[brendanzagaeski]

Changes in the latest commits:

Correct the failing tests. Specifically:

• Move the test for the new error to be part of the existing HybridAOT test instead of in its own separate test.

• Correct the BuildIncrementalAot test so that it sets $(AndroidSupportedAbis). A side effect is that the test cases that use $(AndroidAotMode)=Full now build successfully. Note that although those test cases now build successfully, the resulting app packages abort when run on device because Xamarin.Android requires JIT compilation:

  Unhandled Exception:
  System.ExecutionEngineException: Attempting to JIT compile method '(wrapper other) void Java.Interop.JavaVMInterface:PtrToStructure (intptr,object)' while running in aot-only mode.
  

  I will plan to submit a separate PR to add a build error for $(AndroidAotMode)=Full. I thought about adding it to this PR, but as I started looking at it, it seemed that updating the existing XA3002 error might be the best approach, and I think it might be easier to take care of that in its own PR.

[jonathanpeppers]

If the tests end up green, looks good to me 👍

[brendanzagaeski]

Changes in the latest force-push:

Correct a typo in the commit message.

[jonpryor]

@brendanzagaeski: Should there be an "easy" way to enable Hybrid+armv7, to test that it's still broken (at some point in the future)? As-is, the only way to do such "one-off" testing would be to explicitly delete the <AndroidError/> invocation, which may be "adequate," but certainly isn't easy…

[EDMIStephen]

@brendanzagaeski I don't think this PR is completely right. In VS if select just arm64-v8a then AotAssemblies gets set to false even though AndroidAotMode stays as Hybrid.
Or is this just a VS UI issue?

In VS advanced options this shows as (Notice AndroidSupportedAmis is empty

If I untick armeabi-v7a then VS alters the project to disable AOT and LLVM

So from what I can see the only way to get hybrid AOT (CIL Stripping) working is to specify which builds both arm64-v8a and armeabi-v7a.

Combining Hybrid AOT and Obfuscation seems like the only real solution for an enterprise app as obfuscation abilities are limited (dotfuscator can't bundle or obfuscate anything from nuget), bundle assemblies into native code doesn't actually perform any obfuscation.

So perhaps there are some VS bugs out there but the message I think that this PR needs to be is if you have Hybrid AOT enabled then you must set and build both arm64-v8a and armeabi-v7a but it wont work on an armeabi-v7a device.

Is there anything in the future to get Hybrid AOT out of experimental? Obfuscation alone has some blind spots that cannot be ignored.

[brendanzagaeski]

Apologies for the long delay on updating this pull request.

I investigated the behavior EDMIStephen described. With Visual Studio Enterprise 2019 version 16.7, I could not reproduce the behavior where AotAssemblies and EnableLLVM changed to false after adjustments to Supported architectures. In contrast, I was able to reproduce the behavior in Visual Studio Professional and Visual Studio Community, but that is expected because the AOT Compilation and Use LLVM Optimizing Compiler settings are only available in the Enterprise edition. Curiously, EDMIStephen was using Visual Studio Enterprise, so I'm not sure why the settings changed to false in EDMIStephen's environment. I will tentatively guess it was an unexpected hiccup in that particular environment and hope that the issue has been resolved for EDMIStephen in the latest versions, but if any other users see a similar behavior in Visual Studio Enterprise in the future, be sure to Report a Problem for it. It would indeed be an issue with the Visual Studio property pages UI in that case. Thanks in advance!

Additional testing notes:

To check that the proposed error message logic and wording are accurate, I used Visual Studio 2019 version 16.8 Preview with different values of AndroidSupportedAbis:

• arm64-v8a: The assemblies were stripped, and the application ran successfully on an arm64-v8a device.
• armeabi-v7a;arm64-v8a: The assemblies were stripped, the application ran successfully on an arm64-v8a device, and the application aborted as described in Hybrid AOT doesn't work on arm64 device if built with only armv7 architecture selected #1218 on an armeabi-v7a device.
• armeabi-v7a: The assemblies were stripped, and the application aborted as described in Hybrid AOT doesn't work on arm64 device if built with only armv7 architecture selected #1218 on both arm64-v7a and armeabi-v7a devices.

(In all scenarios, I had AndroidAotMode set to Hybrid, and AotAssemblies and EnableLLVM set to true. I also had AndroidEnableAssemblyCompression set to false to make it easier to verify that the assemblies in the final APK were stripped.)

In summary, the current proposed wording and logic for the error message in this PR align with the observed behaviors for all of the scenarios, so this PR is looking good to move forward.

Next steps:

I will rebase the PR and add a way to disable the error to make it easier to test the Hybrid + armeabi-v7a combination in the future.

[brendanzagaeski]

Changes in the latest force-push:

• Rebase.
• Add a way to disable the error. Setting $(AndroidAotModeValidateAbi)=false will now disable the error. I think a "public" property is OK because the property is expected to be API-stable until the error is no longer relevant (that is, until $(AndroidAotMode)=Hybrid produces apps that run successfully on armeabi-v7a). Because AndroidAotMode is not documented, I also left the new property likewise undocumented. I'm happy to adjust either of these decisions.
• Update the HybridAot test to cover the new $(AndroidAotModeValidateAbi) property.

[brendanzagaeski]

Setting $(AndroidAotModeValidateAbi)=false will now disable the error. I think a "public" property is OK because the property is expected to be API-stable until the error is no longer relevant (that is, until $(AndroidAotMode)=Hybrid produces apps that run successfully on armeabi-v7a). Because AndroidAotMode is not documented, I also left the new property likewise undocumented. I'm happy to adjust either of these decisions.

I debated a little bit on the name of the property. In particular, I wondered whether it would be preferable to word the property in the "negative" like $(AndroidAotModeSkipValidateAbi) because the default behavior right now is that validation is enabled when AndroidAotModeValidateAbi is null, which might be a tiny bit counter-intuitive. Keeping the property in the "positive" keeps the name shorter though, so overall I decided I'd keep it that way for now.

[dellis1972]

I think if we make AndroidAotModeValidateAbi a public property we need to document it. We should probably document AndroidAotMode as well but that could probably be done in another PR maybe..

[jonathanpeppers]

Should we document $(AndroidAotModeValidateAbi) here as well as in BuildProcess.md?

[dellis1972]

Yup

[brendanzagaeski]

As I was writing a note for the AndroidAotModeValidateAbi new property, I began to think it would make more sense to make it an underscore-prefixed "private" property that is only intended for use in a new runtime test that will check if apps built with AndroidAotMode=Hybrid still crash on armeabi-v7a devices. If the test fails in the future, it will mean that armeabi-v7a has started working, so the error can be removed.

The chunk of draft release note I had stared writing was:

If needed, the error can also be disabled by setting the AndroidAotModeValidateAbi MSBuild property to false. Generally this is not recommended because it will allow building app packages that abort during startup on armeabi-v7a ABI device environments.

As I wrote that out, I realized I couldn't think of much value in the property for external users. Most of the value would be for xamarin-android's test suite.

I'll tentatively move forward with that line of thinking, making the property "private" and adding an on-device test for this configuration. But if anybody has other thoughts, I'm happy to adjust that plan, so let me know!

[jonpryor]

making the property "private" and adding an on-device test for this configuration

I'm fine with that idea. 👍

[brendanzagaeski]

Changes in the latest force-push:

• Squash the commits.
• Rename AndroidAotModeValidateAbi to _AndroidAotModeValidateAbi to make it "private".
• Add a AndroidAotModeHybridAbortsOnArmeabiV7a on-device test to remind the team to remove the error if hybrid AOT becomes compatible with armeabi-v7a in the future.
• Expand the release note to include the information from guides/messages/xa1025.md about how to resolve the error.

[brendanzagaeski]

Changes in the latest force-push:

• Remove the AndroidAotModeHybridAbortsOnArmeabiV7a on-device test from this PR. Based on a discussion with Peter Collins around how to run the test on an armeabi-v7a environment, proceeding without a test is the most appropriate option for now. In the past there were some AOT tests that ran on hardware ARM devices, but those are now deprecated (https://github.com/xamarin/QualityAssurance/commit/62d40bfb933d1ac0af27cf33311c837c4c3efbbc) in favor of tests that can run on emulator.

  The proposed AndroidAotModeHybridAbortsOnArmeabiV7a test is essentially the "right" test to add. It does crash correctly on an ARM emulator. The problem is that the test needs to run on an armeabi-v7a environment, and at the moment, the test suite does not yet run on any ARM emulator images, so it can't be added to the test suite yet.

  I have submitted [tests] If test suite adds ARM emulator runs in the future, add test for hybrid AOT + armeabi-v7a expected crash #5291 to track addition of a corresponding test if the test suite adds ARM emulator runs in the future. As mentioned on there, it's possible that .NET 6 or deprecation of armeabi-v7a support might obsolete the whole issue, so the risk that hybrid AOT will become compatible with armeabi-v7a before one of those things happens might be fairly low.

• Adjust the HybridAOT test to ensure that setting _AndroidAotModeValidateAbi=false disables the error. This will be useful for future manual testing or for automated testing if [tests] If test suite adds ARM emulator runs in the future, add test for hybrid AOT + armeabi-v7a expected crash #5291 is added in the future.