[libc][NFC] Start cleanup of time functions

[michaelrj]

Summary

The time functions have not yet been updated to match our new coding
patterns. This patch removes some unnecessary includes, adjusts the
names of the test targets, and adds several TODO comments. It is my
intention to follow this patch up with additional cleanup.

[rtenneti]

Thanks for the changes, they LGTM.

[sivachandra]

OK after excluding one change from this patch.

libc/include/llvm-libc-types/time_t.h

12 ↗

(On Diff #518061)

__INTPTR_TYPE__ might not be the most appropriate, but I am not sure __INT64_TYPE__ is appropriate also? I suggest that you separate this part out to a different patch so that the actual clean up can land while we discuss about the definition of time_t.

[michaelrj]

move time_t change into separate patch.

[mcgrathr]

libc/include/llvm-libc-types/time_t.h

12 ↗

(On Diff #518061)

Agreed on separating semantic from cosmetic changes.
About the time_t defn, this has traditionally been considered part of the OS ABI.
It probably still needs to be, since things like struct stat and whatnot that you generally want to be OS-provided use the type and you want to match the OS semantics for minimum and maximum values expected to get there.

libc/src/time/asctime.h

 
 namespace __llvm_libc {
 
 char *asctime(const struct tm *timeptr);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_ASCTIME_H
-
-#include "include/time.h"

libc/src/time/asctime_r.h

 
 namespace __llvm_libc {
 
 char *asctime_r(const struct tm *timeptr, char *buffer);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_ASCTIME_R_H
-
-#include "include/time.h"

libc/src/time/clock_gettime.cpp

 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 
 #include <sys/syscall.h> // For syscall numbers.
 #include <time.h>
 
 namespace __llvm_libc {
 
+// TODO(michaelrj): Move this into time/linux with the other syscalls.
 LLVM_LIBC_FUNCTION(int, clock_gettime,
                    (clockid_t clockid, struct timespec *tp)) {
   long ret_val =
       __llvm_libc::syscall_impl(SYS_clock_gettime, static_cast<long>(clockid),
                                 reinterpret_cast<long>(tp));
   // A negative return value indicates an error with the magnitude of the
   // value being the error code.
   if (ret_val < 0) {
     libc_errno = -ret_val;
     return -1;
   }
 
   return 0;
 }
 
 } // namespace __llvm_libc

libc/src/time/difftime.h

 
 namespace __llvm_libc {
 
 double difftime(time_t end, time_t beginning);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_DIFFTIME_H
-
-#include "include/time.h"

libc/src/time/gettimeofday.h

 
 namespace __llvm_libc {
 
 int gettimeofday(struct timeval *tv, void *tz);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_GETTIMEOFDAY_H
-
-#include "include/time.h"

libc/src/time/gettimeofday.cpp

 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 
 #include <sys/syscall.h> // For syscall numbers.
 
 namespace __llvm_libc {
 
+// TODO(michaelrj): Move this into time/linux with the other syscalls.
 LLVM_LIBC_FUNCTION(int, gettimeofday,
                    (struct timeval * tv, [[maybe_unused]] void *unused)) {
   if (tv == nullptr)
     return 0;
   struct timespec tp;
   long ret_val = __llvm_libc::syscall_impl(SYS_clock_gettime,
                                            static_cast<long>(CLOCK_REALTIME),
                                            reinterpret_cast<long>(&tp));

libc/src/time/gmtime.h

 
 namespace __llvm_libc {
 
 struct tm *gmtime(const time_t *timer);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_GMTIME_H
-
-#include "include/time.h"

libc/src/time/gmtime_r.h

 
 namespace __llvm_libc {
 
 struct tm *gmtime_r(const time_t *timer, struct tm *result);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_GMTIME_R_H
-
-#include "include/time.h"

libc/src/time/linux/clock.cpp

LLVM_LIBC_FUNCTION(clock_t, clock, ()) {
   struct timespec ts;
   long ret_val = __llvm_libc::syscall_impl(
       SYS_clock_gettime, CLOCK_PROCESS_CPUTIME_ID, reinterpret_cast<long>(&ts));
   if (ret_val < 0) {
     libc_errno = -ret_val;
     return clock_t(-1);
   }
 
-  // The above syscall gets the CPU time in seconds plus nanoseconds. We should
-  // make sure that corresponding clocks can actually be represented by clock-t.
+  // The above syscall gets the CPU time in seconds plus nanoseconds.
   // The standard requires that we return clock_t(-1) if we cannot represent
   // clocks as a clock_t value.
   constexpr clock_t CLOCK_SECS_MAX =
       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)
     return clock_t(-1);

libc/src/time/mktime.h

 
 namespace __llvm_libc {
 
 time_t mktime(struct tm *t1);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_MKTIME_H
-
-#include "include/time.h"

libc/src/time/nanosleep.h

 
 namespace __llvm_libc {
 
 int nanosleep(const struct timespec *req, struct timespec *rem);
 
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_TIME_NANOSLEEP_H
-
-#include "include/time.h"

libc/src/time/nanosleep.cpp

 
 #include "include/sys/syscall.h"          // For syscall numbers.
 #include "src/__support/OSUtil/syscall.h" // For syscall functions.
 #include "src/__support/common.h"
 #include "src/errno/libc_errno.h"
 
 namespace __llvm_libc {
 
+// TODO(michaelrj): Move this into time/linux with the other syscalls.
 LLVM_LIBC_FUNCTION(int, nanosleep,
                    (const struct timespec *req, struct timespec *rem)) {
   int ret = __llvm_libc::syscall_impl(SYS_nanosleep, req, rem);
   if (ret < 0) {
     libc_errno = -ret;
     return -1;
   }
   return ret;
 }
 
 } // namespace __llvm_libc

libc/src/time/time_utils.h

enum Month : int {
   SEPTEMBER,
   OCTOBER,
   NOVEMBER,
   DECEMBER
 };
 
 struct TimeConstants {
   static constexpr int SECONDS_PER_MIN = 60;
-  static constexpr int SECONDS_PER_HOUR = 3600;
-  static constexpr int SECONDS_PER_DAY = 86400;
   static constexpr int MINUTES_PER_HOUR = 60;
+  static constexpr int HOURS_PER_DAY = 24;
   static constexpr int DAYS_PER_WEEK = 7;
   static constexpr int MONTHS_PER_YEAR = 12;
   static constexpr int DAYS_PER_NON_LEAP_YEAR = 365;
   static constexpr int DAYS_PER_LEAP_YEAR = 366;
+
+  static constexpr int SECONDS_PER_HOUR = SECONDS_PER_MIN * MINUTES_PER_HOUR;
+  static constexpr int SECONDS_PER_DAY = SECONDS_PER_HOUR * HOURS_PER_DAY;
+  static constexpr int NUMBER_OF_SECONDS_IN_LEAP_YEAR =
+      DAYS_PER_LEAP_YEAR * SECONDS_PER_DAY;
+
   static constexpr int TIME_YEAR_BASE = 1900;
   static constexpr int EPOCH_YEAR = 1970;
   static constexpr int EPOCH_WEEK_DAY = 4;
-  static constexpr int NUMBER_OF_SECONDS_IN_LEAP_YEAR =
-      (DAYS_PER_NON_LEAP_YEAR + 1) * SECONDS_PER_DAY;
 
   // For asctime the behavior is undefined if struct tm's tm_wday or tm_mon are
   // not within the normal ranges as defined in <time.h>, or if struct tm's
   // tm_year exceeds {INT_MAX}-1990, or if the below asctime_internal algorithm
   // would attempt to generate more than 26 bytes of output (including the
   // terminating null).
   static constexpr int ASCTIME_BUFFER_SIZE = 256;
   static constexpr int ASCTIME_MAX_BYTES = 26;
 
   /* 2000-03-01 (mod 400 year, immediately after feb29 */
   static constexpr int64_t SECONDS_UNTIL2000_MARCH_FIRST =
       (946684800LL + SECONDS_PER_DAY * (31 + 29));
   static constexpr int WEEK_DAY_OF2000_MARCH_FIRST = 3;
 
   static constexpr int DAYS_PER400_YEARS =
-      (DAYS_PER_NON_LEAP_YEAR * 400 + (400 / 4) - 3);
+      (DAYS_PER_NON_LEAP_YEAR * 400) + (400 / 4) - 3;
   static constexpr int DAYS_PER100_YEARS =
-      (DAYS_PER_NON_LEAP_YEAR * 100 + (100 / 4) - 1);
-  static constexpr int DAYS_PER4_YEARS = (DAYS_PER_NON_LEAP_YEAR * 4 + 1);
+      (DAYS_PER_NON_LEAP_YEAR * 100) + (100 / 4) - 1;
+  static constexpr int DAYS_PER4_YEARS = (DAYS_PER_NON_LEAP_YEAR * 4) + 1;
 
   // The latest time that can be represented in this form is 03:14:07 UTC on
   // Tuesday, 19 January 2038 (corresponding to 2,147,483,647 seconds since the
   // start of the epoch). This means that systems using a 32-bit time_t type are
   // susceptible to the Year 2038 problem.
   static constexpr int END_OF32_BIT_EPOCH_YEAR = 2038;
 
   static constexpr time_t OUT_OF_RANGE_RETURN_VALUE = -1;
 };
 
 // Update the "tm" structure's year, month, etc. members from seconds.
 // "total_seconds" is the number of seconds since January 1st, 1970.
 extern int64_t update_from_seconds(int64_t total_seconds, struct tm *tm);
 
+// TODO(michaelrj): move these functions to use ErrorOr instead of setting
+// errno. They always accompany a specific return value so we only need the one
+// variable.
+
 // POSIX.1-2017 requires this.
 LIBC_INLINE time_t out_of_range() {
   libc_errno = EOVERFLOW;
   return static_cast<time_t>(-1);
 }
 
 LIBC_INLINE void invalid_value() { libc_errno = EINVAL; }
 
LIBC_INLINE char *asctime(const struct tm *timeptr, char *buffer,
 
   // TODO(rtenneti): i18n the following strings.
   static const char *week_days_name[TimeConstants::DAYS_PER_WEEK] = {
       "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
 
   static const char *months_name[TimeConstants::MONTHS_PER_YEAR] = {
       "Jan", "Feb", "Mar", "Apr", "May", "Jun",
       "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};
+
+  // TODO(michaelr): look into removing this call to __builtin_snprintf that may
+  // be emitted as a call to snprintf. Alternatively, look into using our
+  // internal printf machinery.
   int written_size = __builtin_snprintf(
       buffer, bufferLength, "%.3s %.3s%3d %.2d:%.2d:%.2d %d\n",
       week_days_name[timeptr->tm_wday], months_name[timeptr->tm_mon],
       timeptr->tm_mday, timeptr->tm_hour, timeptr->tm_min, timeptr->tm_sec,
       TimeConstants::TIME_YEAR_BASE + timeptr->tm_year);
   if (written_size < 0)
     return nullptr;
   if (static_cast<size_t>(written_size) >= bufferLength) {

libc/src/time/time_utils.cpp

int64_t update_from_seconds(int64_t total_seconds, struct tm *tm) {
   // Compute the number of 400 year cycles.
   int64_t numOfFourHundredYearCycles = days / TimeConstants::DAYS_PER400_YEARS;
   int64_t remainingDays = days % TimeConstants::DAYS_PER400_YEARS;
   if (remainingDays < 0) {
     remainingDays += TimeConstants::DAYS_PER400_YEARS;
     numOfFourHundredYearCycles--;
   }
 
-  // The reminder number of years after computing number of
+  // The remaining number of years after computing the number of
   // "four hundred year cycles" will be 4 hundred year cycles or less in 400
   // years.
   int64_t numOfHundredYearCycles = computeRemainingYears(
       TimeConstants::DAYS_PER100_YEARS, 4, &remainingDays);
 
-  // The reminder number of years after computing number of
+  // The remaining number of years after computing the number of
   // "hundred year cycles" will be 25 four year cycles or less in 100 years.
   int64_t numOfFourYearCycles =
       computeRemainingYears(TimeConstants::DAYS_PER4_YEARS, 25, &remainingDays);
 
-  // The reminder number of years after computing number of "four year cycles"
-  // will be 4 one year cycles or less in 4 years.
+  // The remaining number of years after computing the number of
+  // "four year cycles" will be 4 one year cycles or less in 4 years.
   int64_t remainingYears = computeRemainingYears(
       TimeConstants::DAYS_PER_NON_LEAP_YEAR, 4, &remainingDays);
 
   // Calculate number of years from year 2000.
   int64_t years = remainingYears + 4 * numOfFourYearCycles +
                   100 * numOfHundredYearCycles +
                   400LL * numOfFourHundredYearCycles;
 
   int leapDay =
       !remainingYears && (numOfFourYearCycles || !numOfHundredYearCycles);
 
+  // We add 31 and 28 for the number of days in January and February, since our
+  // starting point was March 1st.
   int64_t yday = remainingDays + 31 + 28 + leapDay;
   if (yday >= TimeConstants::DAYS_PER_NON_LEAP_YEAR + leapDay)
     yday -= TimeConstants::DAYS_PER_NON_LEAP_YEAR + leapDay;
 
   int64_t months = 0;
   while (daysInMonth[months] <= remainingDays) {
     remainingDays -= daysInMonth[months];
     months++;

libc/test/src/time/CMakeLists.txt

 add_custom_target(libc_time_unittests)
 
 add_libc_unittest(
-  asctime
+  asctime_test
   SUITE
     libc_time_unittests
   SRCS
     asctime_test.cpp
   HDRS
     TmHelper.h
     TmMatcher.h
   CXX_STANDARD
     20
   DEPENDS
     libc.src.time.asctime
 )
 
 add_libc_unittest(
-  asctime_r
+  asctime_r_test
   SUITE
     libc_time_unittests
   SRCS
     asctime_r_test.cpp
   HDRS
     TmHelper.h
     TmMatcher.h
   CXX_STANDARD
     20
   DEPENDS
     libc.src.time.asctime_r
 )
 
 add_libc_unittest(
-  clock_gettime
+  clock_gettime_test
   SUITE
     libc_time_unittests
   SRCS
   clock_gettime_test.cpp
   DEPENDS
     libc.src.time.clock_gettime
 )
 
 add_libc_unittest(
-  difftime
+  difftime_test
   SUITE
     libc_time_unittests
   SRCS
     difftime_test.cpp
   DEPENDS
     libc.src.time.difftime
 )
 
 add_libc_unittest(
-  gettimeofday
+  gettimeofday_test
   SUITE
     libc_time_unittests
   SRCS
     gettimeofday_test.cpp
   HDRS
     TmHelper.h
     TmMatcher.h
   CXX_STANDARD
     20
   DEPENDS
     libc.include.time
     libc.src.time.gettimeofday
     libc.src.time.nanosleep
     libc.src.errno.errno
 )
 
 add_libc_unittest(
-  gmtime
+  gmtime_test
   SUITE
     libc_time_unittests
   SRCS
     gmtime_test.cpp
   HDRS
     TmMatcher.h
   DEPENDS
     libc.src.time.gmtime
 )
 
 add_libc_unittest(
-  gmtime_r
+  gmtime_r_test
   SUITE
     libc_time_unittests
   SRCS
     gmtime_r_test.cpp
   HDRS
     TmMatcher.h
   DEPENDS
     libc.src.time.gmtime_r
 )
 
 add_libc_unittest(
-  mktime
+  mktime_test
   SUITE
     libc_time_unittests
   SRCS
     mktime_test.cpp
   HDRS
     TmHelper.h
     TmMatcher.h
   CXX_STANDARD
     20
   DEPENDS
     libc.src.time.mktime
 )
 
 add_libc_unittest(
-  nanosleep
+  nanosleep_test
   SUITE
     libc_time_unittests
   SRCS
     nanosleep_test.cpp
   HDRS
     TmHelper.h
     TmMatcher.h
   CXX_STANDARD