Conversation
Summary of ChangesHello @oI0ck, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive support for POSIX standard semaphore interfaces within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This PR introduces POSIX semaphore implementation. The changes are extensive, adding new headers and implementation files for named and unnamed semaphores. The overall structure is good, but I've found several critical issues in the implementation, including race conditions, incorrect logic leading to deadlocks, and potential memory corruption. There are also some areas where the code can be simplified and made more robust. Please address the critical issues before merging.
ba94ea8 to
da0f0fd
Compare
Unit Test Results14 250 tests +520 13 389 ✅ +518 58m 31s ⏱️ + 1m 59s For more details on these failures, see this check. Results for commit 63ca129. ± Comparison against base commit 9bffcee. ♻️ This comment has been updated with latest results. |
da0f0fd to
b6d1928
Compare
b6d1928 to
20f142f
Compare
cf48092 to
f3710e3
Compare
23e321c to
057606b
Compare
|
CI shows problems with this change, I'm converting it to a draft until I am sure that it is in mergable state. |
070fbcb to
67a05ba
Compare
0fcb1c0 to
78ea982
Compare
657275d to
169a290
Compare
|
Please rebase thsi PR and resolve conficts. |
169a290 to
e24eb57
Compare
e24eb57 to
2daeb8f
Compare
No functional change. TASK: RTOS-1407
2daeb8f to
5df6063
Compare
TASK: RTOS-1407
TASK: RTOS-1407
semaphoreUp() incremented the counter unconditionally, so a semaphore posted SEM_VALUE_MAX times wrapped silently. POSIX requires sem_post() to fail with EOVERFLOW instead, so report -EOVERFLOW and leave the value untouched. semaphoreCreate() accepted any initial value. Reject one above the limit, and a NULL semaphore. TASK: RTOS-1407
sem_getvalue() and sem_trywait() need a way to read the counter and to take the semaphore without blocking. Add the two primitives they are built on. Both take the internal mutex with mutexLock() rather than mutexTry(): contention on it is an implementation detail, not a property of the semaphore, so letting it surface would make semaphoreCount() report a spurious 0 and semaphoreTryDown() fail while the semaphore is free. TASK: RTOS-1407 Assisted-by: claude-opus-5
semaphoreDown() takes a relative timeout, resolves it against the monotonic clock and waits on the condvar's own clock. sem_timedwait() is specified in terms of an absolute CLOCK_REALTIME deadline, which that interface cannot express without the caller converting between clock domains by hand. Add semaphoreDownAtClock(), which passes the deadline and the clock straight to condClockWait() and lets the kernel resolve both. A deadline that has already passed comes back as -ETIME before the caller is parked, so the value is still checked first and an available semaphore is still taken. The one value the kernel cannot express is 0, which it reads as "no deadline", so the epoch is turned into -ETIME here. PH_CLOCK_RELATIVE is rejected: the retry loop hands the same value to every condClockWait() call, which would restart a relative timeout after each wakeup. TASK: RTOS-1407 Assisted-by: claude-opus-5
Add the POSIX semaphore API. Unnamed semaphores are backed directly by libphoenix's semaphore_t. Named ones live in posixsrv, which exposes each of them as a device node under /dev/posix/sem/. A named sem_t holds an open descriptor on that node rather, so operations travel as ordinary ioctl() calls and the kernel drops the server-side reference when the process exits. The descriptor is opened O_CLOEXEC, giving the POSIX requirement that named semaphores are closed across a successful exec(). TASK: RTOS-1407 Assisted-by: claude-opus-5
semaphoreUp() signalled only on the 0 -> 1 transition, assuming that a positive value meant there were no waiters. This is incorrect. Track the number of waiters and signal whenever a waiter is present. TASK: RTOS-1407
5df6063 to
63ca129
Compare
| err = condClockWait(s->cond, s->mutex, deadline, clock); | ||
| --s->waiters; | ||
| } while ((err == 0) || (err == -EINTR)); | ||
|
|
||
| mutexUnlock(s->mutex); |
There was a problem hiding this comment.
condClockWait may return with lock not held
| /* not required by POSIX, but ensures that | ||
| * sem_getvalue() does not return an overflowed value. | ||
| */ |
There was a problem hiding this comment.
| /* not required by POSIX, but ensures that | |
| * sem_getvalue() does not return an overflowed value. | |
| */ | |
| /* | |
| * not required by POSIX, but ensures that | |
| * sem_getvalue() does not return an overflowed value. | |
| */ |
TASK: RTOS-1407
Description
This PR introduces implementation of POSIX standard semaphore interfaces.
Types of changes
How Has This Been Tested?
ia32-generic-qemuChecklist:
Special treatment