Make sure we call the previous terminate handle if present when we intercept terminate calls (#514)

Improvement: Make sure we call the previous terminate handle if present when we intercept terminate calls
Improvement: Avoid allocating memory for call stack in terminate handle and assert callback

7 files changed, 125 insertions, 23 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2b71f3f0a..5583482cd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -50,6 +50,8 @@
 - Improvement: Chunk block generation for `zen oplog-export` and `zen builds upload` command are now limited to max 4000 chunks per block
 - Improvement: Added additional validation of compact binary objects when reading from disk/receiving from client
 - Improvement: If blocks metadata is missing for a download `--verbose` will enable output of all the missing chunk block hashes
+- Improvement: Make sure we call the previous terminate handle if present when we intercept terminate calls
+- Improvement: Avoid allocating memory for call stack in terminate handle and assert callback
 - Feature: Options `--allow-partial-block-requests` for `zen builds download` command has been augmented with two new modes, `zencacheonly` and `mixed`. Defaults to `mixed`.
   - `false` only full block requests allowed
   - `mixed`  multiple partial block ranges requests per block allowed to zen cache, single partial block range request per block to host
diff --git a/src/zencore/callstack.cpp b/src/zencore/callstack.cpp
index b22f2ec1f..8aa1111bf 100644
--- a/src/zencore/callstack.cpp
+++ b/src/zencore/callstack.cpp
@@ -210,6 +210,54 @@ CallstackToString(const CallstackFrames* Callstack, std::string_view Prefix)
 	return SB.ToString();
 }
 
+void
+CallstackToStringRaw(const CallstackFrames* Callstack, void* CallbackUserData, CallstackRawCallback Callback)
+{
+	if (Callstack && Callstack->FrameCount > 0)
+	{
+#if ZEN_PLATFORM_WINDOWS
+		char		 SymbolBuffer[sizeof(SYMBOL_INFO) + 1024];
+		SYMBOL_INFO* SymbolInfo							  = (SYMBOL_INFO*)SymbolBuffer;
+		SymbolInfo->SizeOfStruct						  = sizeof(SYMBOL_INFO);
+		SymbolInfo->MaxNameLen							  = 1023;
+		DWORD64								 Displacement = 0;
+		fmt::basic_memory_buffer<char, 2048> Message;
+		for (uint32_t FrameIndex = 0; FrameIndex < Callstack->FrameCount; FrameIndex++)
+		{
+			if (WinSymbols.GetSymbol(Callstack->Frames[FrameIndex], SymbolInfo, Displacement))
+			{
+				auto Appender = fmt::appender(Message);
+				fmt::format_to(Appender, "{}+{:#x} [{:#x}]", SymbolInfo->Name, Displacement, (uintptr_t)Callstack->Frames[FrameIndex]);
+				Message.push_back('\0');
+				Callback(CallbackUserData, FrameIndex, Message.data());
+				Message.resize(0);
+			}
+		}
+#endif
+#if ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
+		char** messages = backtrace_symbols(Callstack->Frames, (int)Callstack->FrameCount);
+		if (messages)
+		{
+			for (uint32_t FrameIndex = 0; FrameIndex < Callstack->FrameCount; FrameIndex++)
+			{
+				Callback(CallbackUserData, FrameIndex, messages[FrameIndex]);
+			}
+			free(messages);
+		}
+#endif
+	}
+}
+
+CallstackFrames*
+GetCallstackRaw(void* CaptureBuffer, int FramesToSkip, int FramesToCapture)
+{
+	CallstackFrames* Callstack = (CallstackFrames*)CaptureBuffer;
+
+	Callstack->Frames	  = (void**)&Callstack[1];
+	Callstack->FrameCount = GetCallstack(FramesToSkip, FramesToCapture, Callstack->Frames);
+	return Callstack;
+}
+
 #if ZEN_WITH_TESTS
 
 TEST_CASE("Callstack.Basic")
diff --git a/src/zencore/include/zencore/callstack.h b/src/zencore/include/zencore/callstack.h
index ef4ba0e91..ca8171435 100644
--- a/src/zencore/include/zencore/callstack.h
+++ b/src/zencore/include/zencore/callstack.h
@@ -32,6 +32,18 @@ GetFrameSymbols(const CallstackFrames* Callstack)
 void		FormatCallstack(const CallstackFrames* Callstack, StringBuilderBase& SB, std::string_view Prefix);
 std::string CallstackToString(const CallstackFrames* Callstack, std::string_view Prefix = {});
 
