Port ITT API Reference Collector to cross-platform Windows and Linux build#231
Port ITT API Reference Collector to cross-platform Windows and Linux build#231alexey-kireev wants to merge 20 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Ports the ITT API reference collector to build and run on both Windows and Linux, adding an automated smoke test to validate basic logging output.
Changes:
- Add CMake/buildall support for building
ittnotify_refcol(and an optional smoke-test executable). - Improve cross-platform behavior in the reference collector implementation (localtime handling, Windows wide-char entrypoints).
- Add CI smoke test job and update README build/usage instructions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
src/ittnotify_refcol/tests/smoke_test.c |
Adds a small executable that exercises task + metadata APIs to produce log output. |
src/ittnotify_refcol/itt_refcol_impl.c |
Adds cross-platform localtime handling and Windows wide-char API entrypoints. |
src/ittnotify_refcol/README.md |
Documents CMake-based build and Windows usage for the reference collector. |
buildall.py |
Adds CLI flags to enable building the reference collector and smoke tests via CMake. |
CMakeLists.txt |
Introduces CMake option/targets for the reference collector shared library and smoke tests. |
.github/workflows/main.yml |
Adds a new CI job to build and run the reference collector smoke test on Linux/Windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set_target_properties(ittnotify_refcol PROPERTIES | ||
| LINKER_LANGUAGE C | ||
| RUNTIME_OUTPUT_DIRECTORY ${LIBRARY_OUTPUT_PATH} | ||
| ARCHIVE_OUTPUT_DIRECTORY ${LIBRARY_OUTPUT_PATH}) |
| sprintf(log_file_name,"libittnotify_refcol_%d%d%d%d%d%d.log", | ||
| time_info->tm_year+1900, time_info->tm_mon+1, time_info->tm_mday, | ||
| time_info->tm_hour, time_info->tm_min, time_info->tm_sec); |
| @@ -85,6 +109,10 @@ static void ref_collector_init() | |||
| { | |||
| sprintf(file_name_buffer,"%s\\%s", temp_dir, log_file); | |||
| sprintf(file_name_buffer,"%s", log_file); | ||
| } | ||
| #else | ||
| sprintf(file_name_buffer,"/tmp/%s", log_file); |
eparshut
left a comment
There was a problem hiding this comment.
I guess we could remove old Makefile as unused
| - name: Checkout sources | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| - name: Build reference collector library and smoke test | ||
| run: python buildall.py --force_bits 64 --refcol -DITT_API_REFCOL_SMOKE_TESTS=ON ${{ runner.os == 'Windows' && '--cmake_gen ninja' || '' }} |
There was a problem hiding this comment.
This is a bad idea with pass-through -D cmake option hack.
Let's split flow into two steps: buildall.py --refcol builds only the refcol library; then in CI call cmake --build <dir> --target refcol_smoke_test builds the test explicitly.
- sprintf -> snprintf with buffer size limits (overflow protection) - Fixed 64-bit format specifiers: %lu/%ld -> PRIu64/PRId64 - Fixed size_t format: %lu -> %zu - Fixed ref_collector_release signature for atexit compatibility - Suppressed MSVC deprecation warnings via _CRT_SECURE_NO_WARNINGS
eparshut
left a comment
There was a problem hiding this comment.
I guess you can move this PR from draft into ready state
there are a few things left to do:
- add PR description
- polish reference collector readme
- update main repository readme
- update official documentation in /docs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/ittnotify_refcol/itt_refcol_impl.c:321
get_metadata_elementsincrementsoffsetby the return value ofsnprintf, butsnprintfreturns the number of bytes that would have been written, which may be larger than the remaining buffer space. OnceoffsetexceedsLOG_BUFFER_MAX_SIZE, subsequentLOG_BUFFER_MAX_SIZE - offsetbecomes negative/huge and can lead to out-of-bounds writes. Also,mallocisn't checked before*metadata_str = '\0'. Consider checking allocation and clamping/stopping appends when the buffer fills (or dynamically grow the buffer).
static char* get_metadata_elements(size_t size, __itt_metadata_type type, void* metadata)
{
char* metadata_str = malloc(sizeof(char) * LOG_BUFFER_MAX_SIZE);
*metadata_str = '\0';
uint16_t offset = 0;
switch (type)
{
case __itt_metadata_u64:
for (uint16_t i = 0; i < size; i++)
offset += snprintf(metadata_str + offset, LOG_BUFFER_MAX_SIZE - offset, "%" PRIu64 ";", ((uint64_t*)metadata)[i]);
src/ittnotify_refcol/itt_refcol_impl.c:855
__itt_bind_context_metadata_to_counterhas the sameoffset += snprintf(...)issue asget_metadata_elements:snprintfcan return a value larger than the remaining space, causingoffsetto exceedLOG_BUFFER_MAX_SIZEand subsequent appends to use an invalid size. Clampoffset/ stop appending once the buffer is full.
char context_metadata[LOG_BUFFER_MAX_SIZE];
context_metadata[0] = '\0';
uint16_t offset = 0;
for(size_t i=0; i<length; i++)
{
char* context_metadata_element = get_context_metadata_element(metadata[i].type, metadata[i].value);
offset += snprintf(context_metadata + offset, LOG_BUFFER_MAX_SIZE - offset, "%s", context_metadata_element);
free(context_metadata_element);
| run: python buildall.py --force_bits 64 --refcol ${{ runner.os == 'Windows' && '--cmake_gen ninja' || '' }} | ||
| - name: Build smoke test | ||
| run: | | ||
| cmake ${{ matrix.build_dir }} -DITT_API_REFCOL_SMOKE_TESTS=ON |
No description provided.