[local gc] disable feature redhawk #17769

davmason · 2018-04-25T07:43:02Z

Changes I intend to be permanent for local gc:

Turned off HandleTable shouldinjectfault for local gc
Included redhawk versions of validate, promote, etc for standalone gc
objecthandle.cpp - turned off check for kEtwGCRootFlagsWeakRef-
objecthandle.cpp - deleted newholder, it was just one instance
Copied ClrSafeInt to local gc

Changes I intend to be temporary and fixed in future issues

Disabled AnalyzeSurvivorsRequested and DACNotifyGcMarkEnd, they used GCNotifications which needs to be changed to use in local gc
Disabled NumaNodeInfo

davmason · 2018-04-25T07:47:04Z

Opened #17770 for numanodeinfo

davmason · 2018-04-25T07:48:46Z

Opened #17771 for AnalyzeSurvivorsRequested and DACNotifyGcMarkEnd

jkotas · 2018-04-25T14:20:39Z

src/gc/env/gcenv.safeint.h

@@ -0,0 +1,810 @@
+// Licensed to the .NET Foundation under one or more agreements.


I think it is overkill to include copy of this whole file for a single overflow check.

Also, the overflow check that this is for is pretty useless. You can just replace it by assert.

@jkotas We also use S_SIZE_T for init() (see gc.cpp:5059), although looking at the code I don't think it is currently protecting anything. It is written as S_SIZE_T safe_sniff_buf_size = S_SIZE_T(1 + n_heaps*n_sniff_buffers + 1);, which seems like any overflow would happen in the calculation before it is passed to the S_SIZE_T constructor.

I'm fine with getting rid of the file, I agree it's pretty heavyweight for how it's used. I was just being cautious because it's tricky to write overflow code so the compiler doesn't defeat it

jkotas · 2018-04-25T14:26:57Z

src/gc/objecthandle.cpp

        break;

-#if defined(FEATURE_COMINTEROP) && !defined(FEATURE_REDHAWK)
+#if defined(FEATURE_COMINTEROP) && !defined(FEATURE_REDHAWK) && !defined(BUILD_AS_STANDALONE)


COM interop is broken with standalone GC. Do you have a follow up bug to fix this?

We have pretty light test coverage for COM interop in the open sourced tests today, so you might not have noticed the breakage if you have just run CoreCLR open sourced tests.

I have only run the open source tests, and they all pass under local gc. But they also passed when it was compiled with FEATURE_REDHAWK so they don't test the corner cases of the gc.

I opened https://github.com/dotnet/coreclr/issues/17789

jkotas · 2018-04-25T14:28:08Z

src/gc/handletable.cpp

    CONTRACTL_END;

-#if defined( _DEBUG) && !defined(FEATURE_REDHAWK)
+#if defined( _DEBUG) && !defined(FEATURE_REDHAWK) && !defined(BUILD_AS_STANDALONE)


It would be better to delete the inject faults from here, and move them to the EE side instead.

does anyone actually use this? I have not seen it myself

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

It has been used for SQL testing 10+ years ago. It is not actively used for anything. I guess we can just delete it.

yeah, I vote for deleting it.

Ok, it's deleted now

jkotas · 2018-04-25T14:29:20Z

src/gc/gcpriv.h

 #endif //TRACE_GC

-#ifndef FEATURE_REDHAWK
+#if !defined(FEATURE_REDHAWK) && !defined(BUILD_AS_STANDALONE)


It would be better to move this to somewhere in vm\gcenv.*.

jkotas · 2018-04-25T14:30:22Z

