Mono.Android-Tests crash when AOT is enabled #56315

jonathanpeppers · 2021-07-26T16:21:22Z

Description

We're hitting a test causes the test app to crash when using -p:RunAOTCompilation=true, but it works fine with JIT:

********** Crash dump: **********
Build fingerprint: 'google/sdk_gphone_x86_64/generic_x86_64:9/PSR1.180720.075/5124027:user/release-keys'
#00 0x00059eac /data/app/Mono.Android.NET_Tests-vtnerz4bHqSkZSQWYCTLvw==/lib/x86/libmonosgen-2.0.so
ves_icall_System_IO_Stream_HasOverriddenBeginEndWrite
/__w/1/s\src/mono/mono/metadata/icall.c:3261:38
ves_icall_System_IO_Stream_HasOverriddenBeginEndWrite_raw
/__w/1/s\src/mono/mono/metadata/icall-def-netcore.h:128:0
#01 0x0000d84f <anonymous:e25e6000>
Crash dump is completed

It seems to happen during this test:

07-26 10:50:51.003  8393  8410 I NUnit   : Redirect_POST_With_Content_Works

https://github.com/xamarin/xamarin-android/blob/e4debf72015549e6e19cbfb247a47fead6ce7a01/tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidClientHandlerTests.cs#L387-L408

Configuration

dotnet --version
6.0.100-rc.1.21372.10

Runtime version:

~\.nuget\packages\microsoft.netcore.app.runtime.mono.android-x86\6.0.0-rc.1.21372.1\runtimes\android-x86\native\libmonosgen-2.0.so

Regression?

No, Android AOT is new.

Other information

Full logcat output: aot.txt

/cc @steveisok

The text was updated successfully, but these errors were encountered:

ghost · 2021-07-26T16:21:27Z

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

We're hitting a test that fails when using -p:RunAOTCompilation=true, but it works fine with JIT:

********** Crash dump: **********
Build fingerprint: 'google/sdk_gphone_x86_64/generic_x86_64:9/PSR1.180720.075/5124027:user/release-keys'
#00 0x00059eac /data/app/Mono.Android.NET_Tests-vtnerz4bHqSkZSQWYCTLvw==/lib/x86/libmonosgen-2.0.so
ves_icall_System_IO_Stream_HasOverriddenBeginEndWrite
/__w/1/s\src/mono/mono/metadata/icall.c:3261:38
ves_icall_System_IO_Stream_HasOverriddenBeginEndWrite_raw
/__w/1/s\src/mono/mono/metadata/icall-def-netcore.h:128:0
#01 0x0000d84f <anonymous:e25e6000>
Crash dump is completed

It seems to happen during this test:

07-26 10:50:51.003  8393  8410 I NUnit   : Redirect_POST_With_Content_Works

https://github.com/xamarin/xamarin-android/blob/e4debf72015549e6e19cbfb247a47fead6ce7a01/tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidClientHandlerTests.cs#L387-L408

Configuration

dotnet --version
6.0.100-rc.1.21372.10

Regression?

No, Android AOT is new.

Other information

Full logcat output: aot.txt

/cc @steveisok

Author:	jonathanpeppers
Assignees:	-
Labels:	`area-Infrastructure-mono`, `untriaged`
Milestone:	-

jonathanpeppers · 2021-07-26T16:25:19Z

-p:UseNativeHttpHandler=false makes the crash go away. But then we get other test failures because AndroidMessageHandler isn't used.

