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

Windows i386 CI very high failure rate lately. #11898

Closed
jaykrell opened this issue Dec 2, 2018 · 14 comments · Fixed by xamarin/xamarin-android#2609

Comments

@jaykrell
Copy link
Collaborator

@jaykrell jaykrell commented Dec 2, 2018

Windows i386 CI is failing very frequently lately.
In pinvoke2.exe.
Here is a reduced case that fails.
It is heap corruption -- well, mismatched allocate vs. free -- different heap.
If you run under a debugger, it breaks in due to the heap corruption detected in free.

using System;
using System.Runtime.InteropServices;

public class Tests
{
	[DllImport ("libtest", EntryPoint="mono_test_asany")]
	public static extern int mono_test_asany ([MarshalAs (UnmanagedType.AsAny)] object o, int what);

	public static int Main (string[] args)
	{
		mono_test_asany ("ABC", 2);
		return 0;
	}
}
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 2, 2018
This regressed six months ago in mono#9503.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 2, 2018

There is a second problem sometimes in this test:

cant resolve internal call to "Tests/T2::MyClone" (tested without signature also)

Your mono runtime and class libraries are out of sync.
The out of sync library is: C:\s2\mono\mono\tests\pinvoke2.exe

When you update one from git you need to update, compile and install
the other too.
Do not report this as a bug unless you're sure you have updated correctly:
you probably have a broken mono install.
If you see other errors or faults after this message they are probably related
and you need to fix your mono install first.
test_0_x86_single_double_struct_ret failed: got 1, expected 0
Regression tests: 97 ran, 1 failed in Tests

that I haven't debugged yet, which really might be what is failing in CI.

The allocater/free problem shows as a break into the debugger with free.
Perhaps outside of a debugger it is tolerated and no failure.

@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 2, 2018

Actually that is maybe just expected, not an error. I need to look more.

@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 2, 2018

Yes, indeed.

@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 2, 2018

So I think the only problem is the allocate/free mismatch, fixed by #11899. Let's see how CI fairs with that.

jaykrell added a commit to jaykrell/mono that referenced this issue Dec 2, 2018
This regressed six months ago in mono#9503.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 2, 2018
This regressed six months ago in mono#9503.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 2, 2018
This regressed six months ago in mono#9503.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 2, 2018
This regressed six months ago in mono#9503.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
akoeplinger added a commit that referenced this issue Dec 3, 2018
Fix #11898
This regressed six months ago at https://github.com/mono/mono/pull/9503/files#r238093574.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
monojenkins added a commit to monojenkins/mono that referenced this issue Dec 3, 2018
There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
monojenkins added a commit to monojenkins/mono that referenced this issue Dec 3, 2018
There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
luhenry added a commit that referenced this issue Dec 4, 2018
luhenry added a commit that referenced this issue Dec 4, 2018
There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 4, 2018

The above problem is specific to debug builds, because the debug C runtime heap.
Retail would be fine.
There is still a problem.

@jaykrell jaykrell reopened this Dec 4, 2018
@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 4, 2018

@vargaz

Regarding Windows i386 failure.
pinvoke2.exe.

gcc and msvc don’t seem to agree on the ABI.
gcc seems wrong, because, the types in the
struct should not matter.

gcc returns the struct in ST0.
msvc in edx:eax.
I can recompile the test in msvc and that will presumably
work, but I don’t think CI does that.

I try putting a union in there to encourage it.
(I forgot __stdcall but that doesn’t fix it.)

type 1.c

typedef struct {
        double d;
} SingleDoubleStruct;

SingleDoubleStruct
mono_test_marshal_return_single_double_struct (void)
{
        SingleDoubleStruct res;
        res.d = 3.0;
	return res;
}

\cygwin64\bin\gcc -m32 -c -S 1.c

C:\s2\mono>type 1.s

_mono_test_marshal_return_single_double_struct:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $16, %esp
        fldl    LC0
        fstpl   -8(%ebp)
        fldl    -8(%ebp)
        leave
        ret

cl /c  1.c  /O2

Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26729 for x86

link /dump  /disasm  1.obj

_mono_test_marshal_return_single_double_struct:
  00000000: 83 EC 08           sub         esp,8
  00000003: F2 0F 10 05 00 00  movsd       xmm0,mmword ptr [__real@4008000000000000]
            00 00
  0000000B: F2 0F 11 04 24     movsd       mmword ptr [esp],xmm0
  00000010: 8B 04 24           mov         eax,dword ptr [esp]
  00000013: 8B 54 24 04        mov         edx,dword ptr [esp+4]
  00000017: 83 C4 08           add         esp,8
  0000001A: C3                 ret

And it looks like mono has a third view of the world.
It thinks the structs come back through a pointer to local.