src/gc/gc.cpp

 BOOL AnalyzeSurvivorsRequested(int condemnedGeneration)
 {
+#ifndef BUILD_AS_STANDALONE
    // Is the list active?


What is the end-user visible debugger feature that is not going to work with standalone GC because of this?

I think it's just some events are written to debugger on a gc, the only thing I could find that uses it is

coreclr/src/ToolBox/SOS/Strike/strike.cpp

Lines 6880 to 6904 in 54bdc16

STDMETHODIMP OnGcEvent(GcEvtArgs gcEvtArgs)

{

// by default don't stop on these notifications...

m_dbgStatus = DEBUG_STATUS_GO_HANDLED;

IXCLRDataProcess2* idp2 = NULL;

if (SUCCEEDED(g_clrData->QueryInterface(IID_IXCLRDataProcess2, (void**) &idp2)))

{

if (gcEvtArgs.typ == GC_MARK_END)

{

// erase notification request

GcEvtArgs gea = { GC_MARK_END, { 0 } };

idp2->SetGcNotification(gea);

s_condemnedGen = bitidx(gcEvtArgs.condemnedGeneration);

ExtOut("CLR notification: GC - Performing a gen %d collection. Determined surviving objects...\n", s_condemnedGen);

// GC_MARK_END notification means: give the user a chance to examine the debuggee

m_dbgStatus = DEBUG_STATUS_BREAK;

}

}

return S_OK;

}

@noahfalk please correct me if I missed something

That OnGcEvent method is part of the implementation for !FindRoot
https://github.com/dotnet/coreclr/blob/master/src/ToolBox/SOS/Strike/sosdocs.txt#L1918
Without this event !FindRoot -gen won't ever break back into the debugger.

janvorli · 2018-04-25T23:00:34Z

src/gc/objecthandle.cpp

        // Let's create a new node
-        NewHolder<HandleTableMap> newMap;
-        newMap = new (nothrow) HandleTableMap;
+        HandleTableMap *newMap = new (nothrow) HandleTableMap;


Why have you removed the holder? It is a pattern that we prefer to use wherever we can.

GC is not using holders anywhere else. I do not think it is worth pulling in the whole holder machinery into local GC just for the one use.

Ah, ok. I have thought we already had it in.

davmason · 2018-08-10T00:08:38Z

@jkotas @Maoni0 Can you take a look?

These were the outstanding issues:

Com interop is still missing
- Tracked by #17789
DAC features are still missing
- Tracked by #17771
Jan suggested it would be better to move handle table fault injection to the VM side
- Won't fix - there are way too many overloads and call sites to do this efficiently, I think it is better to turn off handle table fault injection for local GC. It is not a frequently used item and doesn't matter for production scenarios

Let me know if you have anything else you're concerned about.

Maoni0 · 2018-08-10T01:31:33Z

src/gc/gc.cpp

 bool virtual_alloc_commit_for_heap(void* addr, size_t size, int h_number)
 {
-#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL)
+#if defined(MULTIPLE_HEAPS) && !defined(FEATURE_REDHAWK) && !defined(FEATURE_PAL) && !defined(BUILD_AS_STANDALONE)


this needs to work for standalone GC though... numa is pretty common these days.

Filed #19417 to track it

Maoni0

LGTM!

davmason · 2018-08-10T22:46:10Z

@dotnet-bot test Windows_NT x64 Release CoreFX Tests

davmason · 2018-08-11T06:48:29Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests

The function was incorrectly rounding the dwCommit down instead of up to OS_PAGE_SIZE. It accidentally works for OSes where page size is 4096 bytes, because the dwCommit is 4096. But for ARM64 Linux distros where the page size is 64kB, this was committing zero bytes and so the runtime initialization was crashing a bit later when it tried to access the memory it was supposed to be commited. This problem was introduced in dotnet#17769.

The function was incorrectly rounding the dwCommit down instead of up to OS_PAGE_SIZE. It accidentally works for OSes where page size is 4096 bytes, because the dwCommit is 4096. But for ARM64 Linux distros where the page size is 64kB, this was committing zero bytes and so the runtime initialization was crashing a bit later when it tried to access the memory it was supposed to be commited. This problem was introduced in #17769.

The function was incorrectly rounding the dwCommit down instead of up to OS_PAGE_SIZE. It accidentally works for OSes where page size is 4096 bytes, because the dwCommit is 4096. But for ARM64 Linux distros where the page size is 64kB, this was committing zero bytes and so the runtime initialization was crashing a bit later when it tried to access the memory it was supposed to be commited. This problem was introduced in dotnet#17769.

davmason added 9 commits April 12, 2018 13:51

fix _ASSERTE issue

d2904ff

switch to interface call for GetLoaderAllocatorObject

90afeb7

fix build errors in handletable

1d236fa

use redhawk versions of validate, etc, for local gc

f423637

add ClrSafeInt for local gc

47fd107

fix build errors in gcenv.safeint.h

1713bc1

move gcstress call to vm and disable use of gcnotifications

1dc6363

disable numa info for local gc

489899f

fix build error

aab2a4d

dotnet-bot added the 2 - In Progress label Apr 25, 2018

include type_traits, std::is_signed wasn't found on linux builds

46b1ae1

davmason requested review from Maoni0 and sywhang April 25, 2018 08:44

jkotas reviewed Apr 25, 2018

View reviewed changes

janvorli reviewed Apr 25, 2018

View reviewed changes

davmason mentioned this pull request Jun 8, 2018

Avoid NativeOverlapped pinning by allocating unmanaged memory for it #18360

Merged

remove safeint from gc

dc41778

Maoni0 reviewed Aug 10, 2018

View reviewed changes

Maoni0 approved these changes Aug 10, 2018

View reviewed changes

remove handle table inject fault

a07ed40

switch to getgcsafemethodtable

99fbb6a

davmason merged commit d4738d1 into dotnet:master Aug 11, 2018

davmason deleted the redhawk branch August 14, 2018 20:00

janvorli mentioned this pull request Oct 5, 2018

Fix SegmentInitialize for OS_PAGE_SIZE > 4k #20280

Merged

		@@ -0,0 +1,810 @@
		// Licensed to the .NET Foundation under one or more agreements.

	STDMETHODIMP OnGcEvent(GcEvtArgs gcEvtArgs)
	{
	// by default don't stop on these notifications...
	m_dbgStatus = DEBUG_STATUS_GO_HANDLED;

	IXCLRDataProcess2* idp2 = NULL;
	if (SUCCEEDED(g_clrData->QueryInterface(IID_IXCLRDataProcess2, (void**) &idp2)))
	{
	if (gcEvtArgs.typ == GC_MARK_END)
	{
	// erase notification request
	GcEvtArgs gea = { GC_MARK_END, { 0 } };
	idp2->SetGcNotification(gea);

	s_condemnedGen = bitidx(gcEvtArgs.condemnedGeneration);

	ExtOut("CLR notification: GC - Performing a gen %d collection. Determined surviving objects...\n", s_condemnedGen);

	// GC_MARK_END notification means: give the user a chance to examine the debuggee
	m_dbgStatus = DEBUG_STATUS_BREAK;
	}
	}

	return S_OK;
	}

[local gc] disable feature redhawk #17769

[local gc] disable feature redhawk #17769

Uh oh!

Conversation

davmason commented Apr 25, 2018

Uh oh!

davmason commented Apr 25, 2018

Uh oh!

davmason commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davmason commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maoni0 left a comment

Choose a reason for hiding this comment

Uh oh!

davmason commented Aug 10, 2018

Uh oh!

davmason commented Aug 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jkotas Apr 25, 2018 •

edited

Loading