Fixes: #6052 Helpful reading: * https://github.com/dotnet/runtime/blob/15dec9a2aa5a4236d6ba70de2e9c146867b9d2e0/src/tasks/AotCompilerTask/MonoAOTCompiler.cs * https://github.com/dotnet/runtime/blob/15dec9a2aa5a4236d6ba70de2e9c146867b9d2e0/src/mono/netcore/nuget/Microsoft.NET.Runtime.MonoAOTCompiler.Task/README.md To make this work, I moved the existing `<Aot/>` MSBuild task calls to a new `_AndroidAot` MSBuild target in `Xamarin.Android.Legacy.targets`. In the .NET 6 targets, there is a *different* `_AndroidAot` MSBuild target that runs the new `<MonoAOTCompiler/>` MSBuild task. The `_AndroidAot` target runs per `$(RuntimeIdentifier)` after the linker completes. Native libraries are added to the `@(ResolvedFileToPublish)` item group to be included in the final `.apk`. To follow convention with Blazor WASM: https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-6-preview-4/#blazor-webassembly-ahead-of-time-aot-compilation The `$(RunAOTCompilation)` MSBuild property enables AOT. To help with backwards compatibility with "legacy" Xamarin.Android, I kept the `$(AotAssemblies)` MSBuild property intact. Unfortunately, we have to manually import things to support `$(AotAssemblies)`: <ImportGroup Condition=" '$(MonoAOTCompilerTasksAssemblyPath)' == '' and '$(AotAssemblies)' == 'true' "> <Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.MonoAOTCompiler.Task" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-x86" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-x64" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-arm" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-arm64" /> </ImportGroup> Since, the default Mono workload does not support `$(AotAssemblies)`: https://github.com/dotnet/runtime/blob/69711860262e44458bbe276393ea3eb9f7a2192a/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.targets.in#L20-L25 I think this is reasonable for now. ~~ Limitations ~~ * Profiled AOT is not implemented yet: #6053 * `$(EnableLLVM)` fails when locating `opt`: dotnet/runtime#56386 * `AndroidClientHandler` usage causes runtime crash: dotnet/runtime#56315 * Use of folder names like `Build AndÜmläüts` fails: dotnet/runtime#56163 ~~ Results ~~ All tests were running on a Pixel 5. Using the HelloAndroid app: https://github.com/dotnet/maui-samples/tree/main/HelloAndroid Defaults to two architectures: arm64 and x86 Average of 10 runs with `-c Release` and no AOT: Activity: Displayed 00:00:00.308 Apk size: 8367969 Average of 10 runs with `-c Release -p:RunAOTCompilation=true`: Activity: Displayed 00:00:00.209 Apk size: 12082123 Using the HelloMaui app: https://github.com/dotnet/maui-samples/tree/main/HelloMaui Defaults to two architectures: arm64 and x86 Average of 10 runs with `-c Release` and no AOT: Activity: Displayed 00:00:01.117 Apk size: 16272964 Average of 10 runs with `-c Release -p:RunAOTCompilation=true`: Activity: Displayed 00:00:00.568 Apk size: 42869016