+typedef void (*CallstackRawCallback)(void* UserData, uint32_t FrameIndex, const char* FrameText);
+
+constexpr size_t
+CallstackRawMemorySize(int FramesToSkip, int FramesToCapture)
+{
+	return sizeof(CallstackFrames) + sizeof(void*) * (FramesToSkip + FramesToCapture);
+}
+
+void CallstackToStringRaw(const CallstackFrames* Callstack, void* CallbackUserData, CallstackRawCallback Callback);
+
+CallstackFrames* GetCallstackRaw(void* CaptureBuffer, int FramesToSkip, int FramesToCapture);
+
 void callstack_forcelink();	 // internal
 
 }  // namespace zen
diff --git a/src/zencore/include/zencore/zencore.h b/src/zencore/include/zencore/zencore.h
index d21c0e7e2..b5eb3e3e8 100644
--- a/src/zencore/include/zencore/zencore.h
+++ b/src/zencore/include/zencore/zencore.h
@@ -58,11 +58,11 @@ struct AssertImpl
 		[[noreturn]] (const char* Filename, int LineNumber, const char* FunctionName, const char* Msg);
 
 	virtual void ZEN_FORCENOINLINE ZEN_DEBUG_SECTION
-	OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, CallstackFrames* Callstack);
+	OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, const CallstackFrames* Callstack);
 
 protected:
 	static void ZEN_FORCENOINLINE ZEN_DEBUG_SECTION ThrowAssertException
-		[[noreturn]] (const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, CallstackFrames* Callstack);
+		[[noreturn]] (const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, const CallstackFrames* Callstack);
 	static AssertImpl* CurrentAssertImpl;
 	static AssertImpl  DefaultAssertImpl;
 	AssertImpl*		   NextAssertImpl = nullptr;
diff --git a/src/zencore/sentryintegration.cpp b/src/zencore/sentryintegration.cpp
index fef4cd8ed..00e67dc85 100644
--- a/src/zencore/sentryintegration.cpp
+++ b/src/zencore/sentryintegration.cpp
@@ -37,8 +37,11 @@ namespace {
 
 struct SentryAssertImpl : zen::AssertImpl
 {
-	virtual void ZEN_FORCENOINLINE ZEN_DEBUG_SECTION
-	OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, zen::CallstackFrames* Callstack) override;
+	virtual void ZEN_FORCENOINLINE ZEN_DEBUG_SECTION OnAssert(const char*				  Filename,
+															  int						  LineNumber,
+															  const char*				  FunctionName,
+															  const char*				  Msg,
+															  const zen::CallstackFrames* Callstack) override;
 };
 
 class sentry_sink final : public spdlog::sinks::base_sink<spdlog::details::null_mutex>
@@ -107,7 +110,11 @@ sentry_sink::flush_()
 }
 
 void
