Porting g3log to qnx (#1) by rtimmaraju · Pull Request #551 · KjellKod/g3log

rtimmaraju · 2025-10-10T09:58:54Z

PULL REQUEST DESCRIPTION

Hi,

This contribution adds cross-compilation support for QNX operating system to g3log.

Motivation

QNX is an industry standard real-time operating system for embedded systems, particularly for vehicles, featuring safety focused features. It has also recently been made free for non-commercial use through the QNX Everywhere program, aiming for students, researchers and hobbyists to experiment with the OS.ported g3log which enables to run on QNX.
Note:
Backtrace was disabled from the g3log for QNX. As QNX already provides efficient native tools for crash and post-mortem debugging:
dumper — automatically collects crash dumps.
gdb — can be used to analyze these dumps with full symbol resolution.

How to build for qnx:
The build-files and instructions for cross-compiling g3log for QNX are present at
qnx-ports.
A free QNX8 license can be obtained here

Formatting

[ yes] I am following the formatting style of the existing codebase.

Testing

Windows ,Linux was tested and working with out issues.

QNX full command to install and build g3logtests:

INSTALL_G3LOG_TESTS="ON" ADD_G3LOG_UNIT_TEST="ON" make -C build-files/ports/g3log install JLEVEL=$(nproc) [INSTALL_ROOT_nto=PATH_TO_YOUR_STAGING_AREA USE_INSTALL_ROOT=true] VERBOSE=1

Test Reports for QNX are added here for referance.

Important

Add cross-compilation support for QNX to g3log, disabling backtrace and adjusting tests.

QNX Support:
- Add cross-compilation support for QNX in Build.cmake and CPackLists.txt.
- Disable backtrace functionality for QNX in crashhandler_unix.cpp.
Testing:
- Modify Test.cmake to exclude GoogleTest setup on QNX.
- Adjust test linking for QNX in Test.cmake.
Misc:
- Update installation instructions and test reports for QNX.

^{This description was created by}^{for d7d9da1. You can customize this summary. It will automatically update as commits are pushed.}

* g3log : ported and UT tested for g3log on QNX envionment * g3log: 7.1 compiled version * Removing the crashhandler from the QNX build and removed the tests having those dependencies * g3log:removing extra spaces and blank lines and adding qnx flag to the test_fatal testcases * g3log:refactoring the g3log changes * g3log:performance.cmake kept unchanged * disabled the stackdump from qnx * g3log:Removel of unwanted changes * g3log:review comments are sdressed * g3log:review comments are adressed2 * indentation corrected * Fix multiple issues - Fix intendation, formatting, styling, install locations - Add test install option - Remove unnecessary changes * Add back newline * Add install subdir for clarity --------- Co-authored-by: Ramya Timmaraju <rtimmaraju@CI0600000003641.rim.net> Co-authored-by: Deep Chordia <dchordia@blackberry.com>

ellipsis-dev

Caution

Changes requested ❌

Reviewed everything up to 09913cd in 1 minute and 23 seconds. Click for details.

Reviewed 121 lines of code in 4 files
Skipped 0 files when reviewing.
Skipped posting 4 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. CPackLists.txt:65

Draft comment:
Ensure that the variable 'tests_to_run' is defined when INSTALL_G3LOG_TESTS is enabled to avoid iterating over an empty list.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

2. src/crashhandler_unix.cpp:82

Draft comment:
Using a two-argument LogCapture call for QNX is acceptable if the overload exists. Verify that the alternative constructor (without the dump) behaves as intended on QNX.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

3. src/crashhandler_unix.cpp:146

Draft comment:
Returning an empty string for the stackdump on QNX is acceptable due to the lack of backtrace support. Consider logging or commenting that backtrace is intentionally skipped to aid future debugging.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

4. test_unit/Test.cmake:84

Draft comment:
The QNX conditional blocks for test setup and linking are clear. Ensure that the QNX variable is properly defined in the toolchain so these conditions are triggered as expected.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_roJKYXyCKUcLnxkv

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

Build.cmake

KjellKod · 2025-10-10T12:55:33Z

Thanks. I’ll take a look.
ideally for all supported platforms we have CI that supports that platform. I’ll see if that’s a possibility

KjellKod · 2025-10-14T13:51:30Z

test_unit/Test.cmake

   # 4. create the unit tests for g3log --- ONLY TESTED THE UNIT TEST ON LINUX
   # =========================
   IF (ADD_G3LOG_UNIT_TEST)
+      IF (NOT QNX)


please explain this. Without this wouldn't all tests be failing with QNX unless you had done a seperate manual install of google test

yes we will install the gtest seperately otherwise the build will not pass to generate testbinaries.

shall I get review for the latest commited changes.

I can take another look. Please also see my comments and see if you can address them

KjellKod · 2025-10-14T13:53:19Z

test_unit/Test.cmake

           set_target_properties(${test} PROPERTIES COMPILE_FLAGS "-isystem -pthread ")
        ENDIF( NOT(MSVC)) 
+        IF (QNX)
+        target_link_libraries(${test} g3log gtest gtest_main)


with removing gtest, this would also not work. It seems if I'm reading this correct that there are manual installation steps that are required but not documented yet. I assume that QNX doesn't have Google test suite available by default on the OS.

yes removing gtest will not build the tests , yes Manual installation needed , and steps are documented in the readme file of g3log under
# Clone the repos (for cloning the googletest )
# Build googletest (for building and installing in to sdp)

sorry, maybe I'm tired but I don't see this in my repo or in your pull request. Please provide a valid link

kindly check the readme file in https://github.com/qnx-ports/build-files/tree/main/ports/g3log.

How should someone who clones g3log know and find this information?

I mentioned this in the Pull request , but it will not be visible from the g3log download what will be the right location to keep such information, Readme file of g3log?

That I missed. That’s great and all I need to put up a CI that works for QNX too.

I’ve some time this weekend and if I can get it to work per your instructions then I’ll add a PR towards this PR

KjellKod · 2025-10-14T13:57:53Z

src/crashhandler_unix.cpp

         fatal_stream << "Received fatal signal: " << fatal_reason;
         fatal_stream << "(" << signal_number << ")\tPID: " << getpid() << std::endl;
         fatal_stream << "\n***** SIGNAL " << fatal_reason << "(signo= " << signal_number << " si_errno= " << info->si_errno << " si_code = " << info->si_code << ")" << std::endl;
+#if defined(__QNX__)


Remove this QNX special handling for this function.
You have already put in special handling in the function stackdump which for QNX generates and empty string. So always calling dump.c_str() is completely safe for QNX

KjellKod · 2025-10-14T14:03:21Z

src/crashhandler_unix.cpp

         if (nullptr != rawdump && !std::string(rawdump).empty()) {
            return {rawdump};
         }
+#if defined(__QNX__) // backtrace() and backtrace_symbols() are unavailable on QNX systems


this IS a clean, simple and localizedchange. The ifguard change is small and readable. However, I think a solution like this would be structurally be cleaner and also lend itself to other solutions. A tiny-tiny bit of boilerplate but it helps code readability.

std::string stackdump(const char* rawdump) { if (nullptr != rawdump && !std::string(rawdump).empty()) { return {rawdump}; } return stackdump_platform(); } #if defined(__QNX__) std::string stackdump_platform() { return ""; } #else std::string stackdump_platform() { // backtrace() logic } #endif

KjellKod · 2025-10-14T14:03:36Z

src/crashhandler_unix.cpp

         }  // END: for(size_t idx = 1; idx < size && messages != nullptr; ++idx)
         free(messages);
         return oss.str();