Fixes: #6052 Helpful reading: * https://github.com/dotnet/runtime/blob/15dec9a2aa5a4236d6ba70de2e9c146867b9d2e0/src/tasks/AotCompilerTask/MonoAOTCompiler.cs * https://github.com/dotnet/runtime/blob/15dec9a2aa5a4236d6ba70de2e9c146867b9d2e0/src/mono/netcore/nuget/Microsoft.NET.Runtime.MonoAOTCompiler.Task/README.md To make this work, I moved the existing `<Aot/>` MSBuild task calls to a new `_AndroidAot` MSBuild target in `Xamarin.Android.Legacy.targets`. In the .NET 6 targets, there is a *different* `_AndroidAot` MSBuild target that runs the new `<MonoAOTCompiler/>` MSBuild task. The `_AndroidAot` target runs once per `$(RuntimeIdentifier)`, after the linker completes. Native libraries are added to the `@(ResolvedFileToPublish)` item group to be included in the final `.apk`. To follow [convention with Blazor WASM][0], the `$(RunAOTCompilation)` MSBuild property enables AOT. To help with backwards compatibility with "legacy" Xamarin.Android, I kept the `$(AotAssemblies)` MSBuild property intact. Unfortunately, we have to manually import things to support `$(AotAssemblies)`: <ImportGroup Condition=" '$(MonoAOTCompilerTasksAssemblyPath)' == '' and '$(AotAssemblies)' == 'true' "> <Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.MonoAOTCompiler.Task" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-x86" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-x64" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-arm" /> <Import Project="Sdk.props" Sdk="Microsoft.NETCore.App.Runtime.AOT.Cross.android-arm64" /> </ImportGroup> as the [default Mono workload does not support `$(AotAssemblies)`][1]. I think this is reasonable for now. ~~ Limitations ~~ Profiled AOT is not implemented yet: * #6053 `$(EnableLLVM)` fails when locating `opt`: * dotnet/runtime#56386 `$(AndroidClientHandler)` usage causes runtime crash: * dotnet/runtime#56315 Use of folder names like `Build AndÜmläüts` fails: * dotnet/runtime#56163 ~~ Results ~~ All tests: 1. Were running on a [Google Pixel 5][2], and 2. Enabled two architectures, arm64 and x86, and 3. **JIT time** was average of 10 runs with `-c Release`, with the `Activity: Displayed` time, and 4. **AOT time** was average of 10 runs with `-c Release -p:RunAOTCompilation=true` with the `Activity: Displayed` time. 5. Δ values are (AOT / JIT)*100. | Test | JIT time | AOT time | Δ time | JIT apk size | AOT apk size | Δ size | | ------------------- | ------------: | ------------: | ------: | ------------: | ------------: | -----: | | [HelloAndroid][3] | 00:00:00.308 | 00:00:00.209 | 68% | 8,367,969 | 12,082,123 | 144.4% | | [HelloMaui][4] | 00:00:01.117 | 00:00:00.568 | 51% | 16,272,964 | 42,869,016 | 263.4% | [0]: https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-6-preview-4/#blazor-webassembly-ahead-of-time-aot-compilation [1]: https://github.com/dotnet/runtime/blob/69711860262e44458bbe276393ea3eb9f7a2192a/src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Manifest/WorkloadManifest.targets.in#L20-L25 [2]: https://store.google.com/us/product/pixel_5_specs?hl=en-US [3]: https://github.com/dotnet/maui-samples/tree/714460431541f40570e91225e8ba4bc1fe08025f/HelloAndroid [4]: https://github.com/dotnet/maui-samples/tree/714460431541f40570e91225e8ba4bc1fe08025f/HelloMaui

steveisok · 2021-07-28T19:12:38Z

/cc @lambdageek @vargaz

lambdageek · 2021-07-28T23:59:53Z

@jonathanpeppers does the IL trimmer run when you AOT this test?

I see that in the HasOverridenBeginEndRead implementation we explicitly check whether the linker removed the Stream BeginRead / EndRead methods, but in HasOverridenBeginEndWrite we don't

runtime/src/mono/mono/metadata/icall.c

Lines 3231 to 3265 in 6d35185

    
           ves_icall_System_IO_Stream_HasOverriddenBeginEndRead (MonoObjectHandle stream, MonoError *error) 
        
           { 
        
           	MonoClass* curr_klass = MONO_HANDLE_GET_CLASS (stream); 
        
           	MonoClass* base_klass = mono_class_try_get_stream_class (); 
        
           	if (!io_stream_slots_set) 
        
           		init_io_stream_slots (); 
        
           	// slots can still be -1 and it means Linker removed the methods from the base class (Stream) 
        
           	// in this case we can safely assume the methods are not overridden 
        
           	// otherwise - check vtable 
        
           	MonoMethod **curr_klass_vtable = m_class_get_vtable (curr_klass); 
        
           	gboolean begin_read_is_overriden = io_stream_begin_read_slot != -1 && curr_klass_vtable [io_stream_begin_read_slot]->klass != base_klass; 
        
           	gboolean end_read_is_overriden = io_stream_end_read_slot != -1 && curr_klass_vtable [io_stream_end_read_slot]->klass != base_klass; 
        
           	// return true if BeginRead or EndRead were overriden 
        
           	return begin_read_is_overriden || end_read_is_overriden; 
        
           } 
        
           MonoBoolean 
        
           ves_icall_System_IO_Stream_HasOverriddenBeginEndWrite (MonoObjectHandle stream, MonoError *error) 
        
           { 
        
           	MonoClass* curr_klass = MONO_HANDLE_GETVAL (stream, vtable)->klass; 
        
           	MonoClass* base_klass = mono_class_try_get_stream_class (); 
        
           	if (!io_stream_slots_set) 
        
           		init_io_stream_slots (); 
        
           	MonoMethod **curr_klass_vtable = m_class_get_vtable (curr_klass); 
        
           	gboolean begin_write_is_overriden = curr_klass_vtable [io_stream_begin_write_slot]->klass != base_klass; 
        
           	gboolean end_write_is_overriden = curr_klass_vtable [io_stream_end_write_slot]->klass != base_klass; 
        
           	// return true if BeginWrite or EndWrite were overriden 
        
           	return begin_write_is_overriden || end_write_is_overriden; 
        
           }

