Skip to content
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.

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

[android][armv7][aot][llvm] Llvm generates invalid code because datalayout isn't set #12307

Closed
mathieubourgeois opened this issue Jan 7, 2019 · 14 comments · Fixed by xamarin/xamarin-android#2897

Comments

@mathieubourgeois
Copy link
Contributor

@mathieubourgeois mathieubourgeois commented Jan 7, 2019

This bug is a continuation of a Xamarin.Android issue at xamarin/xamarin-android#2521. A lot of details has been added there, but I'll try to list the important parts here as well.

Creating a new Xamarin.Android app in VS with the latest Xamarin.Android master (or d16-0 p2 branch) causes a crash on code generated by llvm.

Some infos :

  • Crash only occurs on AOT+LLVM on armeabi-v7a architecture. It doesn't matter if it runs on a armv7 or arm64 device, since the issue is in the generated code.
  • The Xamarin.Android issue provides a disassembly of the offending function, plus disassembly of the bitcode before and after opt is run.
  • The problem occurs when running opt. On armeabi-v7a, this is what happens :

Before

BB98:                                             ; preds = %NOEX_BB23, %NOEX_BB11
  %116 = bitcast i32* %107 to i32**
  %117 = getelementptr i32*, i32** %116, i32 3
  %118 = load i32*, i32** %117
  %119 = bitcast i32* %118 to i32* (i32*)*
  %120 = notail call monocc i32* %119(i32* %107)
  br label %BB18

After

BB98:                                             ; preds = %BB99, %NOEX_BB11
  %114 = getelementptr i32, i32* %101, i64 6
  %115 = bitcast i32* %114 to i32* (i32*)**
  %116 = load i32* (i32*)*, i32* (i32*)** %115, align 8
  %117 = notail call monocc i32* %116(i32* %101)
  br label %BB18

The optimisation for getelementptr changed from 3 * i32* to 6 * i32, which is clearly wrong on armeabi-v7a. I managed to have it fixed by passing

llvmopts="-data-layout="p:32:32:32" -O2 -disable-tail-calls"