083b0d73 8d45f8          lea     eax,[ebp-8]
083b0d76 890424          mov     dword ptr [esp],eax
083b0d79 8bc0            mov     eax,eax ; huh?
083b0d7b e850000000      call    083b0dd0
083b0d80 83ec04          sub     esp,4
083b0d83 dd45f8          fld     qword ptr [ebp-8]
@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 4, 2018

I can get gcc to agree with msvc by unioning in a long long.
mono still doesn't agree, because !pinvoke.
The mono code looks correct, if the signature is pinvoke.

jaykrell added a commit to jaykrell/mono that referenced this issue Dec 4, 2018
Windows i386 is frequently failing in CI.
A problem is that given:

typedef struct {
        double d;
 } SingleDoubleStruct;

 LIBTEST_API SingleDoubleStruct STDCALL
 mono_test_marshal_return_single_double_struct (void);

Visual C++ and mono expect the return value in  edx:eax.
Gcc returns it in ST0.
Changing struct to union fixes it.

Mono "actually" expects return value in local via pointer
in extra parameter, but it generates a wrapper to bridge
its custom (SysV?) ABI to the NT ABI.
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 4, 2018
Windows i386 is frequently failing in CI.
A problem is that given:

libtest.c:
typedef struct {
        double d;
 } SingleDoubleStruct;

SingleDoubleStruct mono_test_marshal_return_single_double_struct (void);

Visual C++ and mono expect the return value in edx:eax.
Gcc returns it in ST0.
Changing struct to union fixes it.

Mono "actually" expects return value in local via pointer
in extra parameter, but it generates a wrapper to bridge
its custom (SysV?) ABI to the NT ABI.
@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 4, 2018

Workaround gcc ABI by changing struct to union.

jaykrell added a commit to jaykrell/mono that referenced this issue Dec 4, 2018
Windows i386 is frequently failing in CI.
A problem is that given:

libtest.c:
typedef struct {
        double d;
 } SingleDoubleStruct;

SingleDoubleStruct mono_test_marshal_return_single_double_struct (void);

Visual C++ and mono expect the return value in edx:eax.
Gcc returns it in ST0.
Changing struct to union fixes it.

Mono "actually" expects return value in local via pointer
in extra parameter, but it generates a wrapper to bridge
its custom (SysV?) ABI to the NT ABI.
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 4, 2018
Windows i386 is frequently failing in CI.
A problem is that given:

libtest.c:
typedef struct {
        double d;
 } SingleDoubleStruct;

SingleDoubleStruct mono_test_marshal_return_single_double_struct (void);

Visual C++ and mono expect the return value in edx:eax.
Gcc returns it in ST0.
Changing struct to union fixes it.

Mono "actually" expects return value in local via pointer
in extra parameter, but it generates a wrapper to bridge
its custom (SysV?) ABI to the NT ABI.
jaykrell added a commit to jaykrell/mono that referenced this issue Dec 4, 2018
https://gcc.gnu.org/bugzilla/show_activity.cgi?id=88352

Windows i386 is frequently failing in CI.
A problem is that given:

libtest.c:
typedef struct {
        double d;
 } SingleDoubleStruct;

SingleDoubleStruct mono_test_marshal_return_single_double_struct (void);

Visual C++ and mono expect the return value in edx:eax.
Gcc returns it in ST0.
Changing struct to union fixes it.

Mono "actually" expects return value in local via pointer
in extra parameter, but it generates a wrapper to bridge
its custom (SysV?) ABI to the NT ABI.
@lateralusX

This comment has been minimized.

Copy link
Member

@lateralusX lateralusX commented Dec 4, 2018

Any clue why this has started to happen, looks like the source hasn't changed for a while? On CI we run the msvc build mono runtime and should load the msvc build version of libtest.dll so maybe CI has started to pick up gcc build of the test library instead or started to install new version of gcc introducing this change?

@lateralusX

This comment has been minimized.

Copy link
Member

@lateralusX lateralusX commented Dec 4, 2018

I believe there potentially will be other issues using gcc libraries with msvc build runtime apart from this specific issue. CI should use msvc build runtime and msvc build libtest.dll.

@lateralusX

This comment has been minimized.

Copy link
Member

@lateralusX lateralusX commented Dec 4, 2018

Looked at Windows master CI builds but didn't find failures related to pinvoke2.exe failures? PR says frequent failures on i386, only for PR builds and not master?

@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 4, 2018

I don't know and wonder the same. I don't really know what the builds and CI do. For example I thought there was no msvc build of tests.
Locally I specifically did a cygwin/mingw build, an msbuild build, and ran the tests manually.

alexischr added a commit to alexischr/mono that referenced this issue Dec 5, 2018
Fix mono#11898
This regressed six months ago at https://github.com/mono/mono/pull/9503/files#r238093574.