+#endif


with the solution suggested above this line would be removed

KjellKod · 2025-10-14T14:06:30Z

test_unit/Test.cmake

+       IF (NOT QNX)
       target_link_libraries(test_dynamic_loaded_shared_lib  ${G3LOG_LIBRARY} -ldl gtest_main)
+       ELSE()
+       target_link_libraries(test_dynamic_loaded_shared_lib  ${G3LOG_LIBRARY} gtest gtest_main)


This build will fail on QNX unless the user has manually installed gtest or otherwise been provided gtest targets. The download is prevented but there aren't any mechanisms or documentation that helps QNX users on. how to install it.

Developers on QNX will now need to:
• Manually install GoogleTest, and
• Ensure CMake can find it (find_package(GTest) or add_subdirectory from a local checkout).

That’s not covered in the PR, so it’s an incomplete platform adaptation.

KjellKod · 2025-10-14T14:10:42Z

test_unit/Test.cmake

+       IF (NOT QNX)
       target_link_libraries(test_dynamic_loaded_shared_lib  ${G3LOG_LIBRARY} -ldl gtest_main)
+       ELSE()
+       target_link_libraries(test_dynamic_loaded_shared_lib  ${G3LOG_LIBRARY} gtest gtest_main)


I think (but you would have to test it) that this whole section can be greatly simplified by