-SentryAssertImpl::OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, zen::CallstackFrames* Callstack)
+SentryAssertImpl::OnAssert(const char*				   Filename,
+						   int						   LineNumber,
+						   const char*				   FunctionName,
+						   const char*				   Msg,
+						   const zen::CallstackFrames* Callstack)
 {
 	// Sentry will provide its own callstack
 	ZEN_UNUSED(Callstack);
diff --git a/src/zencore/zencore.cpp b/src/zencore/zencore.cpp
index 5a6232318..8ca1ad8c0 100644
--- a/src/zencore/zencore.cpp
+++ b/src/zencore/zencore.cpp
@@ -116,10 +116,10 @@ AssertImpl::~AssertImpl()
 void
 AssertImpl::ExecAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg)
 {
-	void*	 Frames[8];
-	uint32_t FrameCount = GetCallstack(2, 8, Frames);
-
-	CallstackFrames* Callstack = CreateCallstack(FrameCount, Frames);
+	constexpr int	 SkipFrameCount = 2;
+	constexpr int	 FrameCount		= 8;
+	uint8_t			 CallstackBuffer[CallstackRawMemorySize(SkipFrameCount, FrameCount)];
+	CallstackFrames* Callstack = GetCallstackRaw(&CallstackBuffer[0], SkipFrameCount, FrameCount);
 
 	AssertImpl* AssertImpl = CurrentAssertImpl;
 	while (AssertImpl)
@@ -137,7 +137,7 @@ AssertImpl::ExecAssert(const char* Filename, int LineNumber, const char* Functio
 	ThrowAssertException(Filename, LineNumber, FunctionName, Msg, Callstack);
 }
 void
-AssertImpl::OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, CallstackFrames* Callstack)
+AssertImpl::OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, const CallstackFrames* Callstack)
 {
 	ZEN_UNUSED(FunctionName);
 
@@ -152,11 +152,11 @@ AssertImpl::OnAssert(const char* Filename, int LineNumber, const char* FunctionN
 }
 
 void
-AssertImpl::ThrowAssertException(const char*	  Filename,
-								 int			  LineNumber,
-								 const char*	  FunctionName,
-								 const char*	  Msg,
-								 CallstackFrames* Callstack)
+AssertImpl::ThrowAssertException(const char*			Filename,
+								 int					LineNumber,
+								 const char*			FunctionName,
+								 const char*			Msg,
+								 const CallstackFrames* Callstack)
 {
 	ZEN_UNUSED(FunctionName);
 	fmt::basic_memory_buffer<char, 2048> Message;
@@ -164,7 +164,7 @@ AssertImpl::ThrowAssertException(const char*	  Filename,
 	fmt::format_to(Appender, "{}({}): {}", Filename, LineNumber, Msg);
 	Message.push_back('\0');
 
-	throw AssertException(Message.data(), Callstack);
+	throw AssertException(Message.data(), CloneCallstack(Callstack));
 }
 
 void refcount_forcelink();
@@ -308,7 +308,7 @@ TEST_CASE("Assert.Custom")
 	struct MyAssertImpl : AssertImpl
 	{
 		virtual void ZEN_FORCENOINLINE ZEN_DEBUG_SECTION
-		OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, CallstackFrames* Callstack)
+		OnAssert(const char* Filename, int LineNumber, const char* FunctionName, const char* Msg, const CallstackFrames* Callstack)
 		{
 			ZEN_UNUSED(Callstack);
 			AssertFileName = Filename;
diff --git a/src/zenutil/logging.cpp b/src/zenutil/logging.cpp
index 8ff58ee73..806b96d52 100644
--- a/src/zenutil/logging.cpp
+++ b/src/zenutil/logging.cpp
@@ -42,6 +42,8 @@ InitializeLogging(const LoggingOptions& LogOptions)
 	FinishInitializeLogging(LogOptions);
 }
 
+static std::terminate_handler OldTerminateHandler = nullptr;
+
 void
 BeginInitializeLogging(const LoggingOptions& LogOptions)
 {
@@ -99,12 +101,43 @@ BeginInitializeLogging(const LoggingOptions& LogOptions)
 		}
 	}
 
-	std::set_terminate([]() {
-		void*			 Frames[8];
-		uint32_t		 FrameCount = GetCallstack(2, 8, Frames);
-		CallstackFrames* Callstack	= CreateCallstack(FrameCount, Frames);
-		ZEN_CRITICAL("Program exited abnormally via std::terminate()\n{}", CallstackToString(Callstack, "  "));
-		FreeCallstack(Callstack);
+	OldTerminateHandler = std::set_terminate([]() {
+		try
+		{
+			constexpr int	 SkipFrameCount = 4;
+			constexpr int	 FrameCount		= 8;
+			uint8_t			 CallstackBuffer[CallstackRawMemorySize(SkipFrameCount, FrameCount)];
+			CallstackFrames* Callstack = GetCallstackRaw(&CallstackBuffer[0], SkipFrameCount, FrameCount);
+
+			fmt::basic_memory_buffer<char, 2048> Message;
+			auto								 Appender = fmt::appender(Message);
+			fmt::format_to(Appender, "Program exited abnormally via std::terminate()");
+
+			if (Callstack->FrameCount > 0)
+			{
+				fmt::format_to(Appender, "\n");
+
+				CallstackToStringRaw(Callstack, &Message, [](void* UserData, uint32_t FrameIndex, const char* FrameText) {
+					ZEN_UNUSED(FrameIndex);
+					fmt::basic_memory_buffer<char, 2048>* Message  = (fmt::basic_memory_buffer<char, 2048>*)UserData;
+					auto								  Appender = fmt::appender(*Message);
+					fmt::format_to(Appender, "  {}\n", FrameText);
+				});
+			}
+			Message.push_back('\0');
+
+			// We use direct ZEN_LOG here instead of ZEN_ERROR as we don't care about *this* code location in the log
+			ZEN_LOG(Log(), zen::logging::level::Critical, "{}", Message.data());
+			zen::logging::FlushLogging();
+		}
+		catch (const std::exception&)
+		{
+			// Ignore any exceptions in terminate callback
+		}
+		if (OldTerminateHandler)
+		{
+			OldTerminateHandler();
+		}
 	});
 
 	// Default