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

Make sure all new GCDacVars are reported only for v2 #90043

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

cshung
Copy link
Member

@cshung cshung commented Aug 4, 2023

In my earlier PR #88457, I missed a few extra fields in the GcDacVars population which should be v2 as well. This is causing a crash on Linux when a .NET 8 clrgc with regions turned on is loaded in a .NET 6 process.

In particular, this line:

gcDacVars->global_free_huge_regions = reinterpret_cast<dac_region_free_list**>(&gc_heap::global_free_huge_regions);

, which should not be run on .NET 6, is accidentally overwriting the variable g_sw_ww_table defined below:

GcDacVars g_gc_dac_vars;
GPTR_IMPL(GcDacVars, g_gcDacGlobals);

#ifdef FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP

uint8_t* g_sw_ww_table = nullptr;
bool g_sw_ww_enabled_for_gc_heap = false;

That leads the write-barrier to write to a bad location and got AV with this output (The user program is a the simple HelloWorld template):

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Collections.Generic.Dictionary`2[[System.ReadOnlyMemory`1[[System.Char, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.ConsoleKeyInfo, System.Console, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].TryInsert(System.ReadOnlyMemory`1<Char>, System.ConsoleKeyInfo, System.Collections.Generic.InsertionBehavior)
   at System.Collections.Generic.Dictionary`2[[System.ReadOnlyMemory`1[[System.Char, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.ConsoleKeyInfo, System.Console, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].set_Item(System.ReadOnlyMemory`1<Char>, System.ConsoleKeyInfo)
   at System.ConsolePal+TerminalFormatStrings..ctor(Database)
   at System.ConsolePal+TerminalFormatStrings+<>c.<.cctor>b__28_0()
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ViaFactory(System.Threading.LazyThreadSafetyMode)
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecutionAndPublication(System.LazyHelper, Boolean)
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CreateValue()
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Value()
   at System.ConsolePal+TerminalFormatStrings.get_Instance()
   at System.ConsolePal.EnsureInitializedCore()
   at System.ConsolePal.Write(Microsoft.Win32.SafeHandles.SafeFileHandle, System.ReadOnlySpan`1<Byte>, Boolean)
   at System.ConsolePal+UnixConsoleStream.Write(System.ReadOnlySpan`1<Byte>)
   at System.IO.StreamWriter.Flush(Boolean, Boolean)
   at System.IO.StreamWriter.WriteLine(System.String)
   at System.IO.TextWriter+SyncTextWriter.WriteLine(System.String)
   at System.Console.WriteLine(System.String)
   at Program.<Main>$(System.String[])
Aborted

@cshung cshung self-assigned this Aug 4, 2023
@ghost
Copy link

ghost commented Aug 4, 2023

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

In my earlier PR #88457, I missed a few extra fields in the GcDacVars population which should be v2 as well. This is causing a crash on Linux when a .NET 8 clrgc with regions turned on is loaded in a .NET 6 process.

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr

Milestone: -

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mrsharm
Copy link
Member

mrsharm commented Aug 4, 2023

Adding setting the GCName as libclrgcexp built from .NET8 Release in .NET6 and .NET7 works successfully on a simple GCPerfSim scenario.

@mangod9
Copy link
Member

mangod9 commented Aug 4, 2023

@mrsharm, can you ascertain whether clrgc with regions was actually working with 6? From looking at the change seems like it should be failing too.

@cshung cshung merged commit 25f2f62 into dotnet:main Aug 4, 2023
107 checks passed
@cshung cshung deleted the public/fix-dac-vars branch August 4, 2023 22:53
@mrsharm
Copy link
Member

mrsharm commented Aug 5, 2023

@mangod9, yes, this seems to be working with libclrgc.so (which, is regions for 6.0).

Steps

  1. Checkout 3b204ac.
  2. Build GCPerfSim for .NET 6.
  3. Create a Corerun using 3b204ac.
  4. export DOTNET_GCName=libclrgc.so
  5. Invoke: ~/runtime/artifacts/tests/coreclr/Linux.x64.Debug/Tests/Core_Root/corerun /home/mrsharm/performance/artifacts/bin/GCPerfSim/Release/net6.0/GCPerfSim.dll -tc 18 -tagb 100 -tlgb 2 -lohar 0 -pohar 0 -sohsr 100-4000 -lohsr 102400-204800 -pohsr 100-204800 -sohsi 50 -lohsi 0 -pohsi 0 -sohpi 0 -lohpi 0 -sohfi 0 -lohfi 0 -pohfi 0 -allocType reference -testKind time
  6. The repro doesn't fail on initialize as before.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants