-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Resolve Linux Warning & Improve Windows Debugging #13274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a9288eb
to
b497872
Compare
02df0d3
to
ea76744
Compare
ad3f1d5
to
8542024
Compare
7bc7217
to
b72cf4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors platform-specific initialization code and improves cross-platform signal handling, particularly enhancing Windows debugging capabilities. The changes consolidate scattered platform setup code into a centralized Platform
module and modernize signal handling with better Windows support.
- Centralizes platform-specific initialization into
Platform::setupPreApp()
andPlatform::setupPostApp()
- Implements Windows signal handling using console control handlers alongside existing POSIX signal handling
- Removes unused return value warnings by adding explicit void casts
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/main.cc | Consolidates platform setup calls and removes scattered initialization code |
src/Utilities/Platform.h | Adds new platform setup API with pre/post app initialization functions |
src/Utilities/Platform.cc | Implements centralized platform setup including Windows error handling and debugging |
src/Utilities/SignalHandler.h | Modernizes signal handler with Windows support and atomic operations |
src/Utilities/SignalHandler.cc | Implements cross-platform signal handling with Windows console control handlers |
src/Utilities/QGCLogging.h | Simplifies logging handler installation API |
src/Utilities/QGCLogging.cc | Removes Windows-specific CRT report hook (moved to Platform) |
src/Utilities/CMakeLists.txt | Updates build conditions to include SignalHandler on all desktop platforms |
src/RunGuard.h | Adds copy constructor disabling for better safety |
src/QGCApplication.h | Adds logging category declaration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
g_prevUef = SetUnhandledExceptionFilter(WindowsUnhandledExceptionFilter); | ||
|
||
#if defined(_MSC_VER) | ||
(void) _set_invalid_parameter_handler(WindowsInvalidParameterHandler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WindowsInvalidParameterHandler function is empty, which may not provide meaningful error handling. Consider adding logging or appropriate error handling logic.
Copilot uses AI. Check for mistakes.
return; | ||
} | ||
const char b = 1; | ||
[[maybe_unused]] const ssize_t n = ::write(self->_sigIntFd[1], &b, sizeof(b)); // no logging from signal handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write operation in the signal handler could fail, but there's no way to report this error since logging isn't safe in signal handlers. Consider documenting this limitation or adding a fallback mechanism.
[[maybe_unused]] const ssize_t n = ::write(self->_sigIntFd[1], &b, sizeof(b)); // no logging from signal handler | |
// Only async-signal-safe functions allowed; cannot log here. | |
// If write fails, the signal will not be delivered, so exit immediately as a fallback. | |
const ssize_t n = ::write(self->_sigIntFd[1], &b, sizeof(b)); | |
if (n != sizeof(b)) { | |
_exit(128 + SIGINT); // Fallback: exit with signal code | |
} |
Copilot uses AI. Check for mistakes.
if (!self) { | ||
return; | ||
} | ||
const char b = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the SIGINT handler, the write operation could fail but cannot be reported. This is a limitation of signal-safe programming that should be documented.
const char b = 1; | |
const char b = 1; | |
// Signal-safe programming limitation: the write operation below could fail, | |
// but errors cannot be reported or handled safely in a signal handler. | |
// This is an accepted limitation and should be documented. |
Copilot uses AI. Check for mistakes.
const DWORD code = (ep && ep->ExceptionRecord) ? ep->ExceptionRecord->ExceptionCode : 0; | ||
wchar_t buf[128] = {}; | ||
#if defined(_MSC_VER) | ||
(void) _snwprintf_s(buf, _TRUNCATE, L"QGC: unhandled SEH 0x%08lX\n", static_cast<unsigned long>(code)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer size (128) is hardcoded. Consider using a named constant or sizeof(buf) to make the code more maintainable and less error-prone.
(void) _snwprintf_s(buf, _TRUNCATE, L"QGC: unhandled SEH 0x%08lX\n", static_cast<unsigned long>(code)); | |
(void) _snwprintf_s(buf, sizeof(buf)/sizeof(buf[0]), L"QGC: unhandled SEH 0x%08lX\n", static_cast<unsigned long>(code)); |
Copilot uses AI. Check for mistakes.
There were some unused return values with no discard
Also implements setWindowsNativeFunctions for some basic windows debugging functions
Includes #13306