Code review feedback

dotnet · Mar 1, 2023 · 3e225cd · 3e225cd
1 parent fb01dfb
commit 3e225cd
Show file tree

Hide file tree

Showing 8 changed files with 28 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 (