target_link_libraries(${test} g3log gtest_main) IF (QNX) target_link_libraries(${test} gtest) ENDIF()

KjellKod · 2025-10-14T14:15:39Z

CPackLists.txt

      FILES ${CMAKE_CURRENT_BINARY_DIR}/g3logConfig.cmake
      DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/g3log
   )
+   IF(INSTALL_G3LOG_TESTS)


This is a new option that is used but it isn't defined yet in Options.cmake.
The feature will fail silently nless someone defines it manually via -DINSTALL_G3LOG_TESTS=ON

I'd suggest pairing it locally with the options for test or the options for installing g3log in the Options.cmake file.

option(INSTALL_G3LOG_TESTS "Install g3log test executables (for developer environments)" OFF)

This installation needs to be validated also outside of QNX please.

Something like this maybe needs to go into Options.cmake (please validate)

# -DINSTALL_G3LOG_TESTS=ON : installs test binaries (into <prefix>/bin/g3log_tests) option(INSTALL_G3LOG_TESTS "Install g3log test executables (useful for CI or packaging)" OFF) IF(INSTALL_G3LOG_TESTS) message(STATUS "-DINSTALL_G3LOG_TESTS=ON\tInstalling g3log test executables") ELSE() message(STATUS "-DINSTALL_G3LOG_TESTS=OFF\tNot installing test executables") ENDIF()

KjellKod · 2025-10-14T14:16:46Z

@ellipsis please review this in terms of QNX for the github actions files. Are there docket images we can run to extend the CI matrix with QNX?

ellipsis-dev

Caution

Changes requested ❌

Reviewed everything up to d7d9da1 in 1 minute and 56 seconds. Click for details.

Reviewed 121 lines of code in 4 files
Skipped 0 files when reviewing.
Skipped posting 6 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. Build.cmake:106

Draft comment:
Good handling of QNX by excluding backtrace, but consider extracting the QNX check into a variable or helper macro to avoid duplicate platform checks in the codebase.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the suggestion to extract platform checks could improve maintainability, this specific instance doesn't show strong evidence of repeated QNX checks elsewhere in the file. The platform check here is specific to backtrace functionality. Creating a variable just for this one use might add unnecessary complexity. The comment is somewhat speculative about future duplicate platform checks. I might be overlooking potential future maintenance benefits. There could be other files in the codebase with QNX checks that would benefit from this extraction. However, without seeing evidence of duplicate QNX checks, suggesting extraction seems premature. We should follow YAGNI (You Aren't Gonna Need It) principles. The comment should be deleted as it makes a speculative suggestion about potential future code duplication without strong evidence of current need.

2. CPackLists.txt:65

Draft comment:
Installing test executables when INSTALL_G3LOG_TESTS is enabled is a useful addition. Ensure that the variable 'tests_to_run' is always defined when tests are enabled.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

3. src/crashhandler_unix.cpp:82