There are two identical sounding functions:

mono_string_handle_to_utf8:
  What we usually use. Sounds reasonable. Usually is reasonable.
  Allocates with malloc.

mono_string_to_utf8str_handle:
  What should be used here. Allocates with CoTaskMemAlloc.
  So it can be freed with marshal_free => CoTaskMemFree.

i.e. The allocator and the freeer need to agree.
@lateralusX

This comment has been minimized.

Copy link
Member

@lateralusX lateralusX commented Dec 6, 2018

@jaykrell guess this can be closed?

@jaykrell

This comment has been minimized.

Copy link
Collaborator Author

@jaykrell jaykrell commented Dec 6, 2018

Yes.

@jaykrell jaykrell closed this Dec 6, 2018
lewurm added a commit to lewurm/xamarin-macios that referenced this issue Dec 7, 2018
Commit list for mono/mono:

* mono/mono@f919fb5 Fix mono/mono#11898 (#11904)
* mono/mono@95fbc22 Fix race condition in XmlCharType.Instance (#11828)
* mono/mono@bcda004 [aot] Emit runtime invoke wrappers for up to 40 parameters for bitcode, newly added tests depend on it.
* mono/mono@f0e4666 [crash] Add managed exception class name (#11821)
* mono/mono@23f2024 [2018-08] Fix Encoding serialization issue (#11807)
* mono/mono@414cafa [interp] Don't include internal frames in stack trace (#11793)
* mono/mono@68746a8 [ci] Disable log compression in pipeline builds for now - second attempt
* mono/mono@1f62222 [mini] use AOT trampolines in interp mixed mode (#11781)
* mono/mono@d0184b6 [ci] Disable log compression in pipeline builds for now
* mono/mono@a4c0912 [loader] ignore 'internalcall' impl attribute on 'abstract' methods
* mono/mono@7b9d9b3 [arm64] Fix passing r4/r8 arguments on the stack in the gsharedvt trampoline.
* mono/mono@e3a4484 [2018-08] Add .NET 4.7.2 reference assemblies (#11733)

Diff: mono/mono@99b6da7...f919fb5
rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Dec 10, 2018
* bump mono (2018-08)

Commit list for mono/mono:

* mono/mono@f919fb5 Fix mono/mono#11898 (#11904)
* mono/mono@95fbc22 Fix race condition in XmlCharType.Instance (#11828)
* mono/mono@bcda004 [aot] Emit runtime invoke wrappers for up to 40 parameters for bitcode, newly added tests depend on it.
* mono/mono@f0e4666 [crash] Add managed exception class name (#11821)
* mono/mono@23f2024 [2018-08] Fix Encoding serialization issue (#11807)
* mono/mono@414cafa [interp] Don't include internal frames in stack trace (#11793)
* mono/mono@68746a8 [ci] Disable log compression in pipeline builds for now - second attempt
* mono/mono@1f62222 [mini] use AOT trampolines in interp mixed mode (#11781)
* mono/mono@d0184b6 [ci] Disable log compression in pipeline builds for now
* mono/mono@a4c0912 [loader] ignore 'internalcall' impl attribute on 'abstract' methods
* mono/mono@7b9d9b3 [arm64] Fix passing r4/r8 arguments on the stack in the gsharedvt trampoline.
* mono/mono@e3a4484 [2018-08] Add .NET 4.7.2 reference assemblies (#11733)

Diff: mono/mono@99b6da7...f919fb5

* empty commit (trigger CI)
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jan 11, 2019
Fixes: mono/mono#11874
Fixes: mono/mono#11898
Fixes: mono/mono#12093
Fixes: mono/mono#12130
Fixes: mono/mono#12131

Fix race condition in XmlCharType.Instance.

Fix IndexOutOfRangeException in MethodInfo.ReturnParameter.IsDefined

Fix CVE 2018-8292 on macOS.

NDK r18 build support.

Crash reporter fixes.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Jan 11, 2019
Fixes: mono/mono#11874
Fixes: mono/mono#11898
Fixes: mono/mono#12093
Fixes: mono/mono#12130
Fixes: mono/mono#12131

Fix race condition in XmlCharType.Instance.

Fix IndexOutOfRangeException in MethodInfo.ReturnParameter.IsDefined

Fix CVE 2018-8292 on macOS.

NDK r18 build support.

Crash reporter fixes.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Jan 14, 2019
Fixes: mono/mono#11874
Fixes: mono/mono#11898
Fixes: mono/mono#12093
Fixes: mono/mono#12130
Fixes: mono/mono#12131

Fix race condition in XmlCharType.Instance.

Fix IndexOutOfRangeException in MethodInfo.ReturnParameter.IsDefined

Fix CVE 2018-8292 on macOS.

NDK r18 build support.

Crash reporter fixes.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.