Skip to content

Disable computation of topStack on Windows platforms #54986

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.

Sign up for GitHub

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

Closed
davidwrighton wants to merge 1 commit into from
Closed

Disable computation of topStack on Windows platforms #54986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/coreclr/gc/gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21174,10 +21174,9 @@ uint8_t* gc_heap::find_object (uint8_t* interior)
heap_segment* seg = find_segment (interior, FALSE);
if (seg)
{
#ifdef FEATURE_CONSERVATIVE_GC
if (interior >= heap_segment_allocated(seg))
return 0;
#endif
Comment on lines -21177 to -21180

@Maoni0 Maoni0 Jul 1, 2021

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.

functionally I don't see a problem. I'm curious - could interior be actually >= heap_segment_allocated if GCConfig::GetConservativeGC() is false? this would mean it's reporting an interior pointer that's indeed on a segment but not pointing at a valid object on that seg.


// If interior falls within the first free object at the beginning of a generation,
// we don't have brick entry for it, and we may incorrectly treat it as on large object heap.
int align_const = get_alignment_constant (heap_segment_read_only_p (seg)
Expand Down Expand Up @@ -21208,12 +21207,8 @@ uint8_t* gc_heap::find_object (uint8_t* interior)
heap_segment* seg = find_segment (interior, TRUE);
if (seg)
{
#ifdef FEATURE_CONSERVATIVE_GC
if (interior >= heap_segment_allocated (seg))
return 0;
#else
assert (interior < heap_segment_allocated (seg));
#endif
uint8_t* o = find_first_object (interior, heap_segment_mem (seg));
return o;
}
Expand Down
25 changes: 25 additions & 0 deletions src/coreclr/gc/gcinterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,11 +952,32 @@ inline GC_ALLOC_FLAGS& operator&=(GC_ALLOC_FLAGS& a, GC_ALLOC_FLAGS b)
#define UNCHECKED_OBJECTREF_TO_OBJECTREF(obj) (obj)
#endif

#if !defined(FEATURE_HIJACK) || defined(TARGET_UNIX)
#define FEATURE_SUPPORTS_STACK_LIMIT
#else
#if defined(FEATURE_CONSERVATIVE_GC)
// On Windows platforms when we hijack, we use SuspendThread from outside the thread, and if we suspend
// within an interruptible region, we can stop with the top Frame that is an InlinedCallFrame

@jkotas jkotas Jul 1, 2021

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.

I would expect RedirectedThreadFrame to be the top Frame when we stop within an interruptible region and start GC walk. Why are the problematic cases missing RedirectedThreadFrame ?

@davidwrighton davidwrighton Jul 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You have a point. I thought I had managed to reproduce the scenario (or something very like it) in the debugger, but I agree, I can't quite see how we didn't end up with a proper RedirectedThreadFrame. In any case, the best crash dump I could get from the CI system had a disabled InlinedCallFrame as the top of the framestack. (Unfortunately, for some reason, the thread that frame is on had been terminated, so it wasn't completely workable.) I'll try a again with some debugger fun times.

// that is not active. In that situation, it is possible for the actual top of the stack to be a managed
// function which has a lower SP pointer than is reported from the InlinedCallFrame, or in the case of
// Windows x86 platforms, the InlinedCallFrame may report a GetCallSiteSP of 0, or of an value associated
// with some other callsite within the managed function that set up the InlinedCallFrame. This issue could
// be fixed by capturing the SP after suspension, and storing it on the thread object for use when setting
// stack_limit, but as conservative GC is not needed on any current Windows platforms, and the Unix scenario
// is fully functional, I've added this #error instead.
#error Conservative GC is not supported on this platform as the current computation of stack_limit is not strictly correct
#endif // FEATURE_CONSERVATIVE_GC
#endif // !defined(FEATURE_HIJACK) || defined(TARGET_UNIX)

struct ScanContext
{
Thread* thread_under_crawl;
int thread_number;
#ifdef FEATURE_SUPPORTS_STACK_LIMIT
uintptr_t stack_limit; // Lowest point on the thread stack that the scanning logic is permitted to read
#else
uintptr_t stack_limitUNUSED; // Keep the shape of ScanContext the same whether or not STACK_LIMIT is supported for use
#endif // FEATURE_SUPPORTS_STACK_LIMIT
Comment on lines +976 to +980

@Maoni0 Maoni0 Jul 1, 2021

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.

do we really need this distinction in this file? GC doesn't care at all about whether this is a stack limit you maintain or not... it would be good to not have this knowledge in the GC interface unless it's mandatory...

bool promotion; //TRUE: Promotion, FALSE: Relocation.
bool concurrent; //TRUE: concurrent scanning
void* _unused1;
Expand All @@ -973,7 +994,11 @@ struct ScanContext

thread_under_crawl = 0;
thread_number = -1;
#ifdef FEATURE_SUPPORTS_STACK_LIMIT
stack_limit = 0;
#else
stack_limitUNUSED = 0;
#endif // FEATURE_SUPPORTS_STACK_LIMIT
promotion = false;
concurrent = false;
pMD = NULL;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/inc/switches.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@
// are treated as potential pinned interior pointers. When enabled, the runtime flag COMPLUS_GCCONSERVATIVE
// determines dynamically whether GC is conservative. Note that appdomain unload, LCG and unloadable assemblies
// do not work reliably with conservative GC.
#if defined(TARGET_UNIX) || !defined(FEATURE_HIJACK) // Windows currently doesn't support conservative GC when suspending interruptible code
#define FEATURE_CONSERVATIVE_GC 1
#endif

#if (defined(TARGET_ARM) && (!defined(ARM_SOFTFP) || defined(CONFIGURABLE_ARM_ABI))) || defined(TARGET_ARM64)
#define FEATURE_HFA
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/frames.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1528,8 +1528,10 @@ BOOL TransitionFrame::Protects(OBJECTREF * ppORef)
{
WRAPPER_NO_CONTRACT;
IsObjRefProtectedScanContext sc (ppORef);
#ifdef FEATURE_SUPPORTS_STACK_LIMIT
// Set the stack limit for the scan to the SP of the managed frame above the transition frame
sc.stack_limit = GetSP();
#endif // FEATURE_SUPPORTS_STACK_LIMIT
GcScanRoots (IsObjRefProtected, &sc);
return sc.oref_protected;
}
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/gcenv.ee.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ static void ScanStackRoots(Thread * pThread, promote_func* fn, ScanContext* sc)
IsGCSpecialThread() ||
(GetThread() == ThreadSuspend::GetSuspensionThread() && ThreadStore::HoldingThreadStore()));

#ifdef FEATURE_SUPPORTS_STACK_LIMIT
Frame* pTopFrame = pThread->GetFrame();
Object ** topStack = (Object **)pTopFrame;
if ((pTopFrame != ((Frame*)-1))
Expand All @@ -124,6 +125,7 @@ static void ScanStackRoots(Thread * pThread, promote_func* fn, ScanContext* sc)
}

sc->stack_limit = (uintptr_t)topStack;
#endif // FEATURE_SUPPORTS_STACK_LIMIT

#ifdef FEATURE_CONSERVATIVE_GC
if (g_pConfig->GetGCConservative())
Expand Down
9 changes: 7 additions & 2 deletions src/coreclr/vm/siginfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4906,16 +4906,21 @@ void PromoteCarefully(promote_func fn,
//
// Sanity check the stack scan limit
//
#ifdef FEATURE_SUPPORTS_STACK_LIMIT
assert(sc->stack_limit != 0);
#endif // FEATURE_SUPPORTS_STACK_LIMIT

// Note that the base is at a higher address than the limit, since the stack
// grows downwards.
// To check whether the object is in the stack or not, we also need to check the sc->stack_limit.
// The reason is that on Unix, the stack size can be unlimited. In such case, the system can
// shrink the current reserved stack space. That causes the real limit of the stack to move up and
// the range can be reused for other purposes. But the sc->stack_limit is stable during the scan.
// Even on Windows, we care just about the stack above the stack_limit.
if ((sc->thread_under_crawl->IsAddressInStack(*ppObj)) && (PTR_TO_TADDR(*ppObj) >= sc->stack_limit))
if ((sc->thread_under_crawl->IsAddressInStack(*ppObj))
#ifdef FEATURE_SUPPORTS_STACK_LIMIT
&& (PTR_TO_TADDR(*ppObj) >= sc->stack_limit)
#endif // FEATURE_SUPPORTS_STACK_LIMIT
)
{
return;
}
Expand Down