Draft comment:
The conditional use of LogCapture without the dump parameter on QNX is appropriate. Consider centralizing QNX platform checks to reduce duplicated ifdef logic across files.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment makes a valid point about code organization and maintainability. The QNX-specific code is scattered in multiple places. However, each ifdef is handling a distinct concern (header inclusion, log capture params, stackdump implementation). Centralizing these may not actually improve the code since they deal with different aspects. The comment is more of a general suggestion rather than a specific issue requiring change. The comment could be considered informative rather than actionable. It's not clear exactly what code changes are being requested or how the centralization should be implemented. While code organization could be improved, the current structure with separate ifdefs for distinct concerns may actually be more maintainable than trying to centralize unrelated QNX-specific code. The comment should be deleted as it's more of an informative observation than a specific actionable change request. The current code organization, while containing multiple QNX checks, is logically structured.

4. src/crashhandler_unix.cpp:146

Draft comment:
Returning an empty string in the stackdump function for QNX is appropriate given the unavailability of backtrace functions. Document this behavior for clarity.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

5. test_unit/Test.cmake:26

Draft comment:
Skipping the googletest download when QNX is detected is acceptable. Ensure that in QNX environments either gtest is provided or an alternative approach is documented.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50% None

6. test_unit/Test.cmake:44

Draft comment:
It looks like there might be a typographical error on this line. In CMake, the closing directive for an "if" block is typically written as endif() (case-insensitive) without extra condition information. Consider revising ENDIF(NOT QNX) to endif() to avoid potential confusion.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about CMake best practices, the codebase consistently uses the ENDIF(condition) style throughout. Changing just one instance would make the code less consistent. Style changes should be consistent across the codebase. This kind of minor style suggestion doesn't warrant a comment in a PR review. The comment is technically correct about CMake conventions. Maybe consistency with CMake standards is more important than internal consistency? While CMake standards are important, making isolated style changes creates inconsistency. A style change like this should be done across the entire codebase in a dedicated cleanup PR. Delete the comment. The suggested style change would create inconsistency with the rest of the file's established style.

Workflow ID: wflow_iGRk23xvYkeR2bl2

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev · 2025-10-14T14:18:56Z

test_unit/Test.cmake

        IF( NOT(MSVC))
           set_target_properties(${test} PROPERTIES COMPILE_FLAGS "-isystem -pthread ")
        ENDIF( NOT(MSVC)) 
+        IF (QNX)


Linking tests differently for QNX (using 'gtest' and 'gtest_main' separately) is clear; however, consider refactoring the linking logic to avoid duplication and ensure consistency across platforms.

^{This comment was generated because it violated a code review rule: mrule_YnsKy285VtuNWQg5.}

KjellKod · 2025-10-20T22:51:31Z

fwiw, this is not forgotten, just handling a lot of different priorities this week.

KjellKod · 2025-12-01T17:53:48Z

@rtimmaraju please address the comments I've added

KjellKod · 2026-04-11T19:19:32Z

this seems abandoned. I'd appreciate if anyone in the community would give me a 👍 or 👎 regarding how we should proceed. I've no problem adding the missing CI environment and tests but since I'm not using it myself with QNX I'd rather know someone is before making the effort

ellipsis-dev bot reviewed Oct 10, 2025

View reviewed changes

Build.cmake Outdated Show resolved Hide resolved

added CMAKE_SYSTEM_NAME instead of Using QNX directly

d7d9da1

KjellKod self-assigned this Oct 14, 2025

KjellKod reviewed Oct 14, 2025

View reviewed changes

KjellKod requested changes Oct 14, 2025

View reviewed changes

ellipsis-dev bot reviewed Oct 14, 2025

View reviewed changes

adding seperate method stackdump_platform to improve readability

456fcc9

Uh oh!

Conversation

rtimmaraju commented Oct 10, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PULL REQUEST DESCRIPTION

Formatting

Testing

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KjellKod commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtimmaraju Oct 14, 2025 • edited by KjellKod Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KjellKod commented Oct 14, 2025

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

KjellKod commented Oct 20, 2025

Uh oh!

KjellKod commented Dec 1, 2025

Uh oh!

KjellKod commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtimmaraju commented Oct 10, 2025 •

edited by ellipsis-dev bot

Loading

rtimmaraju Oct 14, 2025 •

edited by KjellKod

Loading