[Local GC] Move workstation GC DAC globals to a struct shared between GC and DAC #9255

swgillespie · 2017-02-02T00:04:24Z

This PR is a Workstation GC-only implementation of the separation of DAC variables for Local GC. The scheme implemented in this PR is that, upon initialization, the GC populates a struct with pointers to its global variables that are known to the DAC. The DAC can then read this struct and utilize its members to peer in to GC internals using the usual DAC machinery (__DPtr and friends).

I'm sending this review out a bit early in the hopes of getting some feedback on this design before bringing it to Server GC. This PR is mergeable, but still in need of more strict validation than what I have done so far (All GC-related SOS behavior that I can test is working for me on x64, though I am still doing manual testing). I also haven't done any testing of the ICorDebug* interfaces - if we have any tests or validation steps for those, I'd like to run those on this PR.

Not implemented in this PR (yet?) is the logic to detect version mismatches between the DAC and the GC. I'd love to hear any feedback about where this check should go - I didn't identify any particular place where it would fit.

swgillespie · 2017-02-02T01:42:35Z

cc @jkotas, @noahfalk, @Maoni0 thoughts on this, other than the padding problems on non 64-bit platforms? (will address that)

jkotas · 2017-02-02T03:02:58Z

src/gc/gcinterface.dac.h

+ dac_heap_segment* start_segment;
+ uint8_t _padding2[8];
+ uint8_t* allocation_start;
+ uint8_t _padding3[149];


The sizes of these structures; and offsets of the interesting members in them should be in the DAC globals to avoid these magic padding values; (and/or these structures should be re-arranged such that all interesting fields for diagnostic are first or together).

It will also fix the #ifdef DEBUG TODO that you got below.

ok, will do!

How about if the DAC-interesting fields are first, and the size and alignment of the structure are both DAC vars? I think this simplifies the DAC code somewhat.

If you make DAC-interesting field first, the DAC should not need to know about total size or alignment of the structure.

The DAC has to do pointer arithmetic when traversing arrays of these structures, doesn't it need to know both of those things to do that correctly?

Ok, make sense. I have not realized that these come in arrays.

It needs to know the aligned size of structure to do that (one number - what C++ sizeof returns). It does not need to know alignment separately.

Okay, sounds good!

noahfalk · 2017-02-02T05:05:49Z

src/gc/gc.cpp

+ assert(gcDacVars != nullptr);
+ *gcDacVars = {};
+ gcDacVars->major_version_number = &g_gc_dac_major_version;
+ gcDacVars->minor_version_number = &g_gc_dac_minor_version;


I'd suggest putting the version numbers directly in the structure rather than using pointers to a global.

noahfalk · 2017-02-02T05:19:54Z

src/gc/gcinterface.dac.h

+// GC-internal type's fields, while still maintaining the same layout.
+// This interface is strictly versioned, see gcinterface.dacvars.def for more information.
+
+#define NUM_GC_DATA_POINTS 9


Do you want the flexibility to change any of these define values in future local builds of the GC? If so I suggest using a global int to store the value and put it in gcGlobals.

I imagine that flexibility will be good - will do. Everything except for NUMBERGENERATIONS is okay to change without changing any type layouts.

@noahfalk So, unfortunately, these values influence the layout of DacpGCInterestingInfoData: https://github.com/dotnet/coreclr/blob/master/src/inc/dacprivate.h#L787, so we'll need to bump a version number if we alter them anyway.

I looked a little closer at these and we are blurring two different contracts together that we should keep separate. One is a contract between the GC and DAC, the other is a contract between DAC and SOS. DacpGCInterestingInfoData is part of that latter contract. The DAC<->SOS contract is based on COM so the pre-existing interface is immutable and the way we would change it one day is to define a new API that uses a new datatype. There is a second set of #defines for macros of the same name right above the definition of DacpGCInterestingInfoData so that type wouldn't change, but it does lead to significant ambiguity and likely bugs if someone ever changed the value of the define you just added here. For example its not obvious which instance of the define this code now refers to:
https://github.com/dotnet/coreclr/blob/master/src/debug/daccess/request.cpp#L3099.

