Page MenuHomePhabricator

[libc++] Avoid using timespec when it might not be available
ClosedPublic

Authored by miyuki on Jun 14 2019, 4:57 AM.

Details

Summary

The type timespec is unconditionally used in __threading_support.
Since the C library is only required to provide it in C11, this might
cause problems for platforms with external thread porting layer (i.e.
when _LIBCPP_HAS_THREAD_API_EXTERNAL is defined) with pre-C11
C libraries.

In our downstream port of libc++ we used to provide a definition of
timespec in __external_threading, but this solution is not ideal
because timespec is not a reserved name.

This patch renames timespec into __libcpp_timespec_t in the
thread-related parts of libc++. For all cases except external
threading this type is an alias for ::timespec (and no functional
changes are intended).

In case of external threading it is expected that the
external_threading header will either provide a similar typedef (if
timespec is available in the vendor's C library) or provide a
definition of
libcpp_timespec_t compatible with POSIX timespec.

Diff Detail

Repository
rL LLVM

Event Timeline

miyuki created this revision.Jun 14 2019, 4:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: dexonsmith. · View Herald Transcript
ldionne requested changes to this revision.Jun 17 2019, 1:37 PM

I don't mind the change (it's safe, simple, etc.). However, I don't understand why timespec is being tied to threads and < __external_threading>. Is is possible that it's only a convenient way of doing it right now?

This revision now requires changes to proceed.Jun 17 2019, 1:37 PM

__libcpp_condvar_timedwait needs to be implemented in a different TU, so it can't be a template and at the same time it needs some sort of time point. I think timespec was chosen because it is what pthread_cond_timedwait expects. Do you think a specialization of chrono::time_point (like chrono::time_point<chrono::system_clock, chrono::nanoseconds> used by condition_variable::__do_timed_wait) would make more sense here?

ldionne accepted this revision.Jun 18 2019, 5:20 AM

__libcpp_condvar_timedwait needs to be implemented in a different TU, so it can't be a template and at the same time it needs some sort of time point. I think timespec was chosen because it is what pthread_cond_timedwait expects. Do you think a specialization of chrono::time_point (like chrono::time_point<chrono::system_clock, chrono::nanoseconds> used by condition_variable::__do_timed_wait) would make more sense here?

My comment was more specifically that it feels weird that the definition of timespec, a time-related type, is tied to _LIBCPP_HAS_NO_THREADS. I would expect it to be tied to something else more general, perhaps.

This revision is now accepted and ready to land.Jun 18 2019, 5:20 AM

In 2019, do we really care about pre-C11 libraries?

We maintain such library for an embedded toolchain. Besides, only C++17 and later versions are based on C11.

This isn't a question of C11 because timespec has been a part of the POSIX spec forever.

From my perspective, this patch is fine since we already have __libcpp_condvar_t and __libcpp_mutex_t, so in some way we're even making things more consistent. I'll commit this tomorrow unless there are reasons why this is harmful.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 21, 1:30 AM