Bug 55041 - Stripping mscorlib in simple example changes IntPtr (5) behavior?
Summary: Stripping mscorlib in simple example changes IntPtr (5) behavior?
Status: VERIFIED FIXED
Alias: None
Product: Runtime
Classification: Mono
Component: General (show other bugs)
Version: master
Hardware: PC Mac OS
: High critical
Target Milestone: 15.4
Assignee: Ludovic Henry
URL:
Depends on:
Blocks:
 
Reported: 2017-04-11 19:19 UTC by Chris Hamons
Modified: 2017-08-07 16:30 UTC (History)
6 users (show)

See Also:
Tags:
Is this bug a regression?: ---
Last known good build:


Attachments

Description Chris Hamons 2017-04-11 19:19:46 UTC
I found this after figuring out that Xamarin.Mac's hybrid support wasn't working, as we were passing the runtime argument to late, and tried to write a test.

$ cat Makefile 
test::
	cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5/mscorlib.dll .
	cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5/System.dll .
	MONO_PATH=. mcs main.cs
	MONO_PATH=. mono --aot=hybrid mscorlib.dll
	MONO_PATH=. mono --hybrid-aot main.exe
	MONO_PATH=. /Library/Frameworks/Mono.framework/Commands/mono-cil-strip mscorlib.dll 
	MONO_PATH=. mono --hybrid-aot main.exe

clean::
	rm *.exe
	rm *.dll
	rm *.dylib
	rm -rf main.exe.dylib.dSYM

$ cat main.cs 
using System;

public static class EntryPoint
{
	public static void Main ()
	{
		Console.WriteLine ("Starting Test");
		var runtime_library = new IntPtr (-5 /* RTLD_MAIN_ONLY */);
		Console.WriteLine (runtime_library);
		Console.WriteLine ("Done!");
	}
}



 $ make
cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5/mscorlib.dll .
cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5/System.dll .
MONO_PATH=. mcs main.cs
warning CS8001: SDK path could not be resolved
Compilation succeeded - 1 warning(s)
MONO_PATH=. mono --aot=hybrid mscorlib.dll
Mono Ahead of Time compiler - compiling assembly /Users/donblas/tmp/il_bug/mscorlib.dll
AOTID B1C18C5D-912D-D700-26A9-4F56A125CDC9
Code: 5013177(76%) Info: 124832(1%) Ex Info: 757175(11%) Unwind Info: 80(0%) Class Info: 109219(1%) PLT: 15001(0%) GOT Info: 458512(6%) Offsets: 101614(1%) GOT: 91572
Compiled: 23814/23814 (100%), No GOT slots: 15905 (66%), Direct calls: 35928 (68%)
Executing the native assembler: "clang" -arch i386 -c -x assembler -o /var/folders/jm/dhhz2b9s0fd2brdldrkcxbn00000gn/T/mono_aot_Nq4yqc.o /var/folders/jm/dhhz2b9s0fd2brdldrkcxbn00000gn/T/mono_aot_Nq4yqc
Executing the native linker: clang -m32 -dynamiclib -o /Users/donblas/tmp/il_bug/mscorlib.dll.dylib.tmp  /var/folders/jm/dhhz2b9s0fd2brdldrkcxbn00000gn/T/mono_aot_Nq4yqc.o 
Executing dsymutil: dsymutil "/Users/donblas/tmp/il_bug/mscorlib.dll.dylib"
JIT time: 5827 ms, Generation time: 4919 ms, Assembly+Link time: 25297 ms.
MONO_PATH=. mono --hybrid-aot main.exe
Starting Test
-5
Done!
MONO_PATH=. /Library/Frameworks/Mono.framework/Commands/mono-cil-strip mscorlib.dll 
Mono CIL Stripper

