From 3e225cd4e681fa7a53168b39c65aa581ec541b75 Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 23 Feb 2023 15:03:26 -0800 Subject: [PATCH] Code review feedback --- src/coreclr/inc/clrconfigvalues.h | 2 +- src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h | 13 +++++++++++++ src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h | 4 ++-- src/mono/mono/eventpipe/ep-rt-mono.h | 10 +++++----- src/native/eventpipe/ep-buffer-manager.c | 2 +- src/native/eventpipe/ep-rt.h | 3 ++- src/native/eventpipe/ep-session.c | 2 +- src/native/eventpipe/ep-session.h | 6 +++--- 8 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index be75f12be9184f..9dd74d914ad746 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -688,7 +688,7 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeRundown, W("EventPipeRundown"), 1, "E RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeCircularMB, W("EventPipeCircularMB"), 1024, "The EventPipe circular buffer size in megabytes.") RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeProcNumbers, W("EventPipeProcNumbers"), 0, "Enable/disable capturing processor numbers in EventPipe event headers") RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeOutputStreaming, W("EventPipeOutputStreaming"), 0, "Enable/disable streaming for trace file set in DOTNET_EventPipeOutputPath. Non-zero values enable streaming.") -RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeDisableStacks, W("EventPipeDisableStacks"), 0, "Set to 1 to disable collecting stacks for EventPipe events.") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeEnableStackwalk, W("EventPipeEnableStackwalk"), 1, "Set to 0 to disable collecting stacks for EventPipe events.") #ifdef FEATURE_AUTO_TRACE RETAIL_CONFIG_DWORD_INFO_EX(INTERNAL_AutoTrace_N_Tracers, W("AutoTrace_N_Tracers"), 0, "", CLRConfig::LookupOptions::ParseIntegerAsBase10) diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h index 35494b254af939..a1ba6e18ab49c2 100644 --- a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h +++ b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h @@ -1639,6 +1639,19 @@ ep_rt_config_value_get_output_streaming (void) return false; } +static +inline +bool +ep_rt_config_value_get_enable_stackwalk (void) +{ + STATIC_CONTRACT_NOTHROW; + // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase + // TODO: EventPipe Configuration values - RhConfig? + // (CLRConfig::INTERNAL_EventPipeEnableStackwalk) + //PalDebugBreak(); + return true; +} + /* * EventPipeSampleProfiler. */ diff --git a/src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h b/src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h index c26617f3543fa4..0f679ab919f8aa 100644 --- a/src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h +++ b/src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h @@ -1694,10 +1694,10 @@ ep_rt_config_value_get_output_streaming (void) static inline bool -ep_rt_config_value_get_disable_stacks (void) +ep_rt_config_value_get_enable_stackwalk (void) { STATIC_CONTRACT_NOTHROW; - return CLRConfig::GetConfigValue(CLRConfig::INTERNAL_EventPipeDisableStacks) != 0; + return CLRConfig::GetConfigValue(CLRConfig::INTERNAL_EventPipeEnableStackwalk) != 0; } /* diff --git a/src/mono/mono/eventpipe/ep-rt-mono.h b/src/mono/mono/eventpipe/ep-rt-mono.h index cf27ccdf42ba6b..cbc8038663b0b3 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono.h +++ b/src/mono/mono/eventpipe/ep-rt-mono.h @@ -1020,16 +1020,16 @@ ep_rt_config_value_get_rundown (void) static inline bool -ep_rt_config_value_get_disable_stacks (void) +ep_rt_config_value_get_enable_stackwalk (void) { - uint32_t value_uint32_t = 0; - gchar *value = g_getenv ("DOTNET_EventPipeDisableStacks"); + uint32_t value_uint32_t = 1; + gchar *value = g_getenv ("DOTNET_EventPipeEnableStackwalk"); if (!value) - value = g_getenv ("COMPlus_EventPipeDisableStacks"); + value = g_getenv ("COMPlus_EventPipeEnableStackwalk"); if (value) value_uint32_t = (uint32_t)atoi (value); g_free (value); - return value_uint32_t; + return value_uint32_t != 0; } /* diff --git a/src/native/eventpipe/ep-buffer-manager.c b/src/native/eventpipe/ep-buffer-manager.c index f928a700aea9fb..5b54da16d1e994 100644 --- a/src/native/eventpipe/ep-buffer-manager.c +++ b/src/native/eventpipe/ep-buffer-manager.c @@ -970,7 +970,7 @@ ep_buffer_manager_write_event ( event_thread = thread; current_stack_contents = ep_stack_contents_init (&stack_contents); - if (stack == NULL && !ep_session_get_disable_stacks(session) && ep_event_get_need_stack (ep_event) && !ep_session_get_rundown_enabled (session)) { + if (stack == NULL && ep_session_get_enable_stackwalk (session) && ep_event_get_need_stack (ep_event) && !ep_session_get_rundown_enabled (session)) { ep_walk_managed_stack_for_current_thread (current_stack_contents); stack = current_stack_contents; } diff --git a/src/native/eventpipe/ep-rt.h b/src/native/eventpipe/ep-rt.h index c7ad03371cc0a0..fe9dc181b7321b 100644 --- a/src/native/eventpipe/ep-rt.h +++ b/src/native/eventpipe/ep-rt.h @@ -403,8 +403,9 @@ bool ep_rt_config_value_get_output_streaming (void); static +inline bool -ep_rt_config_value_get_disable_stacks (void); +ep_rt_config_value_get_enable_stackwalk (void); /* * EventPipeSampleProfiler. diff --git a/src/native/eventpipe/ep-session.c b/src/native/eventpipe/ep-session.c index faf8d23984794b..f7069c3e46084a 100644 --- a/src/native/eventpipe/ep-session.c +++ b/src/native/eventpipe/ep-session.c @@ -202,7 +202,7 @@ ep_session_alloc ( instance->session_start_time = ep_system_timestamp_get (); instance->session_start_timestamp = ep_perf_timestamp_get (); instance->paused = false; - instance->disable_stacks = ep_rt_config_value_get_disable_stacks(); + instance->enable_stackwalk = ep_rt_config_value_get_enable_stackwalk (); ep_on_exit: ep_requires_lock_held (); diff --git a/src/native/eventpipe/ep-session.h b/src/native/eventpipe/ep-session.h index 22f48e3cd68699..36879e2bbc8aab 100644 --- a/src/native/eventpipe/ep-session.h +++ b/src/native/eventpipe/ep-session.h @@ -59,8 +59,8 @@ struct _EventPipeSession_Internal { // we expect to remove it in the future once that limitation is resolved other scenarios are discouraged from using this given that // we plan to make it go away bool paused; - // Set via environment variable to prevent stack collection for all events - bool disable_stacks; + // Set via environment variable to enable or disable stack collection globally + bool enable_stackwalk; }; #if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_SESSION_GETTER_SETTER) @@ -77,7 +77,7 @@ EP_DEFINE_GETTER(EventPipeSession *, session, bool, rundown_requested) EP_DEFINE_GETTER(EventPipeSession *, session, ep_timestamp_t, session_start_time) EP_DEFINE_GETTER(EventPipeSession *, session, ep_timestamp_t, session_start_timestamp) EP_DEFINE_GETTER(EventPipeSession *, session, EventPipeFile *, file) -EP_DEFINE_GETTER(EventPipeSession *, session, bool, disable_stacks) +EP_DEFINE_GETTER(EventPipeSession *, session, bool, enable_stackwalk) EventPipeSession * ep_session_alloc (