Skip to content

Fix int32_t-vs-int portability; align CThread assignment with std::thread#3329

Open
aklofas wants to merge 3 commits into
Haivision:masterfrom
aklofas:baremetal-arm-none-eabi-portability
Open

Fix int32_t-vs-int portability; align CThread assignment with std::thread#3329
aklofas wants to merge 3 commits into
Haivision:masterfrom
aklofas:baremetal-arm-none-eabi-portability

Conversation

@aklofas

@aklofas aklofas commented Jun 1, 2026

Copy link
Copy Markdown

Summary

Three small portability fixes that let libsrt compile and run on toolchains where int32_t is a distinct type from int (e.g. the newlib arm-none-eabi bare-metal toolchain, where int32_t is long int) and on pthread backends without thread cancellation.

Reworked per review (see comments): the config field is typed to match the wire, TEV_SYNC passes no argument, and CThread assignment matches std::thread — no opt-out define needed anymore.

Changes

1. CSrtConfig::iMSS is now int32_t
Matching the handshake wire field CHandShake::m_iMSS it is clamped against, so std::min(m_config.iMSS, w_hs.m_iMSS) in acceptAndRespond() deduces its template argument. The SRTO_UDP_SNDBUF/SRTO_UDP_RCVBUF setters that std::max the option value against iMSS read it with cast_optval<int32_t> for the same reason. Where int32_t == int this is a pure no-op.

2. common.h/core.hEventVariant() no-argument constructor
EventVariant(0) at the TEV_SYNC call site was ambiguous between EventVariant(int32_t) and EventVariant(const CPacket*) once int32_t is not int. TEV_SYNC passes no argument (no slot is connected to it, and updateCC never reads the arg for it), so EventVariant now has a no-argument constructor producing UNDEFINED, used at that call site.

