Generates a guid for activity id (#88838)

* Fixes the way activity id is generated * FB * FB * FB * Apply suggestions from code review Co-authored-by: Jan Kotas <[email protected]> * letting each library specify the config.h * Add needed constants to minipalconfig * triggering random constants in configure.cmake * FB --------- Co-authored-by: Jan Kotas <[email protected]>
dotnet · Jul 26, 2023 · 254263f · 254263f
1 parent 0f9daf0
commit 254263f
Show file tree

Hide file tree

Showing 12 changed files with 273 additions and 218 deletions.
diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/CMakeLists.txt b/src/coreclr/nativeaot/Runtime/eventpipe/CMakeLists.txt
@@ -26,22 +26,28 @@ list(APPEND MINIPAL_SOURCES
 
 if(CLR_CMAKE_HOST_WIN32)
   list(APPEND SHARED_DIAGNOSTIC_SERVER_SOURCES
-      ds-ipc-pal-namedpipe.c
+    ds-ipc-pal-namedpipe.c
   )
 
   list(APPEND SHARED_DIAGNOSTIC_SERVER_HEADERS
-  ds-ipc-pal-namedpipe.h
+    ds-ipc-pal-namedpipe.h
   )
 endif(CLR_CMAKE_HOST_WIN32)
 
 if(CLR_CMAKE_HOST_UNIX)
   list(APPEND SHARED_DIAGNOSTIC_SERVER_SOURCES
-      ds-ipc-pal-socket.c
+    ds-ipc-pal-socket.c
   )
 
   list(APPEND SHARED_DIAGNOSTIC_SERVER_HEADERS
-      ds-ipc-pal-socket.h
+    ds-ipc-pal-socket.h
   )
+
+  include(${CLR_SRC_NATIVE_DIR}/minipal/configure.cmake)
+  list(APPEND MINIPAL_SOURCES
+    random.c
+  )
+
 endif(CLR_CMAKE_HOST_UNIX)
 
 list(APPEND EVENTPIPE_SOURCES

diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ds-rt-aot.h b/src/coreclr/nativeaot/Runtime/eventpipe/ds-rt-aot.h
@@ -181,13 +181,8 @@ ds_rt_generate_core_dump (
 {
     STATIC_CONTRACT_NOTHROW;
 
-    ds_ipc_result_t result = DS_IPC_E_FAIL;
-    uint32_t flags = ds_generate_core_dump_command_payload_get_flags(payload);
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Generate an exception dump
-    // PalDebugBreak();
-
-    return 0;
+    // Eventpipe driven core_dump is not currently supported in NativeAOT
+    return DS_IPC_E_NOTSUPPORTED;
 }
 
 /*

diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
@@ -17,6 +17,8 @@
 #include <unistd.h>
 #endif
 
+#include <minipal/random.h>
+
 #include "gcenv.h"
 
 #ifndef DIRECTORY_SEPARATOR_CHAR
@@ -237,8 +239,6 @@ ep_rt_aot_atomic_inc_int64_t (volatile int64_t *value)
 {
     STATIC_CONTRACT_NOTHROW;
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Consider replacing with a new PalInterlockedIncrement64 service
     int64_t currentValue;
     do {
         currentValue = *value;
@@ -252,8 +252,6 @@ int64_t
 ep_rt_aot_atomic_dec_int64_t (volatile int64_t *value) { 
     STATIC_CONTRACT_NOTHROW;
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Consider replacing with a new PalInterlockedDecrement64 service
     int64_t currentValue;
     do {
         currentValue = *value;
@@ -366,8 +364,7 @@ ep_rt_aot_thread_create (
     STATIC_CONTRACT_NOTHROW;
     EP_ASSERT (thread_func != NULL);
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Fill in the outgoing id if any callers ever need it
+    // Note that none of the callers examine the return id in any way
     if (id)
         *reinterpret_cast<DWORD*>(id) = 0xffffffff;
 
@@ -789,6 +786,47 @@ void ep_rt_aot_os_environment_get_utf16 (dn_vector_ptr_t *env_array)
 #endif
 }
 
+void ep_rt_aot_create_activity_id (uint8_t *activity_id, uint32_t activity_id_len)
+{
+    // We call CoCreateGuid for windows, and use a random generator for non-windows
+    STATIC_CONTRACT_NOTHROW;
+    EP_ASSERT (activity_id != NULL);
+    EP_ASSERT (activity_id_len == EP_ACTIVITY_ID_SIZE);
+#ifdef HOST_WIN32
+    CoCreateGuid (reinterpret_cast<GUID *>(activity_id));
+#else
+    if(minipal_get_cryptographically_secure_random_bytes(activity_id, activity_id_len)==-1)
+    {
+        *activity_id=0;
+        return;
+    }
+
+    const uint16_t version_mask = 0xF000;
+    const uint16_t random_guid_version = 0x4000;
+    const uint8_t clock_seq_hi_and_reserved_mask = 0xC0;
+    const uint8_t clock_seq_hi_and_reserved_value = 0x80;
+
+    // Modify bits indicating the type of the GUID
+    uint8_t *activity_id_c = activity_id + sizeof (uint32_t) + sizeof (uint16_t);
+    uint8_t *activity_id_d = activity_id + sizeof (uint32_t) + sizeof (uint16_t) + sizeof (uint16_t);
+
+    uint16_t c;
+    memcpy (&c, activity_id_c, sizeof (c));
+
+    uint8_t d;
+    memcpy (&d, activity_id_d, sizeof (d));
+
+    // time_hi_and_version
+    c = ((c & ~version_mask) | random_guid_version);
+    // clock_seq_hi_and_reserved
+    d = ((d & ~clock_seq_hi_and_reserved_mask) | clock_seq_hi_and_reserved_value);
+
+    memcpy (activity_id_c, &c, sizeof (c));
+    memcpy (activity_id_d, &d, sizeof (d));
+#endif
+}
+
+
 #ifdef EP_CHECKED_BUILD
 
 void ep_rt_aot_lock_requires_lock_held (const ep_rt_lock_handle_t *lock)

diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
@@ -54,8 +54,7 @@
 
 extern void ep_rt_aot_thread_exited (void);
 
-// shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-// TODO: The NativeAOT ALIGN_UP is defined in a tangled manner that generates linker errors if
+// The NativeAOT ALIGN_UP is defined in a tangled manner that generates linker errors if
 // it is used here; instead, define a version tailored to the existing usage in the shared
 // EventPipe code.
 static inline uint8_t* _rt_aot_align_up(uint8_t* val, uintptr_t alignment)
@@ -324,10 +323,7 @@ ep_rt_method_get_simple_assembly_name (
 {
     STATIC_CONTRACT_NOTHROW;
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Design MethodDesc and method name services if/when needed
-    //PalDebugBreak();
-
+    // NativeAOT does not support method_desc operations
     return false;
 
 }
@@ -339,10 +335,7 @@ ep_rt_method_get_full_name (
     ep_char8_t *name,
     size_t name_len)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Design MethodDesc and method name services if/when needed
-    //PalDebugBreak();
-
+    // NativeAOT does not support method_desc operations
     return false;
 }
 
@@ -613,11 +606,9 @@ EventPipeWaitHandle
 ep_rt_wait_event_get_wait_handle (ep_rt_wait_event_handle_t *wait_event) 
 { 
     STATIC_CONTRACT_NOTHROW;
-    // EP_ASSERT (wait_event != NULL && wait_event->event != NULL);
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: NativeAOT CLREventStatic doesn't have GetHandleUNHOSTED
-    // PalDebugBreak();
+    // This is not reached in the current product
+    abort();
     return 0;
 }
 
@@ -675,41 +666,8 @@ ep_rt_create_activity_id (
     uint8_t *activity_id,
     uint32_t activity_id_len)
 {
-    STATIC_CONTRACT_NOTHROW;
-    EP_ASSERT (activity_id != NULL);
-    EP_ASSERT (activity_id_len == EP_ACTIVITY_ID_SIZE);
-
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement a way to generate a real Guid
-    // CoCreateGuid (reinterpret_cast<GUID *>(activity_id));
-
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Using roughly Mono's implementation but Mono randomly generates this, hardcoding for now
-    uint8_t data1[] = {0x67,0xac,0x33,0xf1,0x8d,0xed,0x41,0x01,0xb4,0x26,0xc9,0xb7,0x94,0x35,0xf7,0x8a};
-    memcpy (activity_id, data1, EP_ACTIVITY_ID_SIZE);
-
-    const uint16_t version_mask = 0xF000;
-    const uint16_t random_guid_version = 0x4000;
-    const uint8_t clock_seq_hi_and_reserved_mask = 0xC0;
-    const uint8_t clock_seq_hi_and_reserved_value = 0x80;
-
-    // Modify bits indicating the type of the GUID
-    uint8_t *activity_id_c = activity_id + sizeof (uint32_t) + sizeof (uint16_t);
-    uint8_t *activity_id_d = activity_id + sizeof (uint32_t) + sizeof (uint16_t) + sizeof (uint16_t);
-
-    uint16_t c;
-    memcpy (&c, activity_id_c, sizeof (c));
-
-    uint8_t d;
-    memcpy (&d, activity_id_d, sizeof (d));
-
-    // time_hi_and_version
-    c = ((c & ~version_mask) | random_guid_version);
-    // clock_seq_hi_and_reserved
-    d = ((d & ~clock_seq_hi_and_reserved_mask) | clock_seq_hi_and_reserved_value);
-
-    memcpy (activity_id_c, &c, sizeof (c));
-    memcpy (activity_id_d, &d, sizeof (d));
+    extern void ep_rt_aot_create_activity_id (uint8_t *activity_id, uint32_t activity_id_len);
+    ep_rt_aot_create_activity_id(activity_id, activity_id_len);
 }
 
 static
@@ -718,9 +676,9 @@ bool
 ep_rt_is_running (void)
 {
     STATIC_CONTRACT_NOTHROW;
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Does NativeAot have the concept of EEStarted
-    // PalDebugBreak();
+
+    // This is only used to check if the profiler can be attached
+    // Profiler attach is not supported in NativeAOT
 
     return false;
 }
@@ -732,11 +690,7 @@ ep_rt_execute_rundown (dn_vector_ptr_t *execution_checkpoints)
 {
     STATIC_CONTRACT_NOTHROW;
 
-    //TODO: Write execution checkpoint rundown events.
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: EventPipe Configuration values - RhConfig?
-    // (CLRConfig::INTERNAL_EventPipeCircularMB)
-    // PalDebugBreak();
+    // NativeAOT does not currently support rundown
 }
 
 /*
@@ -772,8 +726,6 @@ EP_RT_DEFINE_THREAD_FUNC (ep_rt_thread_aot_start_session_or_sampling_thread)
 
     ep_rt_thread_params_t* thread_params = reinterpret_cast<ep_rt_thread_params_t *>(data);
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement thread creation/management if needed.
     // The session and sampling threads both assert that the incoming thread handle is
     // non-null, but do not necessarily rely on it otherwise; just pass a meaningless non-null
     // value until testing shows that a meaningful value is needed.
@@ -808,9 +760,7 @@ inline
 void
 ep_rt_set_server_name(void)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Need to set name for  the thread
-    // ::SetThreadName(GetCurrentThread(), W(".NET EventPipe"));
+    // This is optional, decorates the thread name with EventPipe specific information
 }
 
 
@@ -1569,10 +1519,7 @@ ep_rt_thread_setup (void)
 {
     STATIC_CONTRACT_NOTHROW;
 
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement thread creation/management if needed
-    // Thread* thread_handle = SetupThreadNoThrow ();
-    // EP_ASSERT (thread_handle != NULL);
+    // Likely not needed and do nothing until testing shows to be required
 }
 
 static

diff --git a/src/native/libs/Common/pal_config.h.in b/src/native/libs/Common/pal_config.h.in
@@ -51,7 +51,6 @@
 #cmakedefine01 HAVE_SCHED_GETCPU
 #cmakedefine01 HAVE_PTHREAD_SETCANCELSTATE
 #cmakedefine01 HAVE_GNU_LIBNAMES_H
-#cmakedefine01 HAVE_ARC4RANDOM_BUF
 #cmakedefine01 KEVENT_HAS_VOID_UDATA
 #cmakedefine01 HAVE_FDS_BITS
 #cmakedefine01 HAVE_PRIVATE_FDS_BITS

diff --git a/src/native/libs/System.Native/CMakeLists.txt b/src/native/libs/System.Native/CMakeLists.txt
@@ -14,6 +14,16 @@ if (CLR_CMAKE_TARGET_OSX)
     add_definitions(-D_DARWIN_C_SOURCE)
 endif ()
 
+set (MINIPAL_SOURCES "")
+set (SHARED_MINIPAL_SOURCE_PATH "${CLR_SRC_NATIVE_DIR}/minipal")
+
+include(${CLR_SRC_NATIVE_DIR}/minipal/configure.cmake)
+list(APPEND MINIPAL_SOURCES
+    random.c
+)
+
+addprefix(MINIPAL_SOURCES ${SHARED_MINIPAL_SOURCE_PATH} "${MINIPAL_SOURCES}")
+
 set(NATIVE_SOURCES
     pal_errno.c
     pal_interfaceaddresses.c
@@ -30,6 +40,10 @@ set(NATIVE_SOURCES
     pal_sysctl.c
 )
 
+list(APPEND NATIVE_SOURCES
+  ${MINIPAL_SOURCES}
+)
+
 if (NOT CLR_CMAKE_TARGET_WASI)
     list (APPEND NATIVE_SOURCES
         pal_dynamicload.c