Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Stop unloading the profiler on shutdown to prevent segfaults on background threads #26762

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

davmason
Copy link
Member

Fixes #26687

On Linux we were seeing profilers crash in ELT hooks because the profiler would be shut down before all managed threads had exited. These checks will prevent slow path ELT from crashing, profiler vendors will have to deal with fast path ELT calls that come in after the profiler is shut down.

@davmason davmason added this to the 5.0 milestone Sep 18, 2019
@davmason davmason self-assigned this Sep 18, 2019
@@ -10235,11 +10235,15 @@ HCIMPL2(EXTERN_C void, ProfileEnter, UINT_PTR clientData, void * platformSpecifi

#ifdef PROFILING_SUPPORTED

// We might have already torn down the profiler, if so we need to abort.
if (g_profControlBlock.curProfStatus.Get() != kProfStatusActive
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the profiler is torn down right after we do this check, but before we actually call it below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there are all sorts of races here. I realize now that even the check can race, depending on the order of events g_profControlBlock.pProfInterface could be set to null after g_profControlBlock.pProfInterface.Load() == NULL and then g_profControlBlock.pProfInterface->GetEnterHook() == NULL would AV. I need to rework this entirely.

@davmason
Copy link
Member Author

Looking at why this happens on Linux and not Windows it's because we don't set g_fProcessDetach on Linux. Talking to @janvorli this is on purpose, and we should try to avoid doing any checks about why the runtime is terminating. We already would skip freeing the profiler in many cases on Windows, I think the only safe thing to do here is to stop freeing the profiler on shutdown altogether. It may affect some profilers, but I believe they will be able to either free what they need to in the Shutdown callback or when the OS frees the library after we exit.

The only other options are to continue crashing sometimes or to put expensive synchronization on a hot path in ProfilerEnter,

@davmason davmason changed the title Check if profiler has been shut down to prevent segfault in ELT hooks Stop unloading the profiler on shutdown to prevent segfaults on background threads Sep 23, 2019
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM, sorry I missed this earlier

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.

pProfInterface is used after being freed by TerminateProfiling
3 participants