3. sync_posix.cppCThread assign-to-joinable matches std::thread
CThread::operator= called pthread_cancel() on a still-joinable thread — guarded out on Android/OHOS (#1476 / #1484) and missing on some RTOS pthread backends (e.g. FreeRTOS-Plus-POSIX). Per std::thread semantics ([thread.thread.assign]) and the ENABLE_STDCXX_SYNC backend, this is now std::terminate() after the existing IPE log line. This removes the only pthread_cancel call in the codebase, so no platform special-casing is needed at all.

Testing

Built and run on bare-metal arm-none-eabi (Cortex-M4F) under QEMU on a FreeRTOS + lwIP substrate (pthread sync backend, ENABLE_STDCXX_SYNC=OFF): libsrt cross-compiles cleanly; an srt_startupsrt_create_socketsrt_getsockstatesrt_closesrt_cleanup smoke test and a caller↔listener loopback transfer over a lossy link (plain and AES-128 via mbedTLS) pass. Desktop Release build with ENABLE_UNITTESTS=ON compiles unchanged and the full test-srt suite passes (275/275).

Behavior note: the only behavior change on existing platforms is the assign-to-joinable IPE path, which now terminates — as std::thread and the ENABLE_STDCXX_SYNC build already do — instead of cancelling the live thread.

@ethouris

ethouris commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

I'd have different changes in mind.

  1. For the config.iMSS field, this type should be changed into int32_t.
  2. For EventVariant, that case with TEV_SYNC is that it gets no argument - so this is simply a sealup replacement. A better solution would be to provide the version with no arguments, which results in UNDEFINED type inside. The argument isn't used anyway.
  3. The CThread should behave exactly the same way as std::thread, as far as possible. Not sure why it was done this way, but I think CThread version for POSIX should do the same as std::thread - that is, if (joinable()) std::terminate();. That should also solve the problem of missing pthread_cancel everywhere if we stop using it.

@ethouris ethouris added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core Status: Needs Refinement Comments should contain requests to refine the works. labels Jun 8, 2026
…read

On targets where int32_t is a distinct type from int (e.g. the newlib
arm-none-eabi bare-metal toolchain, where int32_t is long int):

- CSrtConfig::iMSS is now int32_t, matching the wire field
  CHandShake::m_iMSS it is clamped against, so the std::min in
  acceptAndRespond() deduces its template argument. The two
  SRTO_UDP_SNDBUF/RCVBUF setters that std::max against iMSS read the
  option value as int32_t for the same reason.

- EventVariant grows a no-argument constructor (type = UNDEFINED) for
  events that pass no argument; TEV_SYNC uses it instead of the
  ambiguous EventVariant(0) (int32_t vs const CPacket* overloads).

- CThread::operator= on the POSIX backend now matches std::thread
  semantics: assigning to a joinable thread is std::terminate(), not
  pthread_cancel(). This removes the only pthread_cancel() call, so
  pthread backends without thread cancellation (Android, OHOS, RTOS
  pthread layers such as FreeRTOS-Plus-POSIX) need no special-casing.

No-ops on platforms where int32_t == int, except the assign-to-joinable
IPE path, which now aborts like the ENABLE_STDCXX_SYNC backend instead
of cancelling the live thread.
aklofas added a commit to aklofas/ts-transformer that referenced this pull request Jun 11, 2026
Upstream PR Haivision/srt#3329 review (ethouris): type CSrtConfig::iMSS
as int32_t (the wire type) instead of pinning std::min; give EventVariant
a no-arg UNDEFINED constructor for TEV_SYNC instead of int32_t(0); make
POSIX CThread assign-to-joinable std::terminate() like std::thread,
removing libsrt's only pthread_cancel call. The SRT_NO_PTHREAD_CANCEL
opt-out define no longer exists, so drop it from the substrate flags.

All five freertos-srt QEMU gates green with the regenerated patch.
@aklofas aklofas force-pushed the baremetal-arm-none-eabi-portability branch from a2210eb to ac88ce2 Compare June 11, 2026 18:22
@aklofas aklofas changed the title Fix int32_t-vs-int portability; add SRT_NO_PTHREAD_CANCEL opt-out Fix int32_t-vs-int portability; align CThread assignment with std::thread Jun 11, 2026
@aklofas

aklofas commented Jun 11, 2026

Copy link
Copy Markdown
Author

Reworked along the lines you suggested — all three points, and the branch is updated:

  1. CSrtConfig::iMSS is now int32_t. One ripple to be aware of: the SRTO_UDP_SNDBUF/SRTO_UDP_RCVBUF setters std::max the option value against iMSS, so they now read it with cast_optval<int32_t> — otherwise they'd hit the same deduction failure on platforms where int32_tint. With the field typed correctly, the std::min<int> pin is reverted; core.cpp is no longer touched by this PR.

  2. EventVariant has a no-argument constructor producing UNDEFINED, and the TEV_SYNC call site uses it. I checked that nothing reads the argument — no slot is ever connected to TEV_SYNC, and the generic TEV_SYNC handling in updateCC doesn't touch arg.

  3. POSIX CThread::operator= now calls std::terminate() when assigning to a joinable thread, matching std::thread ([thread.thread.assign]) and the ENABLE_STDCXX_SYNC backend. That removes the only pthread_cancel() call in the codebase, so the SRT_NO_PTHREAD_CANCEL define I proposed is gone — and the existing __ANDROID__/__OHOS__ special case ([BUG]build for Android can't use pthread_cancel #1476) with it. I kept the IPE log line ahead of the terminate so the abort is diagnosable.

Re-verified on both ends: desktop Release build with the full test-srt suite green (275/275), and the original bare-metal target (arm-none-eabi Cortex-M4F, FreeRTOS + lwIP under QEMU, where int32_t is long int) — cross-compiles cleanly and passes an srt_startup/socket smoke test plus a caller↔listener loopback transfer over a lossy link, plain and AES-128. I've updated the PR title/description to match.

Comment thread srtcore/common.h Outdated
@ethouris ethouris removed the Status: Needs Refinement Comments should contain requests to refine the works. label Jun 12, 2026
Comment thread srtcore/common.h Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants