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

Enable libraries testing #900

Merged
merged 25 commits into from
Apr 29, 2021

Conversation

MichalStrehovsky
Copy link
Member

No description provided.

@MichalStrehovsky MichalStrehovsky added the area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation label Apr 1, 2021
@MichalStrehovsky
Copy link
Member Author

So the System.Runtime tests are hitting an INT 3 with this stack:

>	[Inline Frame] System.Runtime.Tests.exe!WKS::exclusive_sync::check() Line 1124	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::background_mark_phase() Line 30681	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::gc1() Line 18893	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::bgc_thread_function() Line 31740	C++
 	System.Runtime.Tests.exe!GCToEEInterface::CreateThread::__l2::<lambda>(void * argument) Line 1383	C++

The FileSystem tests are not crashing for me, but they also just hang and don't finish.

I've only been testing unoptimized builds locally in the past.

So for now, removing these tests.

@MichalStrehovsky
Copy link
Member Author

Okay, this still needs to wait for dotnet/runtime#50898 and dotnet/runtime#50280 at minimum and it still managed to crash despite the turned off optimizations.

On Unix we have some ICU or whatever failures.

And I need to block a couple more tests.

This will never end.

@MichalStrehovsky
Copy link
Member Author

So I at least spent some time looking at the hang. We have a bunch of threads waiting for background GC, waiting for suspension, waiting for the GC to complete.

The only two threads that are doing something are the thread trying to do suspension:

>	System.Runtime.Tests.exe!ThreadStore::SuspendAllThreads(bool waitForGCEvent) Line 240	C++
 	System.Runtime.Tests.exe!GCToEEInterface::SuspendEE(SUSPEND_REASON reason) Line 807	C++
 	System.Runtime.Tests.exe!WKS::GCHeap::GarbageCollectGeneration(unsigned int gen, gc_reason reason) Line 42146	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::trigger_gc_for_alloc(int gen_number, gc_reason gr, WKS::GCDebugSpinLock * msl, bool loh_p, WKS::msl_take_state take_state) Line 15740	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::try_allocate_more_space(alloc_context * acontext, unsigned __int64 size, unsigned int flags, int gen_number) Line 15858	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::allocate_more_space(alloc_context * acontext, unsigned __int64 size, unsigned int flags, int alloc_generation_number) Line 16354	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::allocate(unsigned __int64 jsize, alloc_context * acontext, unsigned int flags) Line 16385	C++
 	System.Runtime.Tests.exe!WKS::GCHeap::Alloc(gc_alloc_context * context, unsigned __int64 size, unsigned int flags) Line 41158	C++

But that one cannot make progress because the background GC thread cannot be suspended (we're hitting the codepath in PalGetThreadContext that checks whether the context is accurate and it isn't. But even if we were to succeed there, the background GC thread stack is not walkable so it doesn't help.

The background GC thread is stuck in


>	System.Runtime.Tests.exe!WKS::exclusive_sync::bgc_mark_set(unsigned char * obj) Line 1145	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::revisit_written_page(unsigned char * page, unsigned char * end, int concurrent_p, unsigned char * & last_page, unsigned char * & last_object, int large_objects_p, unsigned __int64 & num_marked_objects) Line 31044	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::revisit_written_pages(int concurrent_p, int reset_only_p) Line 31290	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::background_mark_phase() Line 30597	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::gc1() Line 18891	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::bgc_thread_function() Line 31740	C++
 	System.Runtime.Tests.exe!WKS::gc_heap::bgc_thread_stub(void * arg) Line 29752	C++
 	System.Runtime.Tests.exe!GCToEEInterface::CreateThread::__l2::<lambda>(void * argument) Line 1382	C++

We just keep going to the retry loop because nobody is touching alloc_objects and removing the thing we're checking for because all the other threads are waiting.

I thought that maybe this is caused by not having FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP because I saw code around this that checks for that (I assume nobody tests a GC without this defined over in the CoreCLR repo), but even with that and FEATURE_MANUALLY_MANAGED_CARD_BUNDLES, I don't see improvements.

@@ -246,7 +246,7 @@ private static Version GetICUVersion()
int version = 0;
try
{
Type interopGlobalization = Type.GetType("Interop+Globalization");
Type interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Interop type exist both in the current assembly and in CoreLib. Apparently we're supposed to try in both. The reflection stack is not structured to fix this in straighforward way. I just really don't care.

@@ -917,6 +918,7 @@ long CallGetTotalAllocatedBytes(long previous)
}
}

[ActiveIssue("https://github.com/dotnet/runtimelab/issues/939" /* NativeAot */)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixed now

@@ -81,6 +81,9 @@ public void CustomAttributes()
[Fact]
public void FullyQualifiedName()
{
#if SINGLE_FILE_TEST_RUNNER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not sound right for the test to contain ifdefs like this. It should be a dynamic check instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already merged with dotnet/runtime#50996 - I cherry picked it because I'm tired of working in two repos.

We don't have a way to build a dynamic check that wouldn't just result in testing a tautology ("on a runtime that returns <Unknown> for FullName, check that FullName returns <Unknown>").

@@ -106,7 +106,7 @@ stages:
isOfficialBuild: ${{ variables.isOfficialBuild }}
timeoutInMinutes: 90
testGroup: innerloop
buildArgs: -s nativeaot.objwriter+nativeaot+libs+nativeaot.packages -c release
buildArgs: -s nativeaot+libs+nativeaot.packages -c release
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These shortcuts should be reverted now that the PR is passing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is passing, but it shouldn't. It has failing tests.

<!-- xunit calls MakeGenericType to check if something is IEquatable -->
<IlcArg Include="--feature:System.Reflection.IsTypeConstructionEagerlyValidated=false" />

<!-- Disable background GC because we have issues with it - https://github.com/dotnet/runtimelab/issues/940 -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed anymore

@MichalStrehovsky
Copy link
Member Author

OK, this is ready for review now!

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit a957342 into dotnet:feature/NativeAOT Apr 29, 2021
@MichalStrehovsky MichalStrehovsky deleted the fxtests branch April 29, 2021 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants