Skip to content

Make sure ServerGCHeapDetails is up to date - #56056

Merged
cshung merged 4 commits into
dotnet:mainfrom
cshung:public/server-dac
Aug 2, 2021
Merged

Make sure ServerGCHeapDetails is up to date#56056
cshung merged 4 commits into
dotnet:mainfrom
cshung:public/server-dac

Conversation

@cshung

@cshungcshung commented Jul 20, 2021

Copy link
Copy Markdown
Contributor

Fixes#54368

In order to make sure the debugger does the correct thing for both workstation and server GC, we need to make sure when it load fields, it loads the same set of fields, and they load the correct value.

These are all fields are defined on the gc_heap class, and they were defined as such:

PER_HEAP card_table was defined unconditionally.
PER_HEAP mark_array was defined under BACKGROUND_GC.
PER_HEAP next_sweep_obj was defined under BACKGROUND_GC.
PER_HEAP background_saved_lowest_address was defined under BACKGROUND_GC.
PER_HEAP background_saved_highest_address was defined under BACKGROUND_GC.
PER_HEAP saved_sweep_ephemeral_seg was defined under !USE_REGIONS, BACKGROUND_GC.
PER_HEAP saved_sweep_ephemeral_start was defined under !USE_REGIONS, BACKGROUND_GC.

PER_HEAP means it is static for work station and instance for server.

In .NET 6, we planned to use clrgc.dll for USE_REGIONS and coreclr.dll for !USE_REGIONS. Therefore we will have a different layout of the gc_heap class depending on whether or not we use the standalone GC. To accommodate this situation, the loading of the dac_gc_heap class is now metadata-driven. Instead of assuming the layout of dac_gc_heap matches gc_heap, the GC will export an array of field offsets, and the DAC will load the fields according to the offsets.

@dotnet/dotnet-diag

@ghost

Copy link
Copy Markdown

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

Issue Details

Fixes #54368

@dotnet/dotnet-diag

Author:cshung
Assignees:-
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/server-dac branch 3 times, most recently from dd0fd7b to d4be21fCompareJuly 22, 2021 22:42
@cshungcshung changed the title [WIP] Make sure ServerGCHeapDetails is up to dateMake sure ServerGCHeapDetails is up to dateJul 24, 2021
Comment threadsrc/coreclr/gc/gcpriv.h Outdated
@cshung
cshungforce-pushed the public/server-dac branch from e96bf57 to 0516d9bCompareJuly 28, 2021 20:54
@cshung
cshungforce-pushed the public/server-dac branch from 5cfd25b to 2e58dc6CompareJuly 29, 2021 19:27
@Maoni0
Maoni0 requested a review from noahfalkJuly 30, 2021 04:49
@Maoni0

Copy link
Copy Markdown
Member

@noahfalk PTAL.

@cshung I see what you meant by having to write the same class 3 times but I have no better idea how you could get around that. maybe @noahfalk knows a way?

@noahfalk

Copy link
Copy Markdown
Member

@cshung I see what you meant by having to write the same class 3 times but I have no better idea how you could get around that. maybe @noahfalk knows a way?

There is a macro technique we use elsewhere in runtime code that may be helpful here. In one file we define a bunch of data as the arguments to macros and then we include that file in different places with different definitions of what the macros mean. For example the data might be:

//gc_typefields.hDEFINE_FIELD(gc_heap, uint8_t*, uint8_t*, alloc_allocated)
DEFINE_FIELD_DPTR(gc_heap, heap_segment, dac_heap_segment, ephemeral_heap_segment)
...
#ifdef BACKGROUND_GC_FIELDS
DEFINE_FIELD(gc_heap, uint32_t*, uint32_t*, mark_array)
...
#elseDEFINE_MISSING_FIELD(gc_heap, uint32_t*, uint32_t*, mark_array)
...
#endif

Then DAC might include like this:

