MonoTests.System.Reflection.Emit.SaveTest.Save test fails in Release configuration with BCL linking on

[grendello]

A test from the Mono BCL test suite (MonoTests.System.Reflection.Emit.SaveTest.Save) fails when the tests are built in Release mode with linking set to SdkOnly.
It seems that the linker somehow changes the order of interfaces in emitted code since turning BCL linking off makes the test work again. The test also works fine in Debug configuration (with the linker off)

Steps to Reproduce

1. Comment out this line
2. Run make CONFIGURATION=Release run-apk-tests from the top Xamarin.Android dir
3. Wait patiently
4. Find the failure in the NUnit tests result file (TestResult-Xamarin.Android.Bcl_Tests.nunit-Release.xml in the top Xamarin.Android directory)

Expected Behavior

The test should run fine with BCL linking on

Actual Behavior

The test fails when executed with BCL linking on

Version Information

Xamarin.Android/master (c4e81655ac22b06806e732e9dd0bc729e8d2c1d7)

Test failure

MESSAGE:
  Expected: <iface1>
  But was:  <System.IComparable>

  +++++++++++++++++++
  STACK TRACE:
  at MonoTests.System.Reflection.Emit.SaveTest.CheckAssembly (System.Reflection.Assembly a) [0x000a0] in <1e30aff9165c427db5bedc42c5906861>:0 
  at MonoTests.System.Reflection.Emit.SaveTest.Save () [0x00b6d] in <1e30aff9165c427db5bedc42c5906861>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <0a3e31ed05774b07957e0b91c5cab789>:0

[radekdoulik]

This doesn't look like a linker bug, as the assembly is created by the test itself, not by the linker.

[jonpryor]

@radekdoulik: While that is indeed the case, it's only with a linked BCL that the problem occurs. Running the test with a Debug/unlinked BCL results in the test passing.

We don't understand why or how linking would be involved, but it's the straw we're grasping at.

[radekdoulik]

Further looking into that, I have put the code from the test to simple android app and it works OK in the Release configuration. The first interface is iface1.

So not sure yet what might be broken here.

[radekdoulik]

OK, the issue is the test is wrong :-)

In the documentation it is clearly stated, that the interfaces are not returned in any particular order. https://msdn.microsoft.com/en-us/library/system.type.getinterfaces(v=vs.110).aspx#Anchor_2

[grendello]

@radekdoulik yes, it's not documented but @marek-safar stated that the runtime team aim for 100% compatibility with .NET, and the ordering tested for is what .NET does. As @jonpryor said, we're clutching at straws here as the linking was the only real difference between the builds and it is consistently reproducible.

[radekdoulik]

@grendello not sure I understand you here, when you say that it is not documented . The documentation says

The GetInterfaces method does not return interfaces in a particular order, such as alphabetical or declaration order. Your code must not depend on the order in which interfaces are returned, because that order varies.

[radekdoulik]

@marek-safar Do you have link to our current System.Type::GetInterfaces implementation at hand?

[grendello]

@radekdoulik By 'not documented' I meant it isn't stated that the order must be kept. The wording you pasted states that it 'must not' depend on the order, which would make the test invalid, but as @marek-safar stated we want to keep compatibility with .NET as there might be code out there which relies on the particular behavior of .NET (even if it isn't a documented one)

[migueldeicaza]

You might just be getting lucky with some idioms/patterns in .NET, and might change with others, or might change across other implementations. The documentation is right, we should fix the test.

[jonpryor]

On the implementation side of things, it appears that RuntimeType.GetInterfaces() is implemented as ves_icall_RuntimeType_GetInterfaces() which uses a GHashTable to contain the enumerated interfaces.

I'm going to hazard a guess that GHashTable/g_hash_table_insert() doesn't preserve insertion order, which means the docs are correct and should be followed, because there's no easy way of fixing the implementation, just the tests.

[marek-safar]

I'm ok with the test fix but we have bug reports like https://bugzilla.xamarin.com/show_bug.cgi?id=31020 and similar so you could actually end up fixing this issue in XA for real