I'd suggest:
a) Promote these enums to be part of the official GC<->DAC contract, because implicitly they already are: interesting_data_point, gc_heap_compact_reason, gc_heap_expand_mechanism, gc_mechanism_bit_per_heap.
2) Instead of duplicating the existing DAC defines, refer to the pre-existing enum counts in those enums
3) Add static asserts that those enums keep the exact counts they have now, as that is what the DAC is hard-coded to expect.

If we wanted to go further and allow the counts to vary we still could, but it would require changes in both the GC<->DAC interface and the DAC<->SOS interface to get the flexibility all the way through. I defer to you and Maoni to decide if that is worthwhile to do.

noahfalk · 2017-02-02T05:51:05Z

src/gc/gcpriv.h

@@ -781,6 +779,9 @@ class generation
 int gen_num;

 #ifdef FREE_USAGE_STATS
+ #pragma message ("FREE_USAGE_STATS changes the layout of the 'generation' class, which is known to the DAC. " \


You can put the sizeof(generation) into a global and then compute the size dynamically in the DAC if you need to do debugging on builds where FREE_USAGE_STATS is enabled. It does mean however that you can't use sizeof(dac_generation), dac_generation[], pointer arithmetic on dac_generation or anything else where the size of the statically defined structure is implicitly used by the compiler.

Unfortunately dac_generation is the one type in gcinterface.dac.h with which I need to do pointer arithmetic. Jan suggested above that make the size a dacvar anyway and do the pointer math myself in the DAC - it would give some additional flexibility in this case. Do you or @Maoni0 have any thoughts? (I'm not familiar with what this feature define is used for in the GC)

this wouldn't be defined for shipped ret builds for sure. but this could be heavily used during experimenting/giving out private ret builds. since one of the purposes for the LocalGC project is to make giving out private builds easier it would be really nice to make this work.

I am not in office today...can we look at this tomorrow to see how much work this introduces?

Yeah, its still doable, just extra work to do the pointer arithmetic yourself. Other options would be:
a) including the additional fields on generation always but not using them in some builds
b) using an indirection so that you add one pointer to generation always, but then use of that pointer is optional depending on build type. Then add any number of optional fields in the data structure the pointer points to and it won't affect the layout that DAC deals with.

Any of the options are fine from my end, just different tradeoffs of runtime memory overhead in the GC vs implementation complexity.

@Maoni0 Sure!

We should prefer design choices that allow you to make changes in the GC without changing its contract. I would go via the size in dacvar.

implementation complexity.

This should be pretty minor - one or two macros or helper methods should hide the detail.

agreed with Jan. I just wanted to see if it adds a great deal of work to do the arithmetic; it sounds that's not the case so it's a clear choice to do the arithmetic in dac instead of doing things in GC to accommodate this.

will do, then!

noahfalk · 2017-02-02T06:09:07Z

src/vm/ceemain.cpp

- if (!pGCHeap)
+
+ IGCHeap *pGCHeap;
+ if (!InitializeGarbageCollector(gcToClr, &pGCHeap, &g_gc_dac_vars)) 


Small race condition here. If the debugger were to attach while InitializeGarbageCollector was running it would be able to access the global variables in a partially initialized state. I'd suggest making g_gcDacGlobals the DACVAR and g_gc_dac_vars as a standard global structure in the runtime. This way you can assign g_gcDacGlobals = &g_gc_dac_vars after Initialize returns to publish all the pointers to the debugger atomically. It should also mean you don't need to that same assignment in DAC_ENTER way up top.

noahfalk · 2017-02-02T21:40:46Z

Sorry I misused the review feature I think. I did the review yesterday, but in the pending state I don't think anyone else could see it until just now.