to opt to set the data layout for llvm (http://llvm.org/docs/LangRef.html#data-layout). Otherwise, it looks like llvm will otherwise do suppositions when running opt and was supposing that we have 64-bit pointers. From what I could find in the Mono source, it recently started setting the data layout and target triple, but only for wasm. From the llvm reference,

There is no way to generate IR that does not embed this target-specific detail into the IR. If you don’t specify the string, the default specifications will be used to generate a Data Layout and the optimization phases will operate accordingly and introduce target specificity into the IR with respect to these default specifications.

Therefore, from what I understand of it, Mono should always be specifying a target data (or, at the very least, when cross-compiling) otherwise opt is within its right to suppose and optimise the code based on what it thinks is the target data layout.

Steps to Reproduce

  1. Create a new Xamarin.Android Single-View App in VS on Windows.
  2. Disable Shared Runtime, Activate AOT+LLVM and build only for armeabi-v7a
  3. Build and run (armeabi-v7a or arm64-v8a device, doesn't matter, as long as only armv7 is built)
  4. Notice the crash

Current Behavior

App crashes

Expected Behavior

App actually runs

On which platforms did you notice this

[ ] macOS
[ ] Linux
[x] Windows

Version Used:

Xamarin.Android master 9.1.199.72
This uses mono at commit 23f2024, which is from branch 2018-08

Stacktrace

01-07 15:10:26.051  3597  3597 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
01-07 15:10:26.051  3597  3597 F DEBUG   : Build fingerprint: 'google/walleye/walleye:9/PQ1A.181205.002/5086253:user/release-keys'
01-07 15:10:26.051  3597  3597 F DEBUG   : Revision: 'MP1'
01-07 15:10:26.051  3597  3597 F DEBUG   : ABI: 'arm'
01-07 15:10:26.051  3597  3597 F DEBUG   : pid: 3576, tid: 3576, name: App3.App3  >>> App3.App3 <<<
01-07 15:10:26.051  3597  3597 F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
01-07 15:10:26.051  3597  3597 F DEBUG   : Cause: null pointer dereference
01-07 15:10:26.051  3597  3597 F DEBUG   :     r0  cb409970  r1  00000000  r2  d16b68e8  r3  e2338e00
01-07 15:10:26.051  3597  3597 F DEBUG   :     r4  cb409650  r5  cb409648  r6  00000000  r7  cac00000
01-07 15:10:26.051  3597  3597 F DEBUG   :     r8  ffc7e05c  r9  00000001  r10 cb409428  r11 cb409418
01-07 15:10:26.052  3597  3597 F DEBUG   :     ip  ffc7dd80  sp  ffc7ddb0  lr  c8cd60f8  pc  00000000
01-07 15:10:26.071  3597  3597 F DEBUG   : 
01-07 15:10:26.071  3597  3597 F DEBUG   : backtrace:
01-07 15:10:26.071  3597  3597 F DEBUG   :     #00 pc 00000000  <unknown>
01-07 15:10:26.071  3597  3597 F DEBUG   :     #01 pc 00bd20f4  /data/app/App3.App3-unl1bXynoCP19WPbU_8FFw==/lib/arm/libaot-Mono.Android.dll.so (Android_Runtime_AndroidTypeManager_RegisterNativeMembers_Java_Interop_JniType_System_Type_string+904)
01-07 15:10:26.072  3597  3597 F DEBUG   :     #02 pc 00bde1a0  /data/app/App3.App3-unl1bXynoCP19WPbU_8FFw==/lib/arm/libaot-Mono.Android.dll.so (Android_Runtime_JNIEnv_RegisterJniNatives_intptr_int_intptr_intptr_int+628)
01-07 15:10:26.072  3597  3597 F DEBUG   :     #03 pc 0000f1d2  <anonymous:ea4b1000>
@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Jan 14, 2019

Have trouble reproducing this. Could you upload the obj/Release/aot/armeabi-v7a/Mono.Android.dll
directory from your build ? Thanks.

@mathieubourgeois

This comment has been minimized.

Copy link
Contributor Author

@mathieubourgeois mathieubourgeois commented Jan 14, 2019

Mono.Android.dll.zip

Here you go. On my end, it seems pretty easy to reproduce, which is weird. However, the last master I tested on is 9.1.199.72, which is not exactly brand new (I reported the issue in Xamarin.Android essentially a month ago).

@mathieubourgeois

This comment has been minimized.

Copy link
Contributor Author

@mathieubourgeois mathieubourgeois commented Jan 14, 2019

Also, I noticed that commit a62bb95 seems to implies that llvm for 32-bit cross-compilation is still supposed to be the old version because the new one is still too unstable (maybe because of this issue?). However, from what I've understood in the build system, there's still only one llvm build in the release of Xamarin.Android (only checked on Windows as well) so my guess is it's the new one all the way right now.

@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Jan 15, 2019

I can reproduce with these files by running:
opt -O2 -o 2.bc temp.bc

@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Jan 15, 2019

Not sure why opt makes these assumptions, we don't tell it that this is a 64 bit program.
Thanks for tracking this down.

@mathieubourgeois

This comment has been minimized.

Copy link
Contributor Author

@mathieubourgeois mathieubourgeois commented Jan 15, 2019

Based on https://llvm.org/docs/LangRef.html#data-layout, I can find this section

When constructing the data layout for a given target, LLVM starts with a default set of specifications which are then (possibly) overridden by the specifications in the datalayout keyword. The default specifications are given in this list:

  • E - big endian
  • p:64:64:64 - 64-bit pointers with 64-bit alignment.
  • p[n]:64:64:64 - Other address spaces are assumed to be the same as the default address space.
  • S0 - natural stack alignment is unspecified
  • i1:8:8 - i1 is 8-bit (byte) aligned
  • i8:8:8 - i8 is 8-bit (byte) aligned
  • i16:16:16 - i16 is 16-bit aligned
  • i32:32:32 - i32 is 32-bit aligned
  • i64:32:64 - i64 has ABI alignment of 32-bits but preferred alignment of 64-bits
  • f16:16:16 - half is 16-bit aligned
  • f32:32:32 - float is 32-bit aligned
  • f64:64:64 - double is 64-bit aligned
  • f128:128:128 - quad is 128-bit aligned
  • v64:64:64 - 64-bit vector is 64-bit aligned
  • v128:128:128 - 128-bit vector is 128-bit aligned
  • a:0:64 - aggregates are 64-bit aligned

and

Instead, if specified, the target data layout is required to match what the ultimate code generator expects. This string is used by the mid-level optimizers to improve code, and this only works if it matches what the ultimate code generator uses. There is no way to generate IR that does not embed this target-specific detail into the IR. If you don’t specify the string, the default specifications will be used to generate a Data Layout and the optimization phases will operate accordingly and introduce target specificity into the IR with respect to these default specifications.

From what I can figure out, the documentation is pretty much saying that we should always be setting a datalayout, because otherwise opt would not really be able to do its job properly (my guess is not being to optimize anything related to a pointer because you don't know its size would make optimisations pretty much useless). However, finding this info, I'm worried that the issue would happen as well on x86 for Android. I think I have an old x86 Android device hanging around somewhere, I'll try and repro it on this.

vargaz added a commit to vargaz/mono that referenced this issue Jan 15, 2019
@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Jan 15, 2019

Added a layout for x86 as well.

@mathieubourgeois

This comment has been minimized.

Copy link
Contributor Author

@mathieubourgeois mathieubourgeois commented Jan 15, 2019

Just an FYI, I couldn't find that old x86 Android device, so sadly I can't confirm the issue there.

@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Jan 15, 2019

Its probably needed on all 32 bit platforms now because of the weird 64 bit pointer default.

vargaz added a commit that referenced this issue Jan 15, 2019
…ct optimizations. (#12429)

* [arm] Set the LLVM data layout, without it, opt seems to make incorrect optimizations.

Fixes #12307.

* [x86] Set the LLVM data layout.
monojenkins added a commit to monojenkins/mono that referenced this issue Jan 15, 2019
monojenkins added a commit to monojenkins/mono that referenced this issue Jan 16, 2019
monojenkins added a commit to monojenkins/mono that referenced this issue Jan 16, 2019
marek-safar added a commit that referenced this issue Jan 16, 2019
marek-safar added a commit that referenced this issue Jan 16, 2019
marek-safar added a commit that referenced this issue Jan 16, 2019
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jan 17, 2019
Fixes: mono/mono#12307
Fixes: mono/mono#12461

Bumps to dotnet/corefx@8d1b6ab.
Bumps to mono/api-snapshot@9fb4259.

Fix debugger crash.

Support dumping AOT offsets with Android NDK r17+.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Jan 18, 2019
Fixes: mono/mono#12307
Fixes: mono/mono#12461

Bumps to dotnet/corefx@8d1b6ab.
Bumps to mono/api-snapshot@9fb4259.

Fix debugger crash.

Support dumping AOT offsets with Android NDK r17+.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Jan 18, 2019
Fixes: mono/mono#12307
Fixes: mono/mono#12461

Bumps to dotnet/corefx@8d1b6ab.
Bumps to mono/api-snapshot@9fb4259.

Fix debugger crash.

Support dumping AOT offsets with Android NDK r17+.
@pjcollins

This comment has been minimized.

Copy link
Collaborator

@pjcollins pjcollins commented Jan 31, 2019

@vargaz I'm still seeing this issue with the latest d16-0 xamarin-android build which contains 5d3077c.

Is there anything I can provide to help follow up? The reproduction steps are the same as @mathieubourgeois has mentioned. Projects which support only armeabi-v7a (default setting in Release mode for new projects) crash when AOT+LLVM are also enabled.

@pjcollins pjcollins reopened this Jan 31, 2019
@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Feb 1, 2019

@vargaz could you double check this?

@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Mar 6, 2019

So the commit which actually sets the data layout wasn't backported. Will fix.

vargaz added a commit to vargaz/mono that referenced this issue Mar 6, 2019
monojenkins added a commit to monojenkins/mono that referenced this issue Mar 6, 2019
vargaz added a commit that referenced this issue Mar 6, 2019
Fixes #12307 for 2018-08.
@vargaz

This comment has been minimized.

Copy link
Member

@vargaz vargaz commented Mar 6, 2019

Xam.android needs a mono bump to pick up the fix.

jonpryor added a commit to xamarin/xamarin-android that referenced this issue Mar 7, 2019
marek-safar added a commit that referenced this issue Mar 11, 2019
Fixes #12307 for 2018-08.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Mar 29, 2019
Changes: mono/mono@204d95c...2264e1b

Fixes: mono/mono#12307
Fixes: mono/mono#12843
Fixes: mono/mono#13610

Should fix AOT failures on Windows, when the mono archive is used.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Mar 29, 2019
Fixes: mono/mono#12307
Fixes: mono/mono#12843
Fixes: mono/mono#13610

May fix AOT failures on Windows, when the mono archive is used.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Apr 24, 2019
Bumps to mono/api-snapshot@ae01378
Bumps to mono/reference-assemblies@e5173a5
Bumps to mono/bockbuild@d30329d
Bumps to mono/boringssl@3d87996
Bumps to mono/corefx@72f7d76
Bumps to mono/corert@1b7d4a1
Bumps to mono/helix-binaries@7e893ea
Bumps to mono/illinker-test-assets@f21ff68
Bumps to mono/linker@13d864e
Bumps to mono/llvm@1aaaaa5 [mono]
Bumps to mono/llvm@2c2cffe [xamarin-android]
Bumps to mono/NUnitLite@0029561
Bumps to mono/roslyn-binaries@0bbc9b4
Bumps to mono/xunit-binaries@8f6e62e

	$ git diff --shortstat 886c4901..e66c7667      # mono
        3597 files changed, 350850 insertions(+), 91128 deletions(-)
	$ git diff --shortstat 349752c464c5fc93b32e7d45825f2890c85c8b7d..2c2cffedf01e0fe266b9aaad2c2563e05b750ff4
	 240 files changed, 18562 insertions(+), 6581 deletions(-)

Context: dotnet/coreclr#22046

Fixes: CVE 2018-8292 on macOS
Fixes: http://work.devdiv.io/737323
Fixes: dotnet/corefx#33965
Fixes: dotnet/standard#642
Fixes: mono/mono#6997
Fixes: mono/mono#7326
Fixes: mono/mono#7517
Fixes: mono/mono#7750
Fixes: mono/mono#7859
Fixes: mono/mono#8360
Fixes: mono/mono#8460
Fixes: mono/mono#8766
Fixes: mono/mono#8922
Fixes: mono/mono#9418
Fixes: mono/mono#9507
Fixes: mono/mono#9951
Fixes: mono/mono#10024
Fixes: mono/mono#10030
Fixes: mono/mono#10038
Fixes: mono/mono#10448
Fixes: mono/mono#10735
Fixes: mono/mono#10735
Fixes: mono/mono#10737
Fixes: mono/mono#10743
Fixes: mono/mono#10834
Fixes: mono/mono#10837
Fixes: mono/mono#10838
Fixes: mono/mono#10863
Fixes: mono/mono#10945
Fixes: mono/mono#11020
Fixes: mono/mono#11021
Fixes: mono/mono#11021
Fixes: mono/mono#11049
Fixes: mono/mono#11091
Fixes: mono/mono#11095
Fixes: mono/mono#11123
Fixes: mono/mono#11138
Fixes: mono/mono#11146
Fixes: mono/mono#11202
Fixes: mono/mono#11214
Fixes: mono/mono#11317
Fixes: mono/mono#11326
Fixes: mono/mono#11378
Fixes: mono/mono#11385
Fixes: mono/mono#11478
Fixes: mono/mono#11479
Fixes: mono/mono#11488
Fixes: mono/mono#11489
Fixes: mono/mono#11527
Fixes: mono/mono#11529
Fixes: mono/mono#11596
Fixes: mono/mono#11603
Fixes: mono/mono#11613
Fixes: mono/mono#11623
Fixes: mono/mono#11663
Fixes: mono/mono#11681
Fixes: mono/mono#11684
Fixes: mono/mono#11693
Fixes: mono/mono#11697
Fixes: mono/mono#11779
Fixes: mono/mono#11809
Fixes: mono/mono#11858
Fixes: mono/mono#11895
Fixes: mono/mono#11898
Fixes: mono/mono#11898
Fixes: mono/mono#11965
Fixes: mono/mono#12182
Fixes: mono/mono#12193
Fixes: mono/mono#12218
Fixes: mono/mono#12235
Fixes: mono/mono#12263
Fixes: mono/mono#12307
Fixes: mono/mono#12331
Fixes: mono/mono#12362
Fixes: mono/mono#12374
Fixes: mono/mono#12402
Fixes: mono/mono#12421
Fixes: mono/mono#12461
Fixes: mono/mono#12479
Fixes: mono/mono#12479
Fixes: mono/mono#12552
Fixes: mono/mono#12603
Fixes: mono/mono#12747
Fixes: mono/mono#12831
Fixes: mono/mono#12843
Fixes: mono/mono#12881
Fixes: mono/mono#13030
Fixes: mono/mono#13284
Fixes: mono/mono#13297
Fixes: mono/mono#13455
Fixes: mono/mono#13460
Fixes: mono/mono#13478
Fixes: mono/mono#13479
Fixes: mono/mono#13522
Fixes: mono/mono#13607
Fixes: mono/mono#13610
Fixes: mono/mono#13610
Fixes: mono/mono#13639
Fixes: mono/mono#13672
Fixes: mono/mono#13834
Fixes: mono/mono#13878
Fixes: mono/mono#6352
Fixes: mono/monodevelop#6898
Fixes: xamarin/maccore#1069
Fixes: xamarin/maccore#1407
Fixes: xamarin/maccore#604
Fixes: xamarin/xamarin-macios#4984
Fixes: xamarin/xamarin-macios#5289
Fixes: xamarin/xamarin-macios#5363
Fixes: xamarin/xamarin-macios#5381
Fixes: https://issuetracker.unity3d.com/issues/editor-crashes-with-g-logv-when-entering-play-mode-with-active-flowcanvas-script
@alexanderkyte

This comment has been minimized.

Copy link
Member

@alexanderkyte alexanderkyte commented May 28, 2019

@vargaz looks like the fixes aren't having the desired impact in VS2019 with android: https://github.com/xamarin/Xamarin.Forms/issues/5787#issuecomment-496684909

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.