Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc][NFC] adjust time related implementations #91485

Merged
merged 4 commits into from
May 9, 2024

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented May 8, 2024

This PR

  • Unify the naming style of clock_gettime_impl.
  • Make clock_gettime_impl a header library for more internal usages. Such APIs do not pollute errno so it is more friendly for using inside libc. Implementations of timed locking may be such candidates.
  • Adjust dependency accordingly.

Update:

  • common utilities have been lifted to __support
  • related types are now imported using proxy layers instead of time.h

@llvmbot
Copy link

llvmbot commented May 8, 2024

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

This PR

  • Unify the naming style of clock_getime_impl.
  • Make clock_getime_impl a header library for more internal usage. Such APIs do not pollute errno so it is more friendly for using inside libc. Implementations of timed locking may be such candidates.
  • Adjust dependency accordingly.

Full diff: https://github.com/llvm/llvm-project/pull/91485.diff

6 Files Affected:

  • (modified) libc/src/time/linux/CMakeLists.txt (+16-10)
  • (modified) libc/src/time/linux/clock.cpp (+7-10)
  • (modified) libc/src/time/linux/clock_gettime.cpp (+2-6)
  • (renamed) libc/src/time/linux/clock_gettime_impl.h (+28-6)
  • (modified) libc/src/time/linux/gettimeofday.cpp (+4-7)
  • (modified) libc/src/time/linux/time.cpp (+4-8)
