Skip to content

Commit a83e850

Browse files
gleocadiegleocadie
committed
Fix native and managed callstacks merge
1 parent d0fe312 commit a83e850

File tree

3 files changed

+139
-19
lines changed

3 files changed

+139
-19
lines changed

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

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -355,43 +355,49 @@ int32_t CrashReporting::ExportImpl(ddog_Endpoint* endpoint)
355355
std::vector<StackFrame> CrashReporting::MergeFrames(const std::vector<StackFrame>& nativeFrames, const std::vector<StackFrame>& managedFrames)
356356
{
357357
std::vector<StackFrame> result;
358+
// it's safe here to not use nativeFrames.size() + managedFrames.size()
359+
// because the managed frames should be a subset of the native frames
358360
result.reserve(std::max(nativeFrames.size(), managedFrames.size()));
359361

360-
size_t i = 0, j = 0;
361-
while (i < nativeFrames.size() && j < managedFrames.size())
362+
auto nativeIt = nativeFrames.rbegin();
363+
auto managedIt = managedFrames.rbegin();
364+
while (nativeIt != nativeFrames.rend() && managedIt != managedFrames.rend())
362365
{
363-
if (nativeFrames[i].sp < managedFrames[j].sp)
366+
if (nativeIt->sp > managedIt->sp)
364367
{
365-
result.push_back(nativeFrames[i]);
366-
++i;
368+
result.push_back(*nativeIt);
369+
++nativeIt;
367370
}
368-
else if (managedFrames[j].sp < nativeFrames[i].sp)
371+
else if (managedIt->sp > nativeIt->sp)
369372
{
370-
result.push_back(managedFrames[j]);
371-
++j;
373+
result.push_back(*managedIt);
374+
++managedIt;
372375
}
373376
else
374377
{ // frames[i].sp == managedFrames[j].sp
375378
// Prefer managedFrame when sp values are the same
376-
result.push_back(managedFrames[j]);
377-
++i;
378-
++j;
379+
result.push_back(*managedIt);
380+
++nativeIt;
381+
++managedIt;
379382
}
380383
}
381384

382385
// Add any remaining frames that are left in either vector
383-
while (i < nativeFrames.size())
386+
while (nativeIt != nativeFrames.rend())
384387
{
385-
result.push_back(nativeFrames[i]);
386-
++i;
388+
result.push_back(*nativeIt);
389+
++nativeIt;
387390
}
388391

389-
while (j < managedFrames.size())
392+
while (managedIt != managedFrames.rend())
390393
{
391-
result.push_back(managedFrames[j]);
392-
++j;
394+
result.push_back(*managedIt);
395+
++managedIt;
393396
}
394397

398+
// we could also return the merged callstack without reversing but the caller would have to walk backwards
399+
std::reverse(result.begin(), result.end());
400+
395401
return result;
396402
}
397403

‎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: 113 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,106 @@ 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+
auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames);
164+
165+
std::vector<std::string> expectedFunctions = {
166+
"__GI___wait4",
167+
"PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)",
168+
"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",
169+
"System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch",
170+
"System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart",
171+
};
172+
173+
ASSERT_EQ(mergedFrames.size(), expectedFunctions.size());
174+
for (size_t i = 0; i < mergedFrames.size(); i++)
175+
{
176+
ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]);
177+
}
178+
}
179+
180+
TEST(CrashReportingTest, CheckMergedCallstackOnAlternateStackWithLowAddresses)
181+
{
182+
std::vector<StackFrame> nativeFrames = {
183+
// next two frames simulate signal handler runnin on alternate stack
184+
{0x7F4DECDF2BC0, 0x7F470000ACE0, "__GI___wait4", 0x7F4DECDF2BC0, 0x7F4DECD1F000, false, ""},
185+
{0x7F4DECB882F0, 0x7F470000AD10, "PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)", 0x7F4DECB882F0, 0x7F4DEC514000, false, ""},
186+
// below managed before the signal handler
187+
{0x7F4D778E1A7D, 0x7F476CC3E9D0, "/memfd:doublemapper (deleted)!<unknown>+b84da7d", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
188+
{0x7F4D76593D2A, 0x7F476CC3EA10, "/memfd:doublemapper (deleted)!<unknown>+a4ffd2a", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
189+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "/usr/share/dotnet/shared/Microsoft.NETCore.App/9.0.10/System.Private.CoreLib.dll!<unknown>+5e370b924", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
190+
};
191+
192+
std::vector<StackFrame> managedFrames = {
193+
{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, ""},
194+
{0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
195+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
196+
};
197+
198+
auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames);
199+
200+
std::vector<std::string> expectedFunctions = {
201+
"__GI___wait4",
202+
"PROCCreateCrashDump(std::vector<char const*, std::allocator<char const*> >&, char*, int, bool)",
203+
"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",
204+
"System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch",
205+
"System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart",
206+
};
207+
208+
ASSERT_EQ(mergedFrames.size(), expectedFunctions.size());
209+
for (size_t i = 0; i < mergedFrames.size(); i++)
210+
{
211+
ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]);
212+
}
213+
}
214+
215+
TEST(CrashReportingTest, CheckMergedCallstackButNoFusionBetweenNativeAndManaged)
216+
{
217+
std::vector<StackFrame> nativeFrames = {
218+
{0x7F4DEC9B2982, 0x7F476CC39E90, "MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const", 0x7F4D778E1A7D, 0x7F4D6C094000, false, ""},
219+
{0x7F4DEC9B3233, 0x7F476CC39F10, "WKS::gc_heap::mark_object_simple(unsigned char**)", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
220+
{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, ""},
221+
};
222+
223+
std::vector<StackFrame> managedFrames = {
224+
{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, ""},
225+
{0x7F4D76593D2A, 0x7F476CC3EA10, "System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch", 0x7F4D76593D2A, 0x7F4D6C094000, false, ""},
226+
{0x7F4D6D7D3924, 0x7F476CC3EA90, "System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart", 0x7F4D6D7D3924, 0x7F478A0C8000, false, ""},
227+
};
228+
229+
auto mergedFrames = CrashReporting::MergeFrames(nativeFrames, managedFrames);
230+
231+
std::vector<std::string> expectedFunctions = {
232+
"MethodTable::GetFlag(MethodTable::WFLAGS_HIGH_ENUM) const",
233+
"WKS::gc_heap::mark_object_simple(unsigned char**)",
234+
"WKS::gc_heap::mark_through_cards_helper(unsigned char**, unsigned long&, unsigned long&, void (*)(unsigned char**), unsigned char*, unsigned char*, int, int)",
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+
"System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch",
237+
"System.Private.CoreLib.dll!System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart",
238+
};
239+
240+
ASSERT_EQ(mergedFrames.size(), expectedFunctions.size());
241+
for (size_t i = 0; i < mergedFrames.size(); i++)
242+
{
243+
ASSERT_EQ(mergedFrames[i].method, expectedFunctions[i]);
244+
}
245+
}

0 commit comments

Comments
 (0)