Opened 7 years ago

Closed 2 years ago

#8798 closed bug (fixed)

PTHREAD_RWLOCK_INITIALIZER macro not defined

Reported by: cian Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: System/POSIX Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

The pthread_rwlock functions exist in the header and I presume work, but the pthread_rwlock_initializer macro does not exist.

I've had to take a guess at the component, its likely not correct.

Attachments (4)

0001-pthread.h-Add-PTHREAD_RWLOCK_INITIALIZER-macro.patch (732 bytes ) - added by return_0e 3 years ago.
Patch to define PTHREAD_RWLOCK_INITIALIZER
0001-pthread_rwlock-draft-local-implementation.patch (7.5 KB ) - added by korli 3 years ago.
Try at implementing local pthread_rwlock
0001-pthread_rwlock-local-implementation-with-mutexes.patch (8.3 KB ) - added by korli 3 years ago.
reworked patch for local pthread_rwlock
0001-pthread_rwlock-use-a-mutex-for-process-private-locks.patch (3.9 KB ) - added by korli 2 years ago.
try 3

Download all attachments as: .zip

Change History (23)

comment:1 by anevilyak, 7 years ago

Component: Kits/Kernel KitSystem/libroot.so
Version: R1/alpha3R1/Development

comment:2 by bonefish, 7 years ago

Component: System/libroot.soSystem/POSIX
Owner: changed from axeld to nobody

comment:3 by luroh, 5 years ago

Milestone: R1Unscheduled

Move POSIX compatibility related tickets out of R1 milestone (FutureHaiku/Features).

comment:4 by korli, 4 years ago

FYI this is required by Blender.

comment:5 by diver, 4 years ago

Also for current VLC.

by return_0e, 3 years ago

Patch to define PTHREAD_RWLOCK_INITIALIZER

comment:6 by return_0e, 3 years ago

Has a Patch: set

comment:7 by return_0e, 3 years ago

This macro is defined here http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_destroy.html and is also required for compiling with pthread support for Swift.

Last edited 3 years ago by return_0e (previous) (diff)

comment:8 by waddlesplash, 3 years ago

Resolution: fixed
Status: newclosed

Applied in hrev51184. Thanks!

comment:9 by korli, 3 years ago

This patch assumes the implementation knows what to do with the values provided by pthread_rwlock_initializer, which it doesn't seem to. Best would be to provide a unit test for this, eventually.

comment:10 by korli, 3 years ago

Resolution: fixed
Status: closedreopened

Please revert hrev51184.

comment:11 by pulkomandy, 3 years ago

Reverted in hrev51190. See haiku-commits discussions on hrev51184 to see how this does not seem possible with the current implementation.

by korli, 3 years ago

Try at implementing local pthread_rwlock

comment:12 by korli, 3 years ago

Tested on x86_64 only. I tried to implement again the local pthread_rwlock with mutexes. Might not be the best solution, the old one used block_thread and unblock_thread syscalls. Please review.

Another problem is the size of the struct _pthread_rwlock, it stays the same on x86_64 (40 bytes), not on x86 (was 32 bytes). I suppose we can reduce to only use 32 bytes on x86 and let 8 bytes unused on x86_64. Can this size actually change? Or are we stucked with these sizes for compatibility reasons?

comment:13 by axeld, 3 years ago

Since Haiku introduced the pthread_* functions in the first place, and we didn't reach beta yet, I think it's fair to say that it's okay to break compatibility at this point. It would also be nice to be able to actually break old code visibly, not just let it crash in mysterious ways... Maybe we can achieve this with symbol versioning?

I would probably add a few bytes for future changes, though :-)

by korli, 3 years ago

reworked patch for local pthread_rwlock

comment:14 by korli, 3 years ago

@axeld I've changed the implementation to use only the first 32 bytes of the struct, so we could theoretically add a few bytes at the end: this should normally have no impact on existing binaries (TODO: remove the #ifdef B_HAIKU_64_BIT).

comment:15 by bonefish, 3 years ago

Unfortunately I don't have the time to thoroughly read the patch. From a quick glance: Not using a common waiting queue for readers and writers means that you can't treat both fairly. In this case it looks like writers can overtake waiting readers. As I wrote on the commit mailing list, I'd just replace the semaphore by a mutex and leave the implementation otherwise unchanged.

comment:16 by korli, 3 years ago

bonefish, yes readers and writers are treated unfairly in the patch, as in the specification: "The calling thread acquires the read lock if a writer does not hold the lock and there are no writers blocked on the lock.", which actually means that writers can overtake waiting readers. http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html

in reply to:  16 comment:17 by bonefish, 3 years ago

Replying to korli:

bonefish, yes readers and writers are treated unfairly in the patch, as in the specification: "The calling thread acquires the read lock if a writer does not hold the lock and there are no writers blocked on the lock.", which actually means that writers can overtake waiting readers. http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html

But that only means that a reader may not overtake an already waiting writer. In your implementation a new writer also overtakes an already waiting reader. While the specification doesn't seem to disallow that behavior, it means readers can be starved (which cannot happen with the current common queue implementation).

comment:18 by korli, 2 years ago

Applied in hrev51310.

comment:19 by korli, 2 years ago

Resolution: fixed
Status: reopenedclosed

Not seen a regression so far. Closing.

Note: See TracTickets for help on using tickets.