Assembly mscorlib.dll stripped
MONO_PATH=. mono --hybrid-aot main.exe
Starting Test
0
Done!
Comment 1 Zoltan Varga 2017-04-12 15:59:29 UTC
This is not going to work. The stripper removes the IL code but it retains enough so its still well formed, so when the JIT tries to compile Main (), it will inline the IntPtr ctor, which is now just a 'ret'.
Comment 2 Chris Hamons 2017-04-12 16:03:34 UTC
@Zoltan - I thought the point of hybrid AOT was that you could strip it?
Comment 3 Zoltan Varga 2017-04-12 16:17:55 UTC
Hybrid aot means that all normal IL methods are aot-ed, not just some. the executable needs to be aot-ed too.
Comment 4 Chris Hamons 2017-04-12 16:26:23 UTC
@Zoltan - Yep. That appears to have fixed it:

test::
	cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5/mscorlib.dll .
	cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5/System.dll .
	MONO_PATH=. mcs main.cs
	MONO_PATH=. mono --aot=hybrid mscorlib.dll
	MONO_PATH=. mono --aot=hybrid System.dll
	MONO_PATH=. mono --aot=hybrid main.exe
	MONO_PATH=. mono --hybrid-aot main.exe
	MONO_PATH=. /Library/Frameworks/Mono.framework/Commands/mono-cil-strip mscorlib.dll 
	MONO_PATH=. mono --hybrid-aot main.exe

now gives me -5 both times.

How strict is the rule? Does every assembly used need to be AOT'ed with hybrid when you strip? Just always the executable?
Comment 5 Zoltan Varga 2017-04-12 16:35:56 UTC
Everything needs to be aot-ed. Unfortunately, we don't check for it right now.
Comment 6 Chris Hamons 2017-04-12 16:36:58 UTC
Hmm, that's unfortunately difficult to force in desktop applications, but we can at least document it.

Thanks!
Comment 7 Zoltan Varga 2017-04-12 16:40:34 UTC
Maybe we could disable JIT inlining in hybrid mode, that might fix it, but it will lead to a loss of performance.
Comment 8 Ludovic Henry 2017-04-12 17:09:20 UTC
We can't disable JITting in hybrid, as we still have wrappers to compile, and we want to keep SRE. What we can do though is disabling the load of IL code (but keep metadata) from an assembly.
Comment 9 Chris Hamons 2017-04-12 18:41:28 UTC
@Zoltan / @Ludovic - Does _every_ assembly needs to be AOT'ed for Hybrid + Stripping to work, or at least the main.exe?

For example:

Foo.exe, User.dll, System.dll, mscorlib.dll

Can I AOT just Foo/User and strip them, without AOTing System/mscorlib?
Comment 10 Zoltan Varga 2017-04-12 18:42:41 UTC
Everything needs to be aot-ed.
Comment 11 Zoltan Varga 2017-04-12 18:43:32 UTC
If something is not aot-ed the jit will try to compile it, and there is no IL, in fact there is some IL, but its just dummy, and the JIT can't detect whenever its dummy IL or not since the cil-stripper doesn't put any kind of attribute into the assembly.
Comment 12 Ludovic Henry 2017-04-12 18:48:30 UTC
@Zoltan are you sure it's not going to try to load the method from AOT first, and then if it doesn't find it, fallback to JIT it? (via mono_aot_get_method_checked called from mono_jit_compile_method_with_opt)
Comment 13 Ludovic Henry 2017-04-12 18:56:35 UTC
The problem is that, when compiling Main, we will JIT it because it hasn't been AOT, and we will try to inline intptr:.ctor. But this inline operation will not take the AOTted version, but will JIT compile the stripped version of intptr:.ctor. And because it's only "ret", it will succeed to inline.

So it's a bug with the inliner which should call the AOT version and fail to inline. Moreover, the fact that we need to AOT compile everything is not part of the hybrid "contract" as for full AOT, so relying on this behaviour is not correct.
Comment 14 Ludovic Henry 2017-04-12 18:57:31 UTC
Some output exposing the inliner problem: https://gist.github.com/luhenry/5653e7111418c51231a43aa16f4068a5
Comment 15 Zoltan Varga 2017-04-15 14:04:58 UTC
So its not possible to detect whenever an assembly has been cil-stripped because it has no additional flags/attributes, and the stripped IL is valid IL, most commonly just a 'ret'.
It might be possible to disable inlining of code which has an AOT version in hybrid mode. But the original idea behind hybrid mode was that all assemblies are AOTed.
Comment 16 Chris Hamons 2017-04-17 13:59:31 UTC
As a side note, until/while this bug is around, I PRed a change to force aot'ing all when you are in hybrid mode:

