Fix ipc_read tool offsets on 64-bit targets#17
Merged
Conversation
The tool hardcoded the seq_lock layout assuming the data buffer always starts at CACHELINE_BYTES from the mapping base. That holds on 32-bit ARM (i.MX) where size_t is 4 bytes and the header packs into exactly one cacheline. On 64-bit targets (Jetson aarch64, x86_64) size_t is 8 bytes and the compiler inserts 4 bytes of padding between seq and size, shifting the real entry offset to 36 (aarch64) or 68 (x86_64). This caused all printed fields to be off by 4 bytes. Compute the seq, size and entry offsets with offsetof(seq_lock<char>, ...) so they match the actual struct layout on every target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous fix used offsetof(seq_lock<char>, entry), which gives the offset right after padding[] assuming 1-byte alignment for STORAGE. That works for std_msgs::Float32 (4-byte aligned, lands at offset 36 on aarch64) but not for geometry_msgs::Vector3 (8-byte aligned, entry sits at offset 40 because the compiler rounds up). Symptom: reading Vector3 with 'ddd' produced 0,0,0 because the tool read at offset 36 — 4 zero bytes of inter-field padding plus 20 bytes of real data, losing the last 4 bytes of z. Fix: compute storage_align from the format string (max alignment of any format char) and round offsetof(probe_lock, entry) up to it. Refactor: pull format-string discovery (CLI flag or debug tailer) above the read loop so the entry offset is known before mmap is addressed. Drop the duplicate tailer parse inside the loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
johannesschrimpf
approved these changes
Jun 11, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates tyndall_tool_ipc_read to compute the seq_lock<STORAGE> field offsets using offsetof(...) rather than hard-coded 32-bit assumptions, so the tool reads shared-memory payloads correctly on 64-bit targets (aarch64/x86_64) as well as armv7.
Changes:
- Use
offsetof(seq_lock<char>, seq/size/entry)to locateseqandsizefields reliably across ABIs. - Compute the payload (
entry) start offset using inferredSTORAGEalignment (from the format string) instead of assuming it begins atCACHELINE_BYTES. - Refactor format-string recovery so the tool can determine
entryalignment before reading/printing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+174
to
+192
| auto format_char_align = [](char c) -> size_t { | ||
| switch (c) { | ||
| case 'd': // double | ||
| case 'l': // long | ||
| case 'm': // unsigned long | ||
| case 'x': // long long | ||
| case 'y': // unsigned long long | ||
| return 8; | ||
| case 'f': // float | ||
| case 'i': // int | ||
| case 'j': // unsigned int | ||
| return 4; | ||
| case 's': // short | ||
| case 't': // unsigned short | ||
| return 2; | ||
| default: // b(bool), c(char), h(unsigned char) | ||
| return 1; | ||
| } | ||
| }; |
Comment on lines
+210
to
+226
| } else { | ||
| constexpr int type_info_hash_size = 4; | ||
| const size_t debug_tail_size = ipc_size - *data_size; | ||
| if (debug_tail_size > (size_t)type_info_hash_size) { | ||
| const size_t debug_format_size = debug_tail_size - type_info_hash_size; | ||
| char *const debug_format_loc = static_cast<char *>(mapped) + ipc_size - | ||
| debug_format_size + CACHELINE_BYTES + 24; | ||
| std::string candidate(debug_format_loc); | ||
| auto valid = !candidate.empty() && | ||
| std::all_of(candidate.begin(), candidate.end(), [&](char c) { | ||
| return std::find(allowed.begin(), allowed.end(), c) != | ||
| allowed.end(); | ||
| }); | ||
| if (valid) | ||
| fmt_string = candidate; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
tyndall_tool_ipc_readdata offsets so the tool works on Jetson (aarch64) and x86_64, not just i.MX (armv7).Problem
tools/ipc_read.cpphardcoded theseq_locklayout, assuming the data buffer starts atCACHELINE_BYTESfrom the mapping base and that thesize_t sizefield sits at offsetsizeof(unsigned) == 4.That only holds on 32-bit ARM (i.MX), where
sizeof(size_t) == 4and the header packs into exactly one 32-byte cacheline. On 64-bit targetssize_tis 8 bytes, so the compiler inserts 4 bytes of padding betweenseqandsize, shifting the realentryoffset:offsetof(entry)Symptom on Jetson:
Fix
Compute
seq,size, andentryoffsets withoffsetof(seq_lock<char>, ...)so the tool always agrees with the real struct layout the writer/reader templates use.The templated
ipc_read()API was never affected because the writer and reader both use&this->entryvia the same struct definition.Follow-up
The underlying
seq_lockpadding[]calculation is also off on 64-bit (entryno longer starts on a cacheline boundary, defeating the false-sharing protection). Tracked separately in #16 so this PR stays minimal and unblocks the tool immediately.Testing
Not yet built — please cross-compile for jetson/imx to confirm. Native build wasn't available on my macOS host.