So curr_klass_vtable [io_stream_begin_write_slot]->klass might be a bad access.

jonathanpeppers · 2021-07-29T13:32:23Z

@lambdageek yes, the tests that fail like this were built for Release, and so they are trimmed.

lambdageek · 2021-07-29T13:39:56Z

Ok, I'm going to try to PR the same check for trimmed methods in BeginWrite/EndWrite - I'm assuming that will fix it.

…rridden Add the same logic that we already have for BeginRead/EndRead - if we can't find a vtable slot that has the method in the base class, it must have been linked out, so a subclass can't possibly override it. Fixes dotnet#56315

…rridden (#56559) Add the same logic that we already have for BeginRead/EndRead - if we can't find a vtable slot that has the method in the base class, it must have been linked out, so a subclass can't possibly override it. Fixes #56315

ghost added area-Infrastructure-mono untriaged New issue has not been triaged by the area owner labels Jul 26, 2021

jonathanpeppers mentioned this issue Jul 27, 2021

[One .NET] AOT support dotnet/android#5539

Merged

ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Jul 28, 2021

ViktorHofer added this to the 6.0.0 milestone Jul 28, 2021

steveisok added area-Codegen-AOT-mono and removed area-Infrastructure-mono labels Jul 28, 2021

lambdageek added area-Interop-mono and removed area-Codegen-AOT-mono labels Jul 29, 2021

lambdageek self-assigned this Jul 29, 2021

lambdageek mentioned this issue Jul 29, 2021

[icall] If Stream:BeginWrite/EndWrite are linked out, they're not overridden #56559

Merged

ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 29, 2021

lambdageek closed this as completed in #56559 Jul 30, 2021

ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 30, 2021

ghost locked as resolved and limited conversation to collaborators Aug 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mono.Android-Tests crash when AOT is enabled #56315

Mono.Android-Tests crash when AOT is enabled #56315

jonathanpeppers commented Jul 26, 2021 •

edited

Loading

ghost commented Jul 26, 2021

Description

Configuration

Regression?

Other information

jonathanpeppers commented Jul 26, 2021

steveisok commented Jul 28, 2021

lambdageek commented Jul 28, 2021

jonathanpeppers commented Jul 29, 2021

lambdageek commented Jul 29, 2021

Mono.Android-Tests crash when AOT is enabled #56315

Mono.Android-Tests crash when AOT is enabled #56315

Comments

jonathanpeppers commented Jul 26, 2021 • edited Loading

Description

Configuration

Regression?

Other information

ghost commented Jul 26, 2021

Description

Configuration

Regression?

Other information

jonathanpeppers commented Jul 26, 2021

steveisok commented Jul 28, 2021

lambdageek commented Jul 28, 2021

jonathanpeppers commented Jul 29, 2021

lambdageek commented Jul 29, 2021

jonathanpeppers commented Jul 26, 2021 •

edited

Loading