https://github.com/xamarin/xamarin-macios/pull/1995/files#diff-e197d7812329b415de024aba8c8bf277R117
Comment 17 Rodrigo Kumpera 2017-04-21 22:16:53 UTC
Hey Zoltan,

Hybrid mode must support mixing AOT'd code and JIT'ing.
That's the whole point compared to FullAOT.

This is a MAJOR problem for us. The major driver behind it is Android where SRE is used and will try to call cil-stripped code.
Comment 18 Zoltan Varga 2017-04-21 22:48:36 UTC
We could mark all methods as noinline when stripping them, so the JIT doesn't try to inline. Would this work ?
<<<<<<<<<<<<<<<<<<<<<<<<<
diff --git a/mcs/tools/cil-strip/AssemblyStripper.cs b/mcs/tools/cil-strip/AssemblyStripper.cs
index 3e5b0b9..0ad4bfc 100644
--- a/mcs/tools/cil-strip/AssemblyStripper.cs
+++ b/mcs/tools/cil-strip/AssemblyStripper.cs
@@ -147,6 +147,8 @@ namespace Mono.CilStripper {
                        for (int i = 0; i < methodTable.Rows.Count; i++) {
                                MethodRow methodRow = methodTable[i];
 
+                               methodRow.ImplFlags |= MethodImplAttributes.NoInlining;
+
                                MetadataToken methodToken = MetadataToken.FromMetadataRow (TokenType.Method, i);
 
                                MethodDefinition method = (MethodDefinition) assembly.MainModule.LookupByToken (methodToken);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Comment 19 Zoltan Varga 2017-04-21 23:05:10 UTC
https://github.com/mono/mono/pull/4732
Comment 20 Zoltan Varga 2017-04-24 20:17:51 UTC
Should be fixed now on 2017-02/2017-04/master.
Comment 21 Zoltan Varga 2017-04-24 20:24:54 UTC
Note that currently ios at least uses the cil stripper from the system mono, so this could lead to behavior differences depending on the system mono version.
Comment 22 Rodrigo Kumpera 2017-04-25 15:38:33 UTC
Zoltan, we need this fixed on 2017-02, which has not hit stable yet.
Comment 23 Rodrigo Kumpera 2017-04-25 15:45:54 UTC
Looks like I misread where it landed.
Comment 24 GouriKumari 2017-08-01 13:46:05 UTC
mac-samples when built with core and sdk hybrid, automation test results are still giving the issue with xamarin.mac-3.6.0.15.pkg and mono 5.2.0.220 (2017-04/161f032. 

##Error:  error MM0114: Hybrid AOT compilation requires all assemblies to be AOT compiled [/data/jenkins/workspace/XM/XMAOTTests-d15-3/mac-samples/MacDialog/MacDialog/MacDialog.csproj]

##Logs: 
eg: --aot=core|hybrid: http://xqa.blob.core.windows.net/gist/log-dd6bfc48a9c04caea748036fe820e9d4.txt

--aot=sdk|hybrid: http://xqa.blob.core.windows.net/gist/log-3c5d4b85b86944bfa8f49d4c405e0dbe.txt

##Supplemental Info: --aot=all|hybrid: App builds successfully with this argument. 

##Test Env:

xamarin.mac-3.6.0.15.pkg and mono 5.2.0.220 (2017-04/161f032. 

Reopening: since this is not fixed in mono2017-04.
Comment 26 GouriKumari 2017-08-07 16:30:08 UTC
Verified this bug  with the teststeps from comment #0 and alpha (d15-4) builds. Test result is returning the correct value, -5 with --aot=all|hybrid. Marking this bug as verified fixed.

## Logs:
Build Log: https://gist.github.com/GouriKumari/e8d5890925ac5536cf5d83e3b0a98bf7
Application Output: https://gist.github.com/GouriKumari/74458b48fda51a10cb4c5a0ab9375c48

##Test Env:
https://gist.github.com/602f73dd0319e666e557678d95e5203a

Note You need to log in before you can comment on or make changes to this bug.