Skip to content

Commit 3dd9e0b

Browse files
gleocadiegleocadie
authored
[crashtracker] Fix native and managed callstacks merge (#7703)
1 parent 05622b1 commit 3dd9e0b

File tree

5 files changed

+147
-22
lines changed

5 files changed

+147
-22
lines changed

‎profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.cpp‎

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
#include "unknwn.h"
99
#include <shared/src/native-src/string.h>
1010
#include <shared/src/native-src/util.h>
11+
12+
#include <algorithm>
1113
#include <thread>
1214

1315
#ifdef _WIN32
@@ -234,7 +236,8 @@ int32_t CrashReporting::ResolveStacks(int32_t crashingThreadId, ResolveManagedCa
234236
}
235237

236238
auto currentIsCrashingThread = threadId == crashingThreadId;
237-
for (int i = 0; i < frames.size(); i++)
239+
// GetThreadFrames returns the frames in reverse order, so we need to iterate in reverse
240+
for (auto it = frames.rbegin(); it != frames.rend(); it++)
238241
{
239242
auto [frame, succeeded] = ExtractResult(ddog_crasht_StackFrame_new());
240243

@@ -243,7 +246,7 @@ int32_t CrashReporting::ResolveStacks(int32_t crashingThreadId, ResolveManagedCa
243246
return 1;
244247
}
245248

246-
auto const& currentFrame = frames[i];
249+
auto const& currentFrame = *it;
247250

248251
if (currentIsCrashingThread)
249252
{
@@ -355,41 +358,44 @@ int32_t CrashReporting::ExportImpl(ddog_Endpoint* endpoint)
355358
std::vector<StackFrame> CrashReporting::MergeFrames(const std::vector<StackFrame>& nativeFrames, const std::vector<StackFrame>& managedFrames)
356359
{
357360
std::vector<StackFrame> result;
358-
result.reserve(std::max(nativeFrames.size(), managedFrames.size()));
361+
// it's safe here to not use nativeFrames.size() + managedFrames.size()
362+
// because the managed frames should be a subset of the native frames
363+
result.reserve((std::max)(nativeFrames.size(), managedFrames.size()));
359364

360-
size_t i = 0, j = 0;
361-
while (i < nativeFrames.size() && j < managedFrames.size())
365+
auto nativeIt = nativeFrames.rbegin();
366+
auto managedIt = managedFrames.rbegin();
367+
while (nativeIt != nativeFrames.rend() && managedIt != managedFrames.rend())
362368
{
363-
if (nativeFrames[i].sp < managedFrames[j].sp)
369+
if (nativeIt->sp > managedIt->sp)
364370
{
365-
result.push_back(nativeFrames[i]);
366-
++i;
371+
result.push_back(*nativeIt);
372+
++nativeIt;
367373
}
368-
else if (managedFrames[j].sp < nativeFrames[i].sp)
374+
else if (managedIt->sp > nativeIt->sp)
369375
{
370-
result.push_back(managedFrames[j]);
371-
++j;
376+
result.push_back(*managedIt);
377+
++managedIt;
372378
}
373379
else
374380
{ // frames[i].sp == managedFrames[j].sp
375381
// Prefer managedFrame when sp values are the same
376-
result.push_back(managedFrames[j]);
377-
++i;
378-
++j;
382+
result.push_back(*managedIt);
383+
++nativeIt;
384+
++managedIt;
379385
}
380386
}
381387

382388
// Add any remaining frames that are left in either vector
383-
while (i < nativeFrames.size())
389+
while (nativeIt != nativeFrames.rend())
384390
{
385-
result.push_back(nativeFrames[i]);
386-
++i;
391+
result.push_back(*nativeIt);
392+
++nativeIt;
387393
}
388394

389-
while (j < managedFrames.size())
395+
while (managedIt != managedFrames.rend())
390396
{
391-
result.push_back(managedFrames[j]);
392-
++j;
397+
result.push_back(*managedIt);
398+
++managedIt;
393399
}
394400

395401
return result;

‎profiler/src/ProfilerEngine/Datadog.Profiler.Native/CrashReporting.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ class CrashReporting : public ICrashReporting
143143
virtual std::vector<StackFrame> GetThreadFrames(int32_t tid, ResolveManagedCallstack resolveManagedCallstack, void* context) = 0;
144144
virtual std::string GetSignalInfo(int32_t signal) = 0;
145145

146+
#ifdef DD_TEST
147+
public:
148+
#endif
146149
static std::vector<StackFrame> MergeFrames(const std::vector<StackFrame>& nativeFrames, const std::vector<StackFrame>& managedFrames);
147150
private:
148151
int32_t ExportImpl(ddog_Endpoint* endpoint);

‎profiler/test/Datadog.Profiler.Native.Tests/CrashReportingTest.cpp‎

Lines changed: 114 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,20 @@
11
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
22
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc.
33

4+
#include "gtest/gtest.h"
5+
6+
#include "unknwn.h"
7+
8+
const IID IID_IUnknown = {0x00000000,
9+
0x0000,
10+
0x0000,
11+
{0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46}};
12+
13+
#include "CrashReporting.h"
14+
415
#ifdef _WIN32
516

617
#include "resource.h"
7-
#include "gtest/gtest.h"
818
#include <string_view>
919
#include <windows.h>
1020
#include <vector>
@@ -130,5 +140,107 @@ TEST(CrashReportingTest, ExtractPdbSignaturePE64)
130140
// | Pdb signature |Age|
131141
ASSERT_STRCASEEQ(buildIdStr.data(), "C465AFCDBDBC58A0100995839A0E4C271");
132142
}
143+
#endif
133144

134-
#endif
145+
TEST(CrashReportingTest, CheckMergedCallstackOnAlternateStackWithHighAddresses)
146+
{
147+
std::vector<StackFrame> nativeFrames = {
148+
// next two frames simulate signal handler runnin on alternate stack
149+
{0x7F4DECDF2BC0, 0x7F478000ACE0, "__GI___wait4", 0x7F4DECDF2BC0, 0x7F4DECD1F000, false, ""},
150+
{0x7F4DECB882F0, 0x7F478000AD10, "PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)", 0x7F4DECB882F0, 0x7F4DEC514000, false, ""},
151+
// below managed before the signal handler
152+
{0x7F4D778E1A7D, 0x7F476CC3E9D0, "/memfd:doublemapper (deleted)!<unknown>+b84da7d", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
153+
{0x7F4D76593D2A, 0x7F476CC3EA10, "/memfd:doublemapper (deleted)!<unknown>+a4ffd2a", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
154+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "/usr/share/dotnet/shared/Microsoft.NETCore.App/9.0.10/System.Private.CoreLib.dll!<unknown>+5e370b924", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
155+
};
156+
157+
std::vector<StackFrame> managedFrames = {
158+
{0x7F4D778E1A7D, 0x7F476CC3E9D0, "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>+AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__238<System.__Canon>>.MoveNext", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
159+
{0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
160+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
161+
};
162+
163+
// MergeFrames returns the frames in the order of the sp addresses
164+
auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames);
165+
166+
std::vector<std::string> expectedFunctions = {
167+
"System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart",
168+
"System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch",
169+
"System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>+AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__238<System.__Canon>>.MoveNext",
170+
"PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)",
171+
"__GI___wait4",
172+
};
173+
174+
ASSERT_EQ(mergedFrames.size(), expectedFunctions.size());
175+
for (size_t i = 0; i < mergedFrames.size(); i++)
176+
{
177+
ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]);
178+
}
179+
}
180+
181+
TEST(CrashReportingTest, CheckMergedCallstackOnAlternateStackWithLowAddresses)
182+
{
183+
std::vector<StackFrame> nativeFrames = {
184+
// next two frames simulate signal handler runnin on alternate stack
185+
{0x7F4DECDF2BC0, 0x7F470000ACE0, "__GI___wait4", 0x7F4DECDF2BC0, 0x7F4DECD1F000, false, ""},
186+
{0x7F4DECB882F0, 0x7F470000AD10, "PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)", 0x7F4DECB882F0, 0x7F4DEC514000, false, ""},
187+
// below managed before the signal handler
188+
{0x7F4D778E1A7D, 0x7F476CC3E9D0, "/memfd:doublemapper (deleted)!<unknown>+b84da7d", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
189+
{0x7F4D76593D2A, 0x7F476CC3EA10, "/memfd:doublemapper (deleted)!<unknown>+a4ffd2a", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
190+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "/usr/share/dotnet/shared/Microsoft.NETCore.App/9.0.10/System.Private.CoreLib.dll!<unknown>+5e370b924", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
191+
};
192+
193+
std::vector<StackFrame> managedFrames = {
194+
{0x7F4D778E1A7D, 0x7F476CC3E9D0, "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>+AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__238<System.__Canon>>.MoveNext", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
195+
{0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
196+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
197+
};
198+
199+
auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames);
200+
201+
std::vector<std::string> expectedFunctions = {
202+
"System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart",
203+
"System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch",
204+
"System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>+AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__238<System.__Canon>>.MoveNext",
205+
"PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)",
206+
"__GI___wait4",
207+
};
208+
209+
ASSERT_EQ(mergedFrames.size(), expectedFunctions.size());
210+
for (size_t i = 0; i < mergedFrames.size(); i++)
211+
{
212+
ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]);
213+
}
214+
}
215+
216+
TEST(CrashReportingTest, CheckMergedCallstackButNoFusionBetweenNativeAndManaged)
217+
{
218+
std::vector<StackFrame> nativeFrames = {
219+
{0x7F4DEC9B2982, 0x7F476CC39E90, "MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
220+
{0x7F4DEC9B3233, 0x7F476CC39F10, "WKS::gc_heap::mark_object_simple(unsigned char**)", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
221+
{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, ""},
222+
};
223+
224+
std::vector<StackFrame> managedFrames = {
225+
{0x7F4D778E1A7D, 0x7F476CC3E9D0, "System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>+AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__238<System.__Canon>>.MoveNext", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
226+
{0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
227+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
228+
};
229+
230+
auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames);
231+
232+
std::vector<std::string> expectedFunctions = {
233+
"System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart",
234+
"System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch",
235+
"System.Private.CoreLib.dll!System.Runtime.CompilerServices.AsyncTaskMethodBuilder<System.Threading.Tasks.VoidTaskResult>+AsyncStateMachineBox<Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__238<System.__Canon>>.MoveNext",
236+
"WKS::gc_heap::mark_through_cards_helper(unsigned char**, unsigned long&, unsigned long&, void (*)(unsigned char**), unsigned char*, unsigned char*, int, int)",
237+
"WKS::gc_heap::mark_object_simple(unsigned char**)",
238+
"MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const",
239+
};
240+
241+
ASSERT_EQ(mergedFrames.size(), expectedFunctions.size());
242+
for (size_t i = 0; i < mergedFrames.size(); i++)
243+
{
244+
ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]);
245+
}
246+
}

‎profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\ETW\IpcServer.cpp" />
4747
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\ETW\ProfilerLogger.cpp" />
4848
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\SecurityDescriptorHelpers.cpp" />
49+
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native\CrashReporting.cpp" />
4950
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native\DogstatsdService.cpp" />
5051
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\OsSpecificApi.cpp" />
5152
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\SystemTime.cpp" />

‎profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,9 @@
119119
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\OsSpecificApi.cpp">
120120
<Filter>External Sources</Filter>
121121
</ClCompile>
122+
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native\CrashReporting.cpp">
123+
<Filter>External Sources</Filter>
124+
</ClCompile>
122125
<ClCompile Include="..\..\src\ProfilerEngine\Datadog.Profiler.Native.Windows\SystemTime.cpp">
123126
<Filter>External Sources</Filter>
124127
</ClCompile>

0 commit comments

Comments
 (0)