jkotas · 2017-02-07T00:16:07Z

src/debug/daccess/gcinterface.dac.h

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#include "../gc/gcinterface.dac.h"


gc is two levels up from here ... I am surprised that it works.

You're right, that is surprising - I guess the GC directory is still on the include path and #include "gcinterface.dac.h" includes the original header and not this one? I'll fix this header.

swgillespie · 2017-02-07T00:27:41Z

I ran into trouble with the Windows linker in the last commit - link.exe was removing g_gcDacGlobals because it's never read from within coreclr.dll. The last commit has a volatile read from it to keep the build green, but is there a better way of doing this in general? I saw https://msdn.microsoft.com/en-us/library/2s3hwbhs.aspx , but I'm wary of adding linker flags for a single symbol. (I also see this, using #pragma comment: https://msdn.microsoft.com/en-us/library/7f0aews7(v=vs.140).aspx, perhaps that works too?)

Other than that, the last few commits should address the feedback I've gotten so far, so I think I'm ready to address any additional feedback and/or apply this to Server GC if there aren't any objections.

Also @noahfalk, are there any automated tests I can run to validate these changes from an SOS and DBI perspective? I've been doing manual validation using SOS so far.

noahfalk · 2017-02-07T00:53:53Z

src/debug/daccess/request.cpp

@@ -737,10 +744,12 @@ ClrDataAccess::GetHeapAllocData(unsigned int count, struct DacpGenerationAllocDa

 if (data && count >= 1)
 {
+ auto table = g_gcDacGlobals->generation_table;
 for (int i=0;i<NUMBERGENERATIONS;i++)


NUMBERGENERATIONS [](start = 27, length = 17)

I may have missed it, but if there isn't already a static_assert(DAC_NUMBERGENERATIONS == NUMBERGENERATIONS) we should add it, this loop appears to depend on that fact.

noahfalk · 2017-02-07T00:58:55Z

src/debug/daccess/request.cpp

 {
 // enumerating the generations from max (which is normally gen2) to max+1 gives you
 // the segment list for all the normal segements plus the large heap segment (max+1)
 // this is the convention in the GC so it is repeated here
- for (ULONG i = GCHeapUtilities::GetMaxGeneration(); i <= GCHeapUtilities::GetMaxGeneration()+1; i++)
+ for (ULONG i = *g_gcDacGlobals->max_gen; i <= *g_gcDacGlobals->max_gen +1; i++)


g_gcDacGlobals->max_gen +1 [](start = 59, length = 26)

In some places we seem to be using NUMBERGENERATIONS and in others we are using the max_gen + 1 calculation. I assume the result is always identical, but this seems like it would be a good opportunity to converge on a canonical way the debugger should calculate the number of generations.

Yes, we should consolidate. Note that NUMBERGENERATIONS is 4 and (max_gen + 1) is 3 so make sure you account for that.

noahfalk · 2017-02-07T01:05:27Z

src/debug/daccess/dacimpl.h

@@ -3904,7 +3904,8 @@ class EnumMethodInstances
 #define DAC_ENTER() \
 EnterCriticalSection(&g_dacCritSec); \
 ClrDataAccess* __prevDacImpl = g_dacImpl; \
- g_dacImpl = this;
+ g_dacImpl = this; \
+ g_gcDacGlobals = &g_gc_dac_vars;


g_gcDacGlobals = &g_gc_dac_vars; [](start = 4, length = 32)

I think you need to remove this line now that g_gcDacGlobals is the published DAC global already. #Closed

Nevermind, this is from an older revision.

In reply to: 99724301 [](ancestors = 99724301)

noahfalk · 2017-02-07T01:06:51Z

noahfalk · 2017-02-07T01:09:37Z

@mikem8361 - Can you let Sean know what he needs to do to run the SOS tests? I'm not sure if there are any docs yet that would help someone not on the diagnostics team know what they need to do?

Maoni0 · 2017-02-07T03:03:59Z

src/gc/gcinterface.dac.h

+// from the debugee process to the debugger process when dereferenced.
+struct GcDacVars {
+ uint8_t major_version_number;
+ uint8_t minor_version_number;


Please add a comment that explains what warrants an increment of major and minor version numbers.

There's a comment of that sort here: https://github.com/swgillespie/coreclr/blob/gc-dac/src/gc/gcinterface.dacvars.def#L8 but it needs to be revised now that we can tolerate some changes in layout. I'll update it.

I see. My apologies.

mikem8361 · 2017-02-10T20:28:19Z

The SOS tests are part of the debuggertests repo. We have some docs on building and running (see below). 1) Get permission to the debuggertests repo (I’ve already given Sean permission). 2) Either create a fork of https://github.com/dotnet/debuggertests and clone that or clone it directly on your Windows machine 3) see https://github.com/dotnet/debuggertests/blob/master/Documentation/Engineering/BuildAndTestWindows.md 4) One thing the directions don’t cover is building for Linux or OSX. You still need to build the managed binaries on Windows with “build linux” or “build osx”, but you also need to build the native bits on the target platform with “./build.sh skipmanaged”. 5) Before you run the tests, copy the coreclr binaries with your changes (need to be close to the latest master branch) to “debuggertests\bin\<platform>\Runtimes\Coreclr1” where platform is something like Windows.x64.Debug or Linux.x64.Debug.

swgillespie · 2017-02-11T01:29:45Z

@mikem8361 Thank you!

swgillespie · 2017-02-13T19:58:40Z

An update on this PR: I have a few debugger test failures that I am in the process of fixing, but I have code in place that implements this scheme for Workstation and Server GC: swgillespie/coreclr@gc-dac...swgillespie:gc-dac-staging . Once I have all debugger tests passing, I will merge my PR staging branch into this branch and will be ready for a more formal review. (There's still some cleanup that I need to do before I'm ready to submit it for review.)

I have a couple questions, though (cc @mikem8361):

How can I test the changes that affect the DBI? Do the debugger tests exercise it? I have made a few changes to DBI code paths but I suspect that they are incorrect. (edit: never mind, I see my failing debuggertests are hitting my incorrect code path)
Do the debugger tests cover crash dump scenarios? (Or, is there an automated test suite that I can run to ensure that my crash dump memory enumeration logic is correct?)

mikem8361 · 2017-02-13T22:00:53Z

Yes, the debuggertests repo tests DBI (kind of the whole point of most of the tests). There is no crash (mini-)dump coverage yet. You will probably need to install dotnet/coreclr on Windows with the MSI (the only way coreclr is registered with WER), replace the binaries with yours and cause a crash. WER should call the DAC enum memory region apis to create the dump.

swgillespie · 2017-02-13T22:12:38Z

Thanks!

Yes, the debuggertests repo tests DBI (kind of the whole point of most of the tests).

Yeah, sorry - I was under the impression that SOS didn't use the DBI, but apparently it does. I never encountered it when manual testing with SOS.

You will probably need to install dotnet/coreclr on Windows with the MSI

Does .dump in windbg call the same APIs that WER does (or, can I use that instead)? I was troubleshooting a crash dump scenario once on ProjectN and I used .dump instead of WER to test the DAC enum memory region APIs - is that not sufficient here? (I'll test it anyway, just curious)

the GC and the DAC

…gging on OSX

1. Make g_gcDacGlobals a pointer and dacvar on the VM side, so that publishing the GC dac vars is done atomically (through a pointer assignment). This fixes a race that Noah noticed. 2. Remove the requirement for the GC's generation class struct to be known at compile-time, by using a dacvar as the size of the generation class at run-time (for pointer arithmetic) 3. Move all DAC-interesting fields to be at the start of GC internal classes, so that the DAC does not need to know the size or exact layout of the class past the fields it cares about.

… and GC/DAC interfaces, and add static asserts that they are the same

… GC dac vars

…g the EE store a copy of the global during initialization

jkotas · 2017-02-15T05:22:31Z

I did this change: 573f61a in the spirit of what has already been done with

I do not think that this change is functionally correct because of you have changed the variable from array to pointer to an array, and the assembly helpers still operate as if it was an array. Have you verified that it actually works? (e.g. by forcing this codepath; or by running on single proc VM)

Also, it does not look right that this information is passed via WriteBarrier parameters because of it has nothing to do with write barrier parameters. It should be better done as a flavor of FixAllocContext instead.

mikem8361 · 2017-02-15T17:37:28Z

I think the debugger tests are failing because I forgot to tell you to delete ildasm and ilasm after you copy the coreclr binaries.

mikem8361 · 2017-02-15T17:52:13Z

Yeah, sorry - I was under the impression that SOS didn't use the DBI, but apparently it does. I never >>encountered it when manual testing with SOS.

You will probably need to install dotnet/coreclr on Windows with the MSI

Does .dump in windbg call the same APIs that WER does (or, can I use that instead)? I was troubleshooting a crash dump scenario once on ProjectN and I used .dump instead of WER to test the DAC enum memory region APIs - is that not sufficient here? (I'll test it anyway, just curious)

"!ClrStack -i" uses DBI, but I don't think any other commands do.

I didn't know that .dump in windbg calls the DAC enum memory APIs. I just coming up to speed on these APIs currently myself. It sounds like .dump should be sufficient for testing.

swgillespie · 2017-02-15T18:23:59Z

@jkotas

Have you verified that it actually works? (e.g. by forcing this codepath; or by running on single proc VM)

No - will do that today.

Also, it does not look right that this information is passed via WriteBarrier parameters because of it has nothing to do with write barrier parameters. It should be better done as a flavor of FixAllocContext instead.

Will do.

@mikem8361

Thanks! Trying that now.

swgillespie · 2017-02-15T22:37:35Z

@jkotas (and @Maoni0) I think I may back out of the generation_table change in this PR so I can finish up this PR and deal with generation_table in a follow-up PR, since it's pretty orthogonal to the DAC. Is that reasonable? (I only discovered it because I attempted to move generation_table to be a static on the gc_heap class)

…by having the EE store a copy of the global during initialization" This reverts commit 573f61a.

This reverts commit 188c22d.

… a single-proc allocation helper

jkotas · 2017-02-16T02:44:00Z

back out of the generation_table change in this PR so I can finish up this PR and deal with generation_table in a follow-up PR

Agree.

swgillespie · 2017-02-17T02:31:10Z

I found an explanation for my two failing SOS tests (https://github.com/dotnet/debuggertests/issues/253) and those are now passing, so now I am down to one failing debugger test. The test (https://github.com/dotnet/debuggertests/blob/master/tests/Exceptions/Debugees/TaskCancel/taskcancel.cs#L35) fails in this manner:

    00:25.563: Error: Line comparison failure
    00:25.564:  Command: go 
    00:25.565:  Command Number: 17
    00:25.566: RAW Expected:    '#STDOUT | Exception=System.AggregateException'
    00:25.568: RAW Actual:      '#STDOUT | Exception=System.UnauthorizedAccessException'
    00:25.569: Compared Expected Output:    'Exception=System.AggregateException'
    00:25.571: Compared Actual Output:      'Exception=System.UnauthorizedAccessException'
    00:25.573: Expected Command Output-Start
    00:25.574: 1:#STDOUT | Exception2 Event Occurred
    00:25.575: 2:#STDOUT | CorException2EventArgs:
    00:25.577: 3:#STDOUT | DEBUG_EXCEPTION_FIRST_CHANCE
    00:25.578: 4:#STDOUT | STOP: Exception thrown
    00:25.579: 5:#STDOUT | Exception=System.AggregateException
    00:25.581: 6:#REGEXP | m_innerExceptions=.*<System.Exception>
    00:25.583: 7:#MULTIL | .*_innerException=System.UnauthorizedAccessException
    00:25.585: 8:#MULTIL | .*EventType: DEBUG_EXCEPTION_FIRST_CHANCE
    00:25.586: 9:#IGNORE | Intercept: False
    00:25.587: 10:#IGNORE | located at line 1913 in Task.cs
    00:25.589: Expected Command Output-End
    00:25.590: Expected Command Output is empty
    00:25.591: Actual Command Output-Start
    00:25.592: 1:#STDOUT | Exception2 Event Occurred
    00:25.594: 2:#STDOUT | CorException2EventArgs:
    00:25.595: 3:#STDOUT | DEBUG_EXCEPTION_FIRST_CHANCE
    00:25.597: 4:#STDOUT | STOP: Exception thrown
    00:25.598: 5:#STDOUT | Exception=System.UnauthorizedAccessException
    00:25.599: 6:#STDOUT | s_EDILock=<null>
    00:25.601: 7:#STDOUT | _className=<null>
    00:25.602: 8:#STDOUT | _exceptionMethod=<null>
    00:25.603: 9:#IGNORE | _exceptionMethodString=<null>
    00:25.604: 10:#STDOUT | _message="Application Error Message"
    00:25.605: 11:#STDOUT | _data=<null>
    00:25.606: 12:#STDOUT | _innerException=<null>
    00:25.608: 13:#STDOUT | _helpURL=<null>
    00:25.609: 14:#IGNORE | _stackTrace=array [192]
    00:25.610: 15:#STDOUT | _watsonBuckets=<null>
    00:25.611: 16:#STDOUT | _stackTraceString=<null>
    00:25.613: 17:#IGNORE | _remoteStackTraceString=<null>
    00:25.614: 18:#IGNORE | _remoteStackIndex=0
    00:25.616: 19:#STDOUT | _dynamicMethods=<null>
    00:25.617: 20:#STDOUT | _HResult=-2147024891
    00:25.618: 21:#STDOUT | _source=<null>
    00:25.619: 22:#IGNORE | _xptrs=0
    00:25.621: 23:#IGNORE | _xcode=-532462766
    00:25.621: 24:#STDOUT | _ipForWatsonBuckets=140719758183897
    00:25.623: 25:#STDOUT | Offset:    79
    00:25.624: 26:#STDOUT | EventType: DEBUG_EXCEPTION_FIRST_CHANCE
    00:25.626: 27:#STDOUT | Intercept: True
    00:25.627: 28:#STDOUT | IP: 79 @ System.Threading.ExecutionContext.Run - MAPPING_APPROXIMATE
    00:25.629: Actual Command Output-End
    00:25.631: Error: Command Failed::Quitting MDBG
    00:25.737: Terminating current process...
    00:26.069: RECORDER: Next scheduled replay command (17): go

I am not sure how this is related to my changes - does anyone have any thoughts? This is the last remaining functional issue for this PR.

noahfalk · 2017-02-17T03:13:06Z

Does it fail if you run it on a runtime that doesn't have your changes? We can certainly help you debug into it, but if it repros without your change then you won't need to worry about it.

swgillespie · 2017-02-17T03:54:18Z

@noahfalk It looks like it does, I just did a test with the tip of master and it still failed for me. It doesn't fail when using the coreclr package it obtains through build.cmd restore, though.

noahfalk · 2017-02-17T04:12:05Z

@swgillespie - I think you can safely ignore it then - thanks for checking!

mikem8361 · 2017-02-17T16:55:00Z

It looks like the exception we are expecting isn’t wrapped in an AggregateException anymore. It sounds like this only happens if you use a newer version (master) of the runtime and has nothing to do with your changes.

mikem8361 · 2017-02-17T19:52:43Z

The changes look good to me.

swgillespie · 2017-02-21T17:10:50Z

Thanks Mike!

If there aren't any other objections, I think this is ready to merge - I believe I've addressed all the feedback in this thread.

swgillespie · 2017-02-24T18:12:33Z

thanks!

… GC and DAC (dotnet#9255) * [Local GC] Move workstation GC DAC globals to a struct shared between the GC and the DAC * (Some) code review feedback and bug fixes for issues found while debugging on OSX * Address some code review feedback: 1. Make g_gcDacGlobals a pointer and dacvar on the VM side, so that publishing the GC dac vars is done atomically (through a pointer assignment). This fixes a race that Noah noticed. 2. Remove the requirement for the GC's generation class struct to be known at compile-time, by using a dacvar as the size of the generation class at run-time (for pointer arithmetic) 3. Move all DAC-interesting fields to be at the start of GC internal classes, so that the DAC does not need to know the size or exact layout of the class past the fields it cares about. * Split the definition of the size of several arrays across the SOS/DAC and GC/DAC interfaces, and add static asserts that they are the same * Repair the Windows Release build * Implement the GC DAC scheme for Server GC and eliminate the duplicate GC dac vars * Some work * Decouple use of the GC generation table from a write barrier by having the EE store a copy of the global during initialization * Actually make it work with server GC * Checkpoint * Checkpoint where everything works * Code cleanup * Fix debugger test failures * Additional code cleanup * Address code review feedback by adding a static assert and standardizing the way that we iterate over the generation table * Repair the Windows x86 build * Revert "Decouple use of the GC generation table from a write barrier by having the EE store a copy of the global during initialization" This reverts commit 573f61a. * Revert "Repair the Windows x86 build" This reverts commit 188c22d. * Partial revert, move `generation_table` back the global namespace for a single-proc allocation helper * Fix a debugger test failure * Repair crash dump scenarios

… GC and DAC (dotnet/coreclr#9255) * [Local GC] Move workstation GC DAC globals to a struct shared between the GC and the DAC * (Some) code review feedback and bug fixes for issues found while debugging on OSX * Address some code review feedback: 1. Make g_gcDacGlobals a pointer and dacvar on the VM side, so that publishing the GC dac vars is done atomically (through a pointer assignment). This fixes a race that Noah noticed. 2. Remove the requirement for the GC's generation class struct to be known at compile-time, by using a dacvar as the size of the generation class at run-time (for pointer arithmetic) 3. Move all DAC-interesting fields to be at the start of GC internal classes, so that the DAC does not need to know the size or exact layout of the class past the fields it cares about. * Split the definition of the size of several arrays across the SOS/DAC and GC/DAC interfaces, and add static asserts that they are the same * Repair the Windows Release build * Implement the GC DAC scheme for Server GC and eliminate the duplicate GC dac vars * Some work * Decouple use of the GC generation table from a write barrier by having the EE store a copy of the global during initialization * Actually make it work with server GC * Checkpoint * Checkpoint where everything works * Code cleanup * Fix debugger test failures * Additional code cleanup * Address code review feedback by adding a static assert and standardizing the way that we iterate over the generation table * Repair the Windows x86 build * Revert "Decouple use of the GC generation table from a write barrier by having the EE store a copy of the global during initialization" This reverts commit dotnet/coreclr@573f61a. * Revert "Repair the Windows x86 build" This reverts commit dotnet/coreclr@188c22d. * Partial revert, move `generation_table` back the global namespace for a single-proc allocation helper * Fix a debugger test failure * Repair crash dump scenarios Commit migrated from dotnet/coreclr@6f6fda9

swgillespie added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 2, 2017

dnfclas added the cla-already-signed label Feb 2, 2017

jkotas reviewed Feb 2, 2017

View reviewed changes

noahfalk reviewed Feb 2, 2017

View reviewed changes

jkotas reviewed Feb 7, 2017

View reviewed changes

noahfalk reviewed Feb 7, 2017

View reviewed changes

Maoni0 reviewed Feb 7, 2017

View reviewed changes

swgillespie added 11 commits February 14, 2017 11:20

[Local GC] Move workstation GC DAC globals to a struct shared between

c27774b

the GC and the DAC

(Some) code review feedback and bug fixes for issues found while debu…

3928d10

…gging on OSX

Split the definition of the size of several arrays across the SOS/DAC…

c072a72

… and GC/DAC interfaces, and add static asserts that they are the same

Repair the Windows Release build

bdc2421

Implement the GC DAC scheme for Server GC and eliminate the duplicate…

c356852

… GC dac vars

Some work

2be9e5f

Decouple use of the GC generation table from a write barrier by havin…

573f61a

…g the EE store a copy of the global during initialization

Actually make it work with server GC

2971465

Checkpoint

9cf3401

Checkpoint where everything works

e0a481e

swgillespie added 4 commits February 15, 2017 14:38

Revert "Decouple use of the GC generation table from a write barrier …

9452853

…by having the EE store a copy of the global during initialization" This reverts commit 573f61a.

Revert "Repair the Windows x86 build"

ddc20b5

This reverts commit 188c22d.

Partial revert, move generation_table back the global namespace for…

31c8d51

… a single-proc allocation helper

Fix a debugger test failure

9904c3d

Repair crash dump scenarios

12f8b9f

swgillespie removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 17, 2017

swgillespie changed the title ~~[WIP, Local GC] Move workstation GC DAC globals to a struct shared between GC and DAC~~ [Local GC] Move workstation GC DAC globals to a struct shared between GC and DAC Feb 17, 2017

swgillespie merged commit 6f6fda9 into dotnet:master Feb 24, 2017

swgillespie mentioned this pull request Feb 27, 2017

[Local GC] Decouple the EE's usage of the generation table from the GC #9836

Closed

swgillespie mentioned this pull request Mar 21, 2017

[Local GC] Forbid inclusion of gcscan.h from VM and DAC directories #10332

Merged

swgillespie mentioned this pull request Jun 8, 2017

[Local GC] No longer need to initialize SVR dacvars separately #12173

Merged

swgillespie mentioned this pull request Jul 11, 2017

[Local GC] Refactor usage of GC-internal functions in the DAC #12458

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

[Local GC] Move workstation GC DAC globals to a struct shared between GC and DAC #9255

[Local GC] Move workstation GC DAC globals to a struct shared between GC and DAC #9255

Conversation

swgillespie commented Feb 2, 2017 • edited Loading

swgillespie commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

swgillespie Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maoni0 Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk commented Feb 7, 2017

noahfalk commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikem8361 commented Feb 10, 2017 via email

swgillespie commented Feb 11, 2017

swgillespie commented Feb 13, 2017 • edited Loading

mikem8361 commented Feb 13, 2017 via email

swgillespie commented Feb 13, 2017

jkotas commented Feb 15, 2017 • edited Loading

mikem8361 commented Feb 15, 2017 via email

mikem8361 commented Feb 15, 2017

swgillespie commented Feb 15, 2017

swgillespie commented Feb 15, 2017

jkotas commented Feb 16, 2017

swgillespie commented Feb 17, 2017

noahfalk commented Feb 17, 2017

swgillespie commented Feb 17, 2017

noahfalk commented Feb 17, 2017

mikem8361 commented Feb 17, 2017 via email

mikem8361 commented Feb 17, 2017

swgillespie commented Feb 21, 2017

swgillespie commented Feb 24, 2017

swgillespie commented Feb 2, 2017 •

edited

Loading

swgillespie Feb 2, 2017 •

edited

Loading

swgillespie Feb 3, 2017 •

edited

Loading

Maoni0 Feb 2, 2017 •

edited

Loading

noahfalk Feb 7, 2017 •

edited

Loading

swgillespie commented Feb 13, 2017 •

edited

Loading

jkotas commented Feb 15, 2017 •

edited

Loading