fix(compat): make the POSIX adapter build and run on macOS - #96
fix(compat): make the POSIX adapter build and run on macOS#96AshrafAhmed9 wants to merge 2 commits into
Conversation
The file documents itself as running on Linux, macOS or WSL, but Darwin
does not implement three of the primitives it uses:
- pthread_mutex_timedlock() is absent, so the file does not compile.
- sem_timedwait() is absent, same.
- sem_init() fails with ENOSYS. Darwin has no unnamed semaphores, so
every semaphore creation would fail at runtime even once it built.
- sem_getvalue() fails with ENOSYS, so the count that posix_sem_post()
enforces the ceiling against cannot be read back.
Named semaphores are not a way out: sem_open() works but sem_getvalue()
still does not, and the ceiling check needs the count.
On Apple platforms the semaphore is therefore a mutex and a condition
variable holding its own count, which also makes the timed wait exact
rather than polled, and the timed mutex lock polls trylock until the
deadline. Every other POSIX host keeps the system primitives, reached
through the same small compat_* wrappers so the call sites read the same
on both.
CONTRIBUTING.md asks for #ifdef __APPLE__ around Apple-specific code;
this file previously had none.
Verified on macOS 15 with Apple Clang. Before the change the translation
unit fails with two "call to undeclared function" errors; after it
compiles clean under -std=c11 -Wall -Wextra with no warnings. The new
primitives were exercised directly: a 100 ms timed wait on an empty
semaphore returns -1 after 105 ms, the same wait returns 0 after 152 ms
when another thread posts at 150 ms, an 80 ms timed lock on a held mutex
returns -1 after 80 ms, and the same lock succeeds once released.
srpatcha
left a comment
There was a problem hiding this comment.
Approving. The Darwin reasoning is right and the Linux path is genuinely
unchanged — I checked that rather than assuming it, because the diff touches
shared call sites, not only #ifdef __APPLE__ blocks.
What I verified
Thirty changed lines sit outside the Apple guards — pthread_mutex_timedlock →
compat_mutex_timedlock, sem_t → compat_sem_t, and the five sem_* calls.
So every POSIX host now goes through the shims, and the question is whether the
#else branch reproduces the originals exactly. It does:
typedef sem_t compat_sem_t;
static int compat_sem_init(compat_sem_t *s, unsigned value) { return sem_init(s, 0, value); }
static int compat_sem_destroy(compat_sem_t *s) { return sem_destroy(s); }
...sem_init(s, 0, value) keeps the not-shared 0 from the original call. The
typedef is the same type, so no struct layout moves.
The one thing that looked like a behaviour change is not. compat_sem_wait
carries an EINTR retry loop, but master:393 already had it at the call site:
while ((rc = sem_wait(s)) == -1 && errno == EINTR) { }It moved into the shim; it was not added.
Built and ran on Linux, applied onto a base where master's duplicate test
targets are repaired:
0 build errors
100% tests passed, 0 tests failed out of 28
What I did not verify
The Darwin behaviour itself. I have no macOS host, so sem_init returning
ENOSYS, sem_getvalue being unavailable even for named semaphores, and the
absence of pthread_mutex_timedlock are all things I am taking from your
description and from the documented Darwin behaviour, not from a run. Saying so
explicitly rather than implying the whole PR is tested.
Two things I would want a macOS run to confirm before relying on it:
COMPAT_LOCK_POLL_NSat 1 ms bounds the overshoot of a timed lock, as your
comment says. That is the correct trade-off to name, and the value only
matters if something in the tree takes short timed locks — worth a note in
the docs if any caller does.- The condition-variable semaphore enforces the ceiling against a count it
owns. Since that count is now the authority rather thansem_getvalue, a
post that would exceedmaxhas to be rejected on that path too, not only
on the POSIX one.
Replacing a polled timed wait with an exact one on Darwin is a real improvement
over what a sem_trywait loop would have given, and the comment block explaining
why named semaphores do not rescue sem_getvalue is exactly the kind of thing
that stops someone "simplifying" this back into a bug later.
Needs #82 or #93 to land first — on master today, CMake cannot generate any
test target, so CI here cannot run its own tests.
|
Thanks for checking the shared call sites rather than trusting the On the ceiling. It is rejected on the Apple path. int value = 0;
if (compat_sem_getvalue(&g_sems[handle].sem, &value) == 0 &&
(uint32_t)value >= g_sems[handle].max) {
return -1;
}
return compat_sem_post(&g_sems[handle].sem) == 0 ? 0 : -1;So the count that became the authority is the one the ceiling is tested against. You are onto something real one level down, though. That check is not atomic. Master has the same shape on POSIX today ( The fix is a per-semaphore mutex held across the check and the increment, on both backends, so the invariant survives contention instead of holding only when posts happen to serialise. That is a real behaviour change to the POSIX path, so I would rather not smuggle it into a PR whose stated job is "make it compile on macOS". Happy either way: say the word and I will add it here, otherwise I will open it separately once this lands. On the poll interval. Nothing in the tree reaches it. Agreed on #93. CMake cannot generate a test target on master, so CI here has nothing to run until that lands. |
COMPAT_LOCK_POLL_NS bounds how far a timed mutex lock can overshoot on Apple, where pthread_mutex_timedlock() does not exist. No caller reaches it today: posix_mutex_lock_fn() routes a zero timeout to trylock and EOS_OSA_WAIT_FOREVER to a plain lock, and every eos_mutex_lock() in the tree passes 0 or EOS_NO_WAIT. Record the granularity now so the first caller to want a finite timed lock finds it, rather than after. Comment only, no behaviour change.
srpatcha
left a comment
There was a problem hiding this comment.
Re-approving — my earlier review was auto-dismissed by the follow-up commit, not
withdrawn.
docs(compat): note the timed-lock poll granularity on Darwin addresses the
first of the two things I flagged. Documenting that COMPAT_LOCK_POLL_NS bounds
how far a timed lock can overshoot is the right response: the value is a
trade-off, not a constant someone should tune without knowing what it buys.
Everything in my previous review stands:
- The Linux path is unchanged. Every
compat_*in the#elsebranch delegates
to the original call,sem_init(s, 0, value)keeps the not-shared0, and
typedef sem_t compat_sem_tmoves no struct layout. TheEINTRretry loop in
compat_sem_waitwas already at the call site onmaster:393; it moved rather
than appearing. - Built and ran on Linux against a repaired base: 0 build errors, 28/28
passed. - I still have not verified the Darwin behaviour itself — no macOS host here — so
sem_initreturningENOSYS,sem_getvaluebeing unavailable even for named
semaphores, and the absence ofpthread_mutex_timedlockare taken from your
description and the documented Darwin behaviour, not from a run.
The second point from last time is still open, and is the one I would want a
macOS run to settle: the condition-variable semaphore now owns its own count, so
the max ceiling has to be enforced on that path too, not only on the POSIX one.
Worth a test if you have a Darwin machine.
Needs #82 or #93 to land first — on master today CMake cannot generate a single
test target, so CI here cannot run its own tests.
|
Ran the ceiling on Darwin. It holds. The coverage you wanted already exists: macOS 15, Apple Clang, arm64: So One thing that test does not prove, and I would rather say it than let the green run imply otherwise: the ceiling check is single-threaded there. Getting it to run took an unrelated fix
#ifdef __linux__
eos_hal_linux_register();
#else
eos_hal_rtos_register();
#endifbut CMake picks the source file on a different question: if(EOS_PLATFORM STREQUAL "linux" OR NOT CMAKE_CROSSCOMPILING)
target_sources(eos_hal PRIVATE hal/src/hal_linux.c)
else()
target_sources(eos_hal PRIVATE hal/src/hal_rtos.c)
endif()CMake asks "am I cross-compiling", the C asks "am I on Linux". On a Mac those disagree. CMake compiles The comment above the declarations in I widened all three guards to |
os_adapter_posix.copens by saying it "runs EoS services on Linux, macOS orWSL". It does not run on macOS. Darwin does not implement three of the
primitives it reaches for:
pthread_mutex_timedlock()sem_timedwait()sem_init()ENOSYSsem_getvalue()ENOSYSposix_sem_post()checks the ceiling against cannot be readThe first two are compile errors, so
Build (macos-latest)cannot pass. Theother two would still fail at runtime once it built.
The file had no
#ifdef __APPLE__anywhere, which CONTRIBUTING.md asks for.The approach
Named semaphores don't help:
sem_open()works on Darwin butsem_getvalue()still doesn't, and
posix_sem_post()needs the count to enforce the ceiling.So on Apple the semaphore is a mutex and a condition variable holding its own
count. That gives an exact
sem_getvalue(), and makes the timed wait a realpthread_cond_timedwait()rather than a poll. The timed mutex lock has no suchequivalent, so it polls
pthread_mutex_trylock()on a 1 ms interval until thedeadline — that interval bounds how far a timed lock can overshoot.
Everything else keeps the system primitives. Both sides sit behind the same
compat_*wrappers, so the call sites are unchanged and the non-Apple wrapperscompile to a direct call. The
EINTRretry loops moved into the wrappers, wherethey were already duplicated at each call site.
Testing
Verified on macOS 15, Apple Clang. Before, the translation unit fails with the
two errors above. After, it compiles clean, no warnings:
The new primitives were exercised directly rather than assumed:
That covers the count, the empty case, a timeout that fires, a wait woken by
another thread posting at 150 ms, a timed lock that expires against a held
mutex, and one that succeeds once released.
I couldn't run
ctestend to end — master doesn't compile for reasons unrelatedto this file (duplicate test targets in
tests/CMakeLists.txt, duplicateeos_task_set_current_internalintask.c, the unresolved priority-inheritancemerge in
sync.c), which #93 and #94 address. Once #94 is in, this file isreached by the normal build and the suite should run on macOS for the first time.