diff --git a/libc/src/time/linux/CMakeLists.txt b/libc/src/time/linux/CMakeLists.txt
index df79bf5986261..e0008ed7cdf1f 100644
--- a/libc/src/time/linux/CMakeLists.txt
+++ b/libc/src/time/linux/CMakeLists.txt
@@ -1,3 +1,13 @@
+add_header_library(
+  clock_gettime_impl
+  HDRS
+    clock_gettime_impl.h
+  DEPENDS
+    libc.include.sys_syscall
+    libc.src.__support.OSUtil.osutil
+    libc.src.__support.error_or
+)
+
 add_entrypoint_object(
   time
   SRCS
@@ -5,9 +15,8 @@ add_entrypoint_object(
   HDRS
     ../time_func.h
   DEPENDS
+    .clock_gettime_impl
     libc.include.time
-    libc.include.sys_syscall
-    libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
 
@@ -18,10 +27,9 @@ add_entrypoint_object(
   HDRS
     ../clock.h
   DEPENDS
+    .clock_gettime_impl
     libc.include.time
-    libc.include.sys_syscall
     libc.src.__support.CPP.limits
-    libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
 
@@ -32,10 +40,10 @@ add_entrypoint_object(
   HDRS
     ../nanosleep.h
   DEPENDS
-    libc.include.time
     libc.include.sys_syscall
-    libc.src.__support.CPP.limits
     libc.src.__support.OSUtil.osutil
+    libc.include.time
+    libc.src.__support.CPP.limits
     libc.src.errno.errno
 )
 
@@ -46,9 +54,8 @@ add_entrypoint_object(
   HDRS
     ../clock_gettime.h
   DEPENDS
+    .clock_gettime_impl
     libc.include.time
-    libc.include.sys_syscall
-    libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
 
@@ -59,8 +66,7 @@ add_entrypoint_object(
   HDRS
     ../gettimeofday.h
   DEPENDS
+    .clock_gettime_impl
     libc.include.time
-    libc.include.sys_syscall
-    libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
diff --git a/libc/src/time/linux/clock.cpp b/libc/src/time/linux/clock.cpp
index 1e95f0526bc9c..39606f6d4e988 100644
--- a/libc/src/time/linux/clock.cpp
+++ b/libc/src/time/linux/clock.cpp
@@ -7,21 +7,18 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/time/clock.h"
-
 #include "src/__support/CPP/limits.h"
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
-#include "src/time/linux/clockGetTimeImpl.h"
-
-#include <sys/syscall.h> // For syscall numbers.
+#include "src/time/linux/clock_gettime_impl.h"
 #include <time.h>
 
 namespace LIBC_NAMESPACE {
 
 LLVM_LIBC_FUNCTION(clock_t, clock, ()) {
+  using namespace internal::time_units;
   struct timespec ts;
-  auto result = internal::clock_gettimeimpl(CLOCK_PROCESS_CPUTIME_ID, &ts);
+  auto result = internal::clock_gettime_impl(CLOCK_PROCESS_CPUTIME_ID, &ts);
   if (!result.has_value()) {
     libc_errno = result.error();
     return -1;
@@ -34,15 +31,15 @@ LLVM_LIBC_FUNCTION(clock_t, clock, ()) {
       cpp::numeric_limits<clock_t>::max() / CLOCKS_PER_SEC;
   if (ts.tv_sec > CLOCK_SECS_MAX)
     return clock_t(-1);
-  if (ts.tv_nsec / 1000000000 > CLOCK_SECS_MAX - ts.tv_sec)
+  if (ts.tv_nsec / 1_s_ns > CLOCK_SECS_MAX - ts.tv_sec)
     return clock_t(-1);
 
   // For the integer computation converting tv_nsec to clocks to work
   // correctly, we want CLOCKS_PER_SEC to be less than 1000000000.
-  static_assert(1000000000 > CLOCKS_PER_SEC,
-                "Expected CLOCKS_PER_SEC to be less than 1000000000.");
+  static_assert(1_s_ns > CLOCKS_PER_SEC,
+                "Expected CLOCKS_PER_SEC to be less than 1'000'000'000.");
   return clock_t(ts.tv_sec * CLOCKS_PER_SEC +
-                 ts.tv_nsec / (1000000000 / CLOCKS_PER_SEC));
+                 ts.tv_nsec / (1_s_ns / CLOCKS_PER_SEC));
 }
 
 } // namespace LIBC_NAMESPACE
diff --git a/libc/src/time/linux/clock_gettime.cpp b/libc/src/time/linux/clock_gettime.cpp
index 47e974a866c83..f2943955bc78b 100644
--- a/libc/src/time/linux/clock_gettime.cpp
+++ b/libc/src/time/linux/clock_gettime.cpp
@@ -7,13 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/time/clock_gettime.h"
-
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
-#include "src/time/linux/clockGetTimeImpl.h"
-
-#include <sys/syscall.h> // For syscall numbers.
+#include "src/time/linux/clock_gettime_impl.h"
 #include <time.h>
 
 namespace LIBC_NAMESPACE {
@@ -21,7 +17,7 @@ namespace LIBC_NAMESPACE {
 // TODO(michaelrj): Move this into time/linux with the other syscalls.
 LLVM_LIBC_FUNCTION(int, clock_gettime,
                    (clockid_t clockid, struct timespec *ts)) {
-  auto result = internal::clock_gettimeimpl(clockid, ts);
+  auto result = internal::clock_gettime_impl(clockid, ts);
 
   // A negative return value indicates an error with the magnitude of the
   // value being the error code.
diff --git a/libc/src/time/linux/clockGetTimeImpl.h b/libc/src/time/linux/clock_gettime_impl.h
similarity index 60%
rename from libc/src/time/linux/clockGetTimeImpl.h
rename to libc/src/time/linux/clock_gettime_impl.h
index 8c8c9fcf845cc..752434018e261 100644
--- a/libc/src/time/linux/clockGetTimeImpl.h
+++ b/libc/src/time/linux/clock_gettime_impl.h
@@ -6,13 +6,13 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_LIBC_SRC_TIME_LINUX_CLOCKGETTIMEIMPL_H
-#define LLVM_LIBC_SRC_TIME_LINUX_CLOCKGETTIMEIMPL_H
+#ifndef LLVM_LIBC_SRC_TIME_LINUX_CLOCK_GETTIME_IMPL_H
+#define LLVM_LIBC_SRC_TIME_LINUX_CLOCK_GETTIME_IMPL_H
 
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/__support/error_or.h"
-#include "src/errno/libc_errno.h"
+#include "src/__support/macros/attributes.h"
 
 #include <stdint.h>      // For int64_t.
 #include <sys/syscall.h> // For syscall numbers.
@@ -21,8 +21,30 @@
 namespace LIBC_NAMESPACE {
 namespace internal {
 
-LIBC_INLINE ErrorOr<int> clock_gettimeimpl(clockid_t clockid,
-                                           struct timespec *ts) {
+namespace time_units {
+LIBC_INLINE constexpr time_t operator""_s_ns(unsigned long long s) {
+  return s * 1'000'000'000;
+}
+LIBC_INLINE constexpr time_t operator""_s_us(unsigned long long s) {
+  return s * 1'000'000;
+}
+LIBC_INLINE constexpr time_t operator""_s_ms(unsigned long long s) {
+  return s * 1'000;
+}
+LIBC_INLINE constexpr time_t operator""_ms_ns(unsigned long long ms) {
+  return ms * 1'000'000;
+}
+LIBC_INLINE constexpr time_t operator""_ms_us(unsigned long long ms) {
+  return ms * 1'000;
+}
+LIBC_INLINE constexpr time_t operator""_us_ns(unsigned long long us) {
+  return us * 1'000;
+}
+} // namespace time_units
+// namespace time_units
+
+LIBC_INLINE ErrorOr<int> clock_gettime_impl(clockid_t clockid,
+                                            struct timespec *ts) {
 #if SYS_clock_gettime
   int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_clock_gettime,
                                               static_cast<long>(clockid),
@@ -45,4 +67,4 @@ LIBC_INLINE ErrorOr<int> clock_gettimeimpl(clockid_t clockid,
 } // namespace internal
 } // namespace LIBC_NAMESPACE
 
-#endif // LLVM_LIBC_SRC_TIME_LINUX_CLOCKGETTIMEIMPL_H
+#endif // LLVM_LIBC_SRC_TIME_LINUX_CLOCK_GETTIME_IMPL_H
diff --git a/libc/src/time/linux/gettimeofday.cpp b/libc/src/time/linux/gettimeofday.cpp
index 07ab4d579176e..bf388a656441f 100644
--- a/libc/src/time/linux/gettimeofday.cpp
+++ b/libc/src/time/linux/gettimeofday.cpp
@@ -7,24 +7,21 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/time/gettimeofday.h"
-
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
-#include "src/time/linux/clockGetTimeImpl.h"
-
-#include <sys/syscall.h> // For syscall numbers.
+#include "src/time/linux/clock_gettime_impl.h"
 
 namespace LIBC_NAMESPACE {
 
 // TODO(michaelrj): Move this into time/linux with the other syscalls.
 LLVM_LIBC_FUNCTION(int, gettimeofday,
                    (struct timeval * tv, [[maybe_unused]] void *unused)) {
+  using namespace internal::time_units;
   if (tv == nullptr)
     return 0;
 
   struct timespec ts;
-  auto result = internal::clock_gettimeimpl(CLOCK_REALTIME, &ts);
+  auto result = internal::clock_gettime_impl(CLOCK_REALTIME, &ts);
 
   // A negative return value indicates an error with the magnitude of the
   // value being the error code.
@@ -34,7 +31,7 @@ LLVM_LIBC_FUNCTION(int, gettimeofday,
   }
 
   tv->tv_sec = ts.tv_sec;
-  tv->tv_usec = static_cast<suseconds_t>(ts.tv_nsec / 1000);
+  tv->tv_usec = static_cast<suseconds_t>(ts.tv_nsec / 1_us_ns);
   return 0;
 }
 
diff --git a/libc/src/time/linux/time.cpp b/libc/src/time/linux/time.cpp
index e286fae095b2a..336c723356805 100644
--- a/libc/src/time/linux/time.cpp
+++ b/libc/src/time/linux/time.cpp
@@ -6,22 +6,18 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "src/time/time_func.h"
-
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
-#include "src/time/linux/clockGetTimeImpl.h"
-
-#include <sys/syscall.h> // For syscall numbers.
+#include "src/time/linux/clock_gettime_impl.h"
+#include "src/time/time_func.h"
 #include <time.h>
 
 namespace LIBC_NAMESPACE {
 
-LLVM_LIBC_FUNCTION(time_t, time, (time_t * tp)) {
+LLVM_LIBC_FUNCTION(time_t, time, (time_t *tp)) {
   // TODO: Use the Linux VDSO to fetch the time and avoid the syscall.
   struct timespec ts;
-  auto result = internal::clock_gettimeimpl(CLOCK_REALTIME, &ts);
+  auto result = internal::clock_gettime_impl(CLOCK_REALTIME, &ts);
   if (!result.has_value()) {
     libc_errno = result.error();
     return -1;

Copy link

github-actions bot commented May 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@SchrodingerZhu SchrodingerZhu merged commit 8ac928f into llvm:main May 9, 2024
4 checks passed
@SchrodingerZhu SchrodingerZhu deleted the libc/time branch May 9, 2024 20:35
SchrodingerZhu added a commit that referenced this pull request May 9, 2024
lntue pushed a commit that referenced this pull request May 9, 2024
SchrodingerZhu added a commit that referenced this pull request May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants