From 7624daa9049fd0307133fc882aee68c8d44bb3f4 Mon Sep 17 00:00:00 2001 From: "Marco Antonio J. Costa" Date: Fri, 31 Jul 2026 13:25:11 -0300 Subject: [PATCH] delete the stack tracer that never captured a frame ENABLE_STACK_TRACE has been 0 for as long as the file has been here, so StackTrace's constructor captured nothing and every line it ever wrote to stack_trace.log was a bare message with an empty frame list behind it. The DbgHelp singleton underneath it resolved symbols for that empty list, and the game linked dbghelp.lib to do it. The crash reports cover what this was meant to cover, and the VFS errors that were its only real content are already in vfs.log and game_log.log. Drop the tracer, the log, and the dbghelp dependency. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Opus 5 --- CMakeLists.txt | 1 - sgp/CMakeLists.txt | 1 - sgp/DEBUG.cpp | 2 - sgp/debug_util.cpp | 26 ----- sgp/debug_util.h | 44 +-------- sgp/debug_win_util.cpp | 213 ----------------------------------------- sgp/sgp.cpp | 16 +--- 7 files changed, 5 insertions(+), 298 deletions(-) delete mode 100644 sgp/debug_util.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 10ce1b4d7..ede067867 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,7 +137,6 @@ endif() set(Ja2_Libraries "${binkw32_lib}" "${CMAKE_SOURCE_DIR}/libexpatMT.lib" -"dbghelp.lib" "winmm.lib" "ws2_32.lib" "winhttp.lib" diff --git a/sgp/CMakeLists.txt b/sgp/CMakeLists.txt index 35db8757d..f7a91c16f 100644 --- a/sgp/CMakeLists.txt +++ b/sgp/CMakeLists.txt @@ -5,7 +5,6 @@ set(sgpSrc "${CMAKE_CURRENT_SOURCE_DIR}/crash_telemetry.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/Cursor Control.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/DEBUG.cpp" -"${CMAKE_CURRENT_SOURCE_DIR}/debug_util.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/debug_win_util.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/DirectDraw Calls.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/DirectX Common.cpp" diff --git a/sgp/DEBUG.cpp b/sgp/DEBUG.cpp index c18b7bbc4..5a71dc8c4 100644 --- a/sgp/DEBUG.cpp +++ b/sgp/DEBUG.cpp @@ -397,8 +397,6 @@ void _FailMessage(const char* message, unsigned lineNum, const char * functionNa // everything below it (a stack walk, a save, a screen) can die trying. sgp::raiseAssertException(lineNum, sourceFileName, message); - sgp::dumpStackTrace(message); - mprintf( 10, 10, L"%s: %s %s", pMessageStrings[ MSG_VERSION ], zProductLabel, zBuildInformation ); std::stringstream basicInformation; diff --git a/sgp/debug_util.cpp b/sgp/debug_util.cpp deleted file mode 100644 index 7afcfcf4a..000000000 --- a/sgp/debug_util.cpp +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "debug_util.h" - -const void *const *StackTrace::Addresses(size_t* count) { - *count = trace_.size(); - if (trace_.size()) - return &trace_[0]; - return NULL; -} - -void sgp::dumpStackTrace(vfs::String const& msg) -{ - // the first error is the important one anyway - static bool already_dumping = false; - - if(!already_dumping) // needs a mutes to be sure - { - already_dumping = true; - StackTrace str; - str.PrintBacktrace(msg.utf8().c_str()); - already_dumping = false; - } -} diff --git a/sgp/debug_util.h b/sgp/debug_util.h index cfd741457..9eed9ed66 100644 --- a/sgp/debug_util.h +++ b/sgp/debug_util.h @@ -2,50 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// This is a cross platform interface for helper functions related to debuggers. -// You should use this to test if you're running under a debugger, and if you -// would like to yield (breakpoint) into the debugger. +// Crash reporting: capture a fault (or an assertion) as a report file, and hand +// the pending reports to the telemetry uploader at startup. #ifndef BASE_DEBUG_UTIL_H_ #define BASE_DEBUG_UTIL_H_ -#include -#include - -#include "sgp_logger.h" - -// A macro to disallow the copy constructor and operator= functions -// This should be used in the private: declarations for a class -#define DISALLOW_COPY_AND_ASSIGN(TypeName) \ - TypeName(const TypeName&); \ - void operator=(const TypeName&) - -// An older, deprecated, politically incorrect name for the above. -#define DISALLOW_EVIL_CONSTRUCTORS(TypeName) DISALLOW_COPY_AND_ASSIGN(TypeName) - -// A stacktrace can be helpful in debugging. For example, you can include a -// stacktrace member in a object (probably around #ifndef NDEBUG) so that you -// can later see where the given object was created from. -class StackTrace { -public: - // Create a stacktrace from the current location - StackTrace(); - // Get an array of instruction pointer values. - // count: (output) the number of elements in the returned array - const void *const *Addresses(size_t* count); - // Print a backtrace to stderr - void PrintBacktrace(const char* msg); - - // Resolve backtrace to symbols and write to stream. - void OutputToStream(const char* msg, sgp::Logger::LogInstance* os); - -private: - std::vector trace_; - int count_; - - DISALLOW_EVIL_CONSTRUCTORS(StackTrace); -}; - struct _EXCEPTION_POINTERS; // Assertion failures raise this software exception so they get the same crash @@ -55,8 +17,6 @@ struct _EXCEPTION_POINTERS; namespace sgp { - void dumpStackTrace(vfs::String const& msg); - // Raise SGP_EXCEPTION_ASSERT and swallow it again, so the vectored crash // handler writes a report for an assertion that never faults. Carries the // assert's line, file and message (NULL for a plain Assert) as exception diff --git a/sgp/debug_win_util.cpp b/sgp/debug_win_util.cpp index abc286fb0..711b2d78a 100644 --- a/sgp/debug_win_util.cpp +++ b/sgp/debug_win_util.cpp @@ -11,204 +11,11 @@ #include -#include #include // PEB loader list, so the module table costs no allocation -#include -// The arraysize(arr) macro returns the # of elements in an array arr. -// The expression is a compile-time constant, and therefore can be -// used in defining new arrays, for example. If you use arraysize on -// a pointer by mistake, you will get a compile-time error. -// -// One caveat is that arraysize() doesn't accept any array of an -// anonymous type or a type defined inside a function. In these rare -// cases, you have to use the unsafe ARRAYSIZE_UNSAFE() macro below. This is -// due to a limitation in C++'s template system. The limitation might -// eventually be removed, but it hasn't happened yet. - -// This template function declaration is used in defining arraysize. -// Note that the function doesn't need an implementation, as we only -// use its type. -template -char (&ArraySizeHelper(T (&array)[N]))[N]; - -// That gcc wants both of these prototypes seems mysterious. VC, for -// its part, can't decide which to use (another mystery). Matching of -// template overloads: the final frontier. -#ifndef _MSC_VER -template -char (&ArraySizeHelper(const T (&array)[N]))[N]; -#endif - -#define arraysize(array) (sizeof(ArraySizeHelper(array))) - - -namespace { - // SymbolContext is a threadsafe singleton that wraps the DbgHelp Sym* family - // of functions. The Sym* family of functions may only be invoked by one - // thread at a time. SymbolContext code may access a symbol server over the - // network while holding the lock for this singleton. In the case of high - // latency, this code will adversly affect performance. - // - // There is also a known issue where this backtrace code can interact - // badly with breakpad if breakpad is invoked in a separate thread while - // we are using the Sym* functions. This is because breakpad does now - // share a lock with this function. See this related bug: - // - // http://code.google.com/p/google-breakpad/issues/detail?id=311 - // - // This is a very unlikely edge case, and the current solution is to - // just ignore it. - class SymbolContext { - public: - static SymbolContext* Get() { - static SymbolContext* s_singleton = NULL; - if(!s_singleton) s_singleton = new SymbolContext(); - return s_singleton; - // We use a leaky singleton because code may call this during process - // termination. - - //return Singleton >::get(); - } - - // Returns the error code of a failed initialization. - DWORD init_error() const { - return init_error_; - } - - // For the given trace, attempts to resolve the symbols, and output a trace - // to the ostream os. The format for each line of the backtrace is: - // - // SymbolName[0xAddress+Offset] (FileName:LineNo) - // - // This function should only be called if Init() has been called. We do not - // LOG(FATAL) here because this code is called might be triggered by a - // LOG(FATAL) itself. - void OutputTraceToStream(const std::vector& trace, sgp::Logger::LogInstance* os) { - //AutoLock lock(lock_); - - for (size_t i = 0; (i < trace.size()); ++i) { - const int kMaxNameLength = 256; - DWORD_PTR frame = reinterpret_cast(trace[i]); - - // Code adapted from MSDN example: - // http://msdn.microsoft.com/en-us/library/ms680578(VS.85).aspx - ULONG64 buffer[ - (sizeof(SYMBOL_INFO) + - kMaxNameLength * sizeof(wchar_t) + - sizeof(ULONG64) - 1) / - sizeof(ULONG64)]; - - // Initialize symbol information retrieval structures. - DWORD64 sym_displacement = 0; - PSYMBOL_INFO symbol = reinterpret_cast(&buffer[0]); - symbol->SizeOfStruct = sizeof(SYMBOL_INFO); - symbol->MaxNameLen = kMaxNameLength; - BOOL has_symbol = SymFromAddr(GetCurrentProcess(), frame, - &sym_displacement, symbol); - - // Attempt to retrieve line number information. - DWORD line_displacement = 0; - IMAGEHLP_LINE64 line = {}; - line.SizeOfStruct = sizeof(IMAGEHLP_LINE64); - BOOL has_line = SymGetLineFromAddr64(GetCurrentProcess(), frame, - &line_displacement, &line); - - // Output the backtrace line. - (*os) << " "; - if (has_symbol) - { - (*os) << " [0x" << trace[i] << "+" << sym_displacement << "]\t"; - (*os) << vfs::String(symbol->Name); - } - else - { - (*os) << " [0x" << trace[i] << "]\t"; - // If there is no symbol information, add a spacer. - (*os) << " (No symbol)"; - } - if (has_line) - { - (*os) << " - (" << line.FileName << ":" << line.LineNumber << ")"; - } - (*os) << sgp::endl; - } - (*os) << sgp::endl; - } - - private: - SymbolContext() : init_error_(ERROR_SUCCESS) { - // Initializes the symbols for the process. - // Defer symbol load until they're needed, use undecorated names, and - // get line numbers. - SymSetOptions(SYMOPT_DEFERRED_LOADS | - SYMOPT_UNDNAME | - SYMOPT_LOAD_LINES); - if (SymInitialize(GetCurrentProcess(), NULL, TRUE)) { - init_error_ = ERROR_SUCCESS; - } else { - __debugbreak(); - init_error_ = GetLastError(); - } - } - - DWORD init_error_; - //Lock lock_; - DISALLOW_COPY_AND_ASSIGN(SymbolContext); - }; - -} // namespace - -#define ENABLE_STACK_TRACE 0 - -StackTrace::StackTrace() { - // From http://msdn.microsoft.com/en-us/library/bb204633(VS.85).aspx, - // the sum of FramesToSkip and FramesToCapture must be less than 63, - // so set it to 62. - const int kMaxCallers = 62; - // TODO(ajwong): Migrate this to StackWalk64. - -#if ENABLE_STACK_TRACE - - // WANNE: This only works with Visual Studio version >= 2008 - #if _MSC_VER >= 1500 - - void* callers[kMaxCallers]; - int count = CaptureStackBackTrace(0, kMaxCallers, callers, NULL); - - // Not used, because we use CaptureStackBackTrace() - //int count = RtlCaptureStackBackTrace(0, kMaxCallers, callers, NULL); - - if (count > 0) { - trace_.resize(count); - memcpy(&trace_[0], callers, sizeof(callers[0]) * count); - } - else - { - trace_.resize(0); - } - - #endif - -#endif -} - -static struct StackTraceLog { - sgp::Logger_ID id; - StackTraceLog() { - id = sgp::Logger::instance().createLogger(); - sgp::Logger::instance().connectFile(id, L"stack_trace.log", false, sgp::Logger::FLUSH_ON_ENDL); - }; -} s_log; - -void StackTrace::PrintBacktrace(const char* msg) { - sgp::Logger::LogInstance log = SGP_LOG(s_log.id); - OutputToStream(msg, &log); -} - // Called first-chance from the vectored crash handler, so it runs *inside* the // faulting thread with a possibly-wrecked heap: the very crash we want to report // is often heap corruption (a trashed std::map, a wild pointer). Anything that @@ -460,24 +267,4 @@ void writeExceptionBacktrace(_EXCEPTION_POINTERS* ep) { } } // namespace sgp -void StackTrace::OutputToStream(const char* msg, sgp::Logger::LogInstance* os) { - SymbolContext* context = SymbolContext::Get(); - DWORD error = context->init_error(); - if (error != ERROR_SUCCESS) - { - (*os) << "Error initializing symbols (" << error - << "). Dumping unresolved backtrace:" - << sgp::endl; - for (size_t i = 0; (i < trace_.size()); ++i) - { - (*os) << "\t" << trace_[i] << sgp::endl; - } - } - else - { - (*os) << L"Backtrace: " << (msg != NULL ? msg : "") << sgp::endl; - context->OutputTraceToStream(trace_, os); - } -} - #endif // _MSC_VER diff --git a/sgp/sgp.cpp b/sgp/sgp.cpp index afad417de..539d6bfb4 100644 --- a/sgp/sgp.cpp +++ b/sgp/sgp.cpp @@ -636,27 +636,18 @@ static vfs::String getGameID() class VfsLogAdapter : public vfs::Aspects::ILogger { public: - VfsLogAdapter(sgp::Logger_ID ID, bool stacktrace = false) : _id(ID), _trace(stacktrace) {}; + VfsLogAdapter(sgp::Logger_ID ID) : _id(ID) {}; virtual void Msg(const wchar_t* msg) { SGP_LOG(_id, msg); - if(_trace) - { - sgp::dumpStackTrace(msg); - } } virtual void Msg(const char* msg) { SGP_LOG(_id, msg); - if(_trace) - { - sgp::dumpStackTrace(msg); - } } private: sgp::Logger_ID _id; - bool _trace; }; //#include @@ -751,10 +742,9 @@ int PASCAL WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR pCommandL sgp::Logger::instance().connectFile(VFS_LOG, L"vfs.log", false, sgp::Logger::FLUSH_ON_DELETE); - VfsLogAdapter* vfslog = new VfsLogAdapter(VFS_LOG, false); - VfsLogAdapter* vfslog_error = new VfsLogAdapter(VFS_LOG, true); + VfsLogAdapter* vfslog = new VfsLogAdapter(VFS_LOG); - vfs::Aspects::setLogger(vfslog, vfslog, vfslog_error, NULL /* vfslog */); + vfs::Aspects::setLogger(vfslog, vfslog, vfslog, NULL /* vfslog */); // Make sure that only one instance of this application is running at once // // Look for prev instance by searching for the window