ctrace initial development - #2548
Conversation
Test Results 2 files - 39 14 suites - 111 11m 45s ⏱️ - 6m 46s Results for commit 6fbf7f9. ± Comparison against base commit fba299a. This pull request removes 454 tests.♻️ This comment has been updated with latest results. |
jreineckearm
left a comment
There was a problem hiding this comment.
Just had a look at the first few files in this PR (the GH workflows). You may want to review those first. I think some of them will impact more than ctrace.
Also, why do you make workflows sensitive to changes in subfolders of external? I don't think GitHub actions are clever enough to detect changes in git submodules. But I might be wrong.
edriouk
left a comment
There was a problem hiding this comment.
all ctrace* project files must have Header File directory => changes in CMakeLists.txt files are required:
SET(PROJMGR_HEADER_FILES ...
edriouk
left a comment
There was a problem hiding this comment.
All classes and their public methods must have DOXYGEN-style comments
|
test/data folders contain *.license files. They probably should be removed. |
Simplify diagnostics and trace metadata handling, remove stale APIs and schema entries, and align output selection behavior. Refresh platform test support, documentation, and release-facing validation.
jreineckearm
left a comment
There was a problem hiding this comment.
Light-weight review of all but the ctrace test subfolder. See comments.
Overall massive improvement since initial push. No more systematic flaws like the LCOV excludes or use of #pragma once.
|
|
||
| - name: Archive unit tests results | ||
| # Keep the report available when test execution fails. | ||
| if: always() && (matrix.arch != 'arm64') |
There was a problem hiding this comment.
Just learnt what always() is: makes a step execute even if previous ones failed.
I think both "run" and "archive" steps should have it or remove it. As is, "run" has it removed, "archive" has it and depends on "run" results.
| endif() | ||
|
|
||
| set_property(TARGET opencsd_options PROPERTY INTERFACE_COMPILE_OPTIONS) | ||
| target_compile_options(opencsd PRIVATE /EHsc /W0) |
There was a problem hiding this comment.
Maybe we want to show some warnings? /W0 hides them all. I think msvc has couple of /W levels/
| @@ -0,0 +1,288 @@ | |||
| # ctrace Architecture | |||
There was a problem hiding this comment.
maybe split internal engineering docs from later end user docs, i.e. introduce a folder internal-docs or developer-docs.
| #include <stdexcept> | ||
|
|
||
| /** @brief Provides checked saturating arithmetic for trace timestamps. */ | ||
| class SaturatingArithmetic final { |
| @@ -0,0 +1,123 @@ | |||
| # ctrace | |||
There was a problem hiding this comment.
Good for now, consider split into README ("landing page" on GH) and developer docs.
| apply_patch(${CMAKE_CURRENT_SOURCE_DIR}/external/xerces-c) | ||
| apply_patch(${CMAKE_CURRENT_SOURCE_DIR}/external/yaml-cpp) | ||
|
|
||
| # OpenCSD is a private build dependency of ctrace. |
There was a problem hiding this comment.
Would probably do just to call it a build dependency unless "private" has a special meaning here (which I didn't get if so).
| |Google C++ Testing Framework|1.11.0|[BSD-3-Clause](https://opensource.org/licenses/BSD-3-Clause)|https://github.com/google/googletest.git| Testing | | ||
| |cxxopts|2.2.1|[MIT](https://opensource.org/licenses/MIT)|https://github.com/jarro2783/cxxopts.git| packgen | | ||
| |yaml-cpp|0.7.0|[MIT](https://opensource.org/licenses/MIT)|https://github.com/jbeder/yaml-cpp.git| packgen | | ||
| |cxxopts|3.0.0|[MIT](https://opensource.org/licenses/MIT)|https://github.com/jarro2783/cxxopts.git| packgen, ctrace (statically incorporated) | |
There was a problem hiding this comment.
We didn't have the parts in parenthesis before. Probably should keep it that way, i.e. remove them. Same for the next two entries.
|
|
||
| <!-- markdownlint-restore --> | ||
|
|
||
| Dependency modifications incorporated into ctrace are documented in its |
There was a problem hiding this comment.
I thought you removed dependency modifications for ctrace? Would remove the entire paragraph.
| @@ -0,0 +1,54 @@ | |||
| # Copyright (c) 2026 Arm Limited. All rights reserved. | |||
There was a problem hiding this comment.
Is cmake used to run tests? How do we do this for other tools in the repo?
| |Google C++ Testing Framework|1.11.0|[BSD-3-Clause](https://opensource.org/licenses/BSD-3-Clause)|https://github.com/google/googletest.git| Testing | | ||
| |cxxopts|2.2.1|[MIT](https://opensource.org/licenses/MIT)|https://github.com/jarro2783/cxxopts.git| packgen | | ||
| |yaml-cpp|0.7.0|[MIT](https://opensource.org/licenses/MIT)|https://github.com/jbeder/yaml-cpp.git| packgen | | ||
| |cxxopts|3.0.0|[MIT](https://opensource.org/licenses/MIT)|https://github.com/jarro2783/cxxopts.git| packgen, ctrace | |
There was a problem hiding this comment.
Existing dependencies seem to be updated to newer versions. One even a major version change. Does this also apply to the other libraries/tools? And have they been successfully tested?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2548 +/- ##
==========================================
+ Coverage 68.34% 71.71% +3.36%
==========================================
Files 141 185 +44
Lines 26511 30117 +3606
Branches 16022 17769 +1747
==========================================
+ Hits 18119 21598 +3479
- Misses 6090 6214 +124
- Partials 2302 2305 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Implements
ctraceimplementation for decoding SWO streams containing ITM and DWT data.Limitations
Checklist