From 6f6c759cf94fc61b06133a70342725b720b13f0d Mon Sep 17 00:00:00 2001 From: Gregory LEOCADIE Date: Fri, 24 Oct 2025 01:55:26 +0200 Subject: [PATCH 1/4] Fix native and managed callstacks merge --- .../CrashReporting.cpp | 40 +++--- .../Datadog.Profiler.Native/CrashReporting.h | 3 + .../CrashReportingTest.cpp | 115 +++++++++++++++++- 3 files changed, 139 insertions(+), 19 deletions(-) diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp index 5d4a7f569ec1..ed2229eb530f 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp @@ -355,43 +355,49 @@ int32_t CrashReporting::ExportImpl(ddog_Endpoint* endpoint) std::vector CrashReporting::MergeFrames(const std::vector& nativeFrames, const std::vector& managedFrames) { std::vector result; + // it's safe here to not use nativeFrames.size() + managedFrames.size() + // because the managed frames should be a subset of the native frames result.reserve(std::max(nativeFrames.size(), managedFrames.size())); - size_t i = 0, j = 0; - while (i < nativeFrames.size() && j < managedFrames.size()) + auto nativeIt = nativeFrames.rbegin(); + auto managedIt = managedFrames.rbegin(); + while (nativeIt != nativeFrames.rend() && managedIt != managedFrames.rend()) { - if (nativeFrames[i].sp < managedFrames[j].sp) + if (nativeIt->sp > managedIt->sp) { - result.push_back(nativeFrames[i]); - ++i; + result.push_back(*nativeIt); + ++nativeIt; } - else if (managedFrames[j].sp < nativeFrames[i].sp) + else if (managedIt->sp > nativeIt->sp) { - result.push_back(managedFrames[j]); - ++j; + result.push_back(*managedIt); + ++managedIt; } else { // frames[i].sp == managedFrames[j].sp // Prefer managedFrame when sp values are the same - result.push_back(managedFrames[j]); - ++i; - ++j; + result.push_back(*managedIt); + ++nativeIt; + ++managedIt; } } // Add any remaining frames that are left in either vector - while (i < nativeFrames.size()) + while (nativeIt != nativeFrames.rend()) { - result.push_back(nativeFrames[i]); - ++i; + result.push_back(*nativeIt); + ++nativeIt; } - while (j < managedFrames.size()) + while (managedIt != managedFrames.rend()) { - result.push_back(managedFrames[j]); - ++j; + result.push_back(*managedIt); + ++managedIt; } + // we could also return the merged callstack without reversing but the caller would have to walk backwards + std::reverse(result.begin(), result.end()); + return result; } diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.h index 3a29d3ca83fb..ebe9302dd9c5 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.h +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.h @@ -143,6 +143,9 @@ class CrashReporting : public ICrashReporting virtual std::vector GetThreadFrames(int32_t tid, ResolveManagedCallstack resolveManagedCallstack, void* context) = 0; virtual std::string GetSignalInfo(int32_t signal) = 0; +#ifdef DD_TEST +public: +#endif static std::vector MergeFrames(const std::vector& nativeFrames, const std::vector& managedFrames); private: int32_t ExportImpl(ddog_Endpoint* endpoint); diff --git a/profiler/test/Datadog.Profiler.Native.Tests/CrashReportingTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/CrashReportingTest.cpp index e27b04016bf9..93d8a2cf1477 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/CrashReportingTest.cpp +++ b/profiler/test/Datadog.Profiler.Native.Tests/CrashReportingTest.cpp @@ -1,10 +1,20 @@ // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc. +#include "gtest/gtest.h" + +#include "unknwn.h" + +const IID IID_IUnknown = {0x00000000, + 0x0000, + 0x0000, + {0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46}}; + +#include "CrashReporting.h" + #ifdef _WIN32 #include "resource.h" -#include "gtest/gtest.h" #include #include #include @@ -130,5 +140,106 @@ TEST(CrashReportingTest, ExtractPdbSignaturePE64) // | Pdb signature |Age| ASSERT_STRCASEEQ(buildIdStr.data(), "C465AFCDBDBC58A0100995839A0E4C271"); } +#endif -#endif \ No newline at end of file +TEST(CrashReportingTest, CheckMergedCallstackOnAlternateStackWithHighAddresses) +{ + std::vector nativeFrames = { + // next two frames simulate signal handler runnin on alternate stack + {0x7F4DECDF2BC0, 0x7F478000ACE0, "__GI___wait4", 0x7F4DECDF2BC0, 0x7F4DECD1F000, false, ""}, + {0x7F4DECB882F0, 0x7F478000AD10, "PROCCreateCrashDump(std::vector >&, char*, int, bool)", 0x7F4DECB882F0, 0x7F4DEC514000, false, ""}, + // below managed before the signal handler + {0x7F4D778E1A7D, 0x7F476CC3E9D0, "/memfd:doublemapper (deleted)!+b84da7d", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""}, + {0x7F4D76593D2A, 0x7F476CC3EA10, "/memfd:doublemapper (deleted)!+a4ffd2a", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""}, + {0x7F4D6D7D3924, 0x7F476CC3EA90, "/usr/share/dotnet/shared/Microsoft.NETCore.App/9.0.10/System.Private.CoreLib.dll!+5e370b924", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""}, + }; + + std::vector managedFrames = { + {0x7F4D778E1A7D, 0x7F476CC3E9D0, "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""}, + {0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""}, + {0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""}, + }; + + auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames); + + std::vector expectedFunctions = { + "__GI___wait4", + "PROCCreateCrashDump(std::vector >&, char*, int, bool)", + "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", + "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", + "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", + }; + + ASSERT_EQ(mergedFrames.size(), expectedFunctions.size()); + for (size_t i = 0; i < mergedFrames.size(); i++) + { + ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]); + } +} + +TEST(CrashReportingTest, CheckMergedCallstackOnAlternateStackWithLowAddresses) +{ + std::vector nativeFrames = { + // next two frames simulate signal handler runnin on alternate stack + {0x7F4DECDF2BC0, 0x7F470000ACE0, "__GI___wait4", 0x7F4DECDF2BC0, 0x7F4DECD1F000, false, ""}, + {0x7F4DECB882F0, 0x7F470000AD10, "PROCCreateCrashDump(std::vector >&, char*, int, bool)", 0x7F4DECB882F0, 0x7F4DEC514000, false, ""}, + // below managed before the signal handler + {0x7F4D778E1A7D, 0x7F476CC3E9D0, "/memfd:doublemapper (deleted)!+b84da7d", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""}, + {0x7F4D76593D2A, 0x7F476CC3EA10, "/memfd:doublemapper (deleted)!+a4ffd2a", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""}, + {0x7F4D6D7D3924, 0x7F476CC3EA90, "/usr/share/dotnet/shared/Microsoft.NETCore.App/9.0.10/System.Private.CoreLib.dll!+5e370b924", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""}, + }; + + std::vector managedFrames = { + {0x7F4D778E1A7D, 0x7F476CC3E9D0, "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""}, + {0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""}, + {0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""}, + }; + + auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames); + + std::vector expectedFunctions = { + "__GI___wait4", + "PROCCreateCrashDump(std::vector >&, char*, int, bool)", + "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", + "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", + "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", + }; + + ASSERT_EQ(mergedFrames.size(), expectedFunctions.size()); + for (size_t i = 0; i < mergedFrames.size(); i++) + { + ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]); + } +} + +TEST(CrashReportingTest, CheckMergedCallstackButNoFusionBetweenNativeAndManaged) +{ + std::vector nativeFrames = { + {0x7F4DEC9B2982, 0x7F476CC39E90, "MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""}, + {0x7F4DEC9B3233, 0x7F476CC39F10, "WKS::gc_heap::mark_object_simple(unsigned char**)", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""}, + {0x7F4DEC9B7929, 0x7F476CC39F70, "WKS::gc_heap::mark_through_cards_helper(unsigned char**, unsigned long&, unsigned long&, void (*)(unsigned char**), unsigned char*, unsigned char*, int, int)", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""}, + }; + + std::vector managedFrames = { + {0x7F4D778E1A7D, 0x7F476CC3E9D0, "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""}, + {0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""}, + {0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""}, + }; + + auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames); + + std::vector expectedFunctions = { + "MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const", + "WKS::gc_heap::mark_object_simple(unsigned char**)", + "WKS::gc_heap::mark_through_cards_helper(unsigned char**, unsigned long&, unsigned long&, void (*)(unsigned char**), unsigned char*, unsigned char*, int, int)", + "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", + "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", + "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", + }; + + ASSERT_EQ(mergedFrames.size(), expectedFunctions.size()); + for (size_t i = 0; i < mergedFrames.size(); i++) + { + ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]); + } +} \ No newline at end of file From 0c5a3cdba9ec8d8661922745f07ea728aef95d80 Mon Sep 17 00:00:00 2001 From: Gregory LEOCADIE Date: Fri, 24 Oct 2025 11:36:04 +0200 Subject: [PATCH 2/4] Fix windows build --- .../Datadog.Profiler.Native.Tests.vcxproj | 1 + .../Datadog.Profiler.Native.Tests.vcxproj.filters | 3 +++ 2 files changed, 4 insertions(+) diff --git a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj index 6c2e7e51ad90..9e1cf1dfcc37 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj +++ b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj @@ -46,6 +46,7 @@ + diff --git a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters index 3c43e258d269..73d316176b7f 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters +++ b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters @@ -119,6 +119,9 @@ External Sources + + External Sources + External Sources From 85d4b76694191dc7bbd8786afb7c6ac107e41f65 Mon Sep 17 00:00:00 2001 From: Gregory LEOCADIE Date: Fri, 24 Oct 2025 14:01:11 +0200 Subject: [PATCH 3/4] improve performance --- .../CrashReporting.cpp | 10 +++---- .../CrashReportingTest.cpp | 27 ++++++++++--------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp index ed2229eb530f..690d9b8a8f8e 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp @@ -8,6 +8,8 @@ #include "unknwn.h" #include #include + +#include #include #ifdef _WIN32 @@ -234,7 +236,8 @@ int32_t CrashReporting::ResolveStacks(int32_t crashingThreadId, ResolveManagedCa } auto currentIsCrashingThread = threadId == crashingThreadId; - for (int i = 0; i < frames.size(); i++) + // GetThreadFrames returns the frames in reverse order, so we need to iterate in reverse + for (auto it = frames.rbegin(); it != frames.rend(); it++) { auto [frame, succeeded] = ExtractResult(ddog_crasht_StackFrame_new()); @@ -243,7 +246,7 @@ int32_t CrashReporting::ResolveStacks(int32_t crashingThreadId, ResolveManagedCa return 1; } - auto const& currentFrame = frames[i]; + auto const& currentFrame = *it; if (currentIsCrashingThread) { @@ -395,9 +398,6 @@ std::vector CrashReporting::MergeFrames(const std::vector expectedFunctions = { - "__GI___wait4", - "PROCCreateCrashDump(std::vector >&, char*, int, bool)", - "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", - "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", + "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", + "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", + "PROCCreateCrashDump(std::vector >&, char*, int, bool)", + "__GI___wait4", }; ASSERT_EQ(mergedFrames.size(), expectedFunctions.size()); @@ -198,11 +199,11 @@ TEST(CrashReportingTest, CheckMergedCallstackOnAlternateStackWithLowAddresses) auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames); std::vector expectedFunctions = { - "__GI___wait4", - "PROCCreateCrashDump(std::vector >&, char*, int, bool)", - "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", - "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", + "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", + "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", + "PROCCreateCrashDump(std::vector >&, char*, int, bool)", + "__GI___wait4", }; ASSERT_EQ(mergedFrames.size(), expectedFunctions.size()); @@ -229,12 +230,12 @@ TEST(CrashReportingTest, CheckMergedCallstackButNoFusionBetweenNativeAndManaged) auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames); std::vector expectedFunctions = { - "MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const", - "WKS::gc_heap::mark_object_simple(unsigned char**)", - "WKS::gc_heap::mark_through_cards_helper(unsigned char**, unsigned long&, unsigned long&, void (*)(unsigned char**), unsigned char*, unsigned char*, int, int)", - "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", - "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", + "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", + "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder+AsyncStateMachineBoxd__238>.MoveNext", + "WKS::gc_heap::mark_through_cards_helper(unsigned char**, unsigned long&, unsigned long&, void (*)(unsigned char**), unsigned char*, unsigned char*, int, int)", + "WKS::gc_heap::mark_object_simple(unsigned char**)", + "MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const", }; ASSERT_EQ(mergedFrames.size(), expectedFunctions.size()); From 7cd74d560466b477df355ff8fdae1d678ad80309 Mon Sep 17 00:00:00 2001 From: Gregory LEOCADIE Date: Fri, 24 Oct 2025 15:01:44 +0200 Subject: [PATCH 4/4] Still trying to fix windows profiler build job --- .../ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp index 690d9b8a8f8e..5cf6fd77d468 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp @@ -360,7 +360,7 @@ std::vector CrashReporting::MergeFrames(const std::vector result; // it's safe here to not use nativeFrames.size() + managedFrames.size() // because the managed frames should be a subset of the native frames - result.reserve(std::max(nativeFrames.size(), managedFrames.size())); + result.reserve((std::max)(nativeFrames.size(), managedFrames.size())); auto nativeIt = nativeFrames.rbegin(); auto managedIt = managedFrames.rbegin();