int i = 0;
#defineBACKGROUND_GC_FIELDS
#defineDEFINE_FIELD(typename, runtimeFieldType, dacFieldType, fieldName) LOAD(fieldName, i, dacFieldType); i++;
#defineDEFINE_FIELD_DPTR(typename, runtimeFieldType, dacFieldType, fieldName) LOAD_DPTR(fieldName, i, dacFieldType); i++;
#include"gc_typefields.h"

And runtime layout generator might include like this:

#ifdef BACKGROUND_GC
#defineBACKGROUND_GC_FIELDS
#endif
#defineDEFINE_FIELD(typename, runtimeFieldType, dacFieldType, fieldName) offsetof(runtimeType, runtimeFieldType),
#defineDEFINE_FIELD_DPTR(typename, runtimeFieldType, dacFieldType, fieldName) offsetof(runtimeType, runtimeFieldType),
#defineDEFINE_MISSING_FIELD(typename, runtimeType, runtimeFieldType, dacFieldType, fieldName) -1,
staticint gc_heap_field_offsets[] = {
#include"gc_typefields.h"
};

What I wrote above probably needs some adjustment to make it work, but hopefully it conveys the idea.
A complete working example of this pattern in the runtime is corelib.h and an inclusion in binder.h. Like most macro things it can be a little harder read/debug in exchange for letting you consolidate different codegen from a single common definition of the type information. Its entirely your call whether you find it appealing or you prefer to manually keep the different code pieces in sync.

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Modulo the few commented bits it looked good to me : )

Comment threadsrc/coreclr/debug/daccess/request_common.h
Comment threadsrc/coreclr/debug/daccess/request_common.h Outdated
Comment threadsrc/coreclr/debug/daccess/request_svr.cpp Outdated
Comment threadsrc/coreclr/debug/daccess/request_svr.cpp Outdated
Comment threadsrc/coreclr/debug/daccess/request.cpp
@noahfalk

Copy link
Copy Markdown
Member

The more interesting part is the generation_table, we will only fill 4 generations, and there is nothing we can do about that since DacpGcHeapDetails cannot be changed.

A change there would require changing SOS, which is certainly doable, but entirely reasonable to be out-of-scope for this PR. @davmason - do you know if only supporting 4 generations in the DacpGcHeapDetails is preventing SOS from properly supporting pinned object heap or it turned out not to be relevant?

@davmason

Copy link
Copy Markdown
Contributor

A change there would require changing SOS, which is certainly doable, but entirely reasonable to be out-of-scope for this PR. @davmason - do you know if only supporting 4 generations in the DacpGcHeapDetails is preventing SOS from properly supporting pinned object heap or it turned out not to be relevant?

I ended up adding new APIs to be able to support POH: #37853

@cshung
cshung merged commit f74c14f into dotnet:mainAug 2, 2021
@cshung
cshung deleted the public/server-dac branch August 2, 2021 23:39
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
* origin/main: (64 commits)
[wasm][debugger] Create test Inherited Properties (dotnet#56754)
Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
[mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
Update windows-requirements.md (dotnet#56476)
Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
[wasm][debugger] Inspect static class (dotnet#56740)
Fix stack overflow handling issue in GC stress (dotnet#56733)
Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
Move Windows Compat pack to NuGet pack task (dotnet#56686)
Fix build error when building some packages (dotnet#56767)
Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
Fix race in crossdac publishing with PGO (dotnet#56762)
Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
Use ComWrappers in some Marshal unit-tests and update platform metadata (dotnet#56595)
Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
Make sure ServerGCHeapDetails is up to date (dotnet#56056)
[libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
Disable failing arm64 win10 Graphics.FromHdc tests (dotnet#56732)
Match xplat event source conditions (dotnet#56435)
...
@ghostghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for freeto 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.

Incomplete loading of DacpGcHeapDetails in ClrDataAccess::ServerGCHeapDetails

5 participants

@cshung@Maoni0@noahfalk@davmason@hoyosjs