Merge "localtime_r(3) should act as if it calls tzset(3)."

benchmarks/time_benchmark.cpp

@@ -40,7 +40,7 @@ BENCHMARK(BM_time_clock_gettime_syscall);
 static void BM_time_gettimeofday(benchmark::State& state) {
   timeval tv;
   while (state.KeepRunning()) {
-    gettimeofday(&tv, NULL);
+    gettimeofday(&tv, nullptr);
   }
 }
 BENCHMARK(BM_time_gettimeofday);
@@ -48,14 +48,31 @@ BENCHMARK(BM_time_gettimeofday);
 void BM_time_gettimeofday_syscall(benchmark::State& state) {
   timeval tv;
   while (state.KeepRunning()) {
-    syscall(__NR_gettimeofday, &tv, NULL);
+    syscall(__NR_gettimeofday, &tv, nullptr);
   }
 }
 BENCHMARK(BM_time_gettimeofday_syscall);
 
 void BM_time_time(benchmark::State& state) {
   while (state.KeepRunning()) {
-    time(NULL);
+    time(nullptr);
   }
 }
 BENCHMARK(BM_time_time);
+
+void BM_time_localtime(benchmark::State& state) {
+  time_t t = time(nullptr);
+  while (state.KeepRunning()) {
+    localtime(&t);
+  }
+}
+BENCHMARK(BM_time_localtime);
+
+void BM_time_localtime_r(benchmark::State& state) {
+  time_t t = time(nullptr);
+  while (state.KeepRunning()) {
+    struct tm tm;
+    localtime_r(&t, &tm);
+  }
+}
+BENCHMARK(BM_time_localtime_r);

libc/tzcode/localtime.c

@@ -1327,21 +1327,22 @@ tzset_unlocked(void)
 
   // If that's not set, look at the "persist.sys.timezone" system property.
   if (name == NULL) {
+    // The lookup is the most expensive part by several orders of magnitude, so we cache it.
+    // We check for null more than once because the system property may not have been set
+    // yet, so our first lookup may fail.
     static const prop_info* pi;
+    if (pi == NULL) pi = __system_property_find("persist.sys.timezone");
 
-    if (!pi) {
-      pi = __system_property_find("persist.sys.timezone");
-    }
     if (pi) {
-      static char buf[PROP_VALUE_MAX];
-      static uint32_t s = -1;
-      static bool ok = false;
+      // If the property hasn't changed since the last time we read it, there's nothing else to do.
+      static uint32_t last_serial = -1;
       uint32_t serial = __system_property_serial(pi);
-      if (serial != s) {
-        ok = __system_property_read(pi, 0, buf) > 0;
-        s = serial;
-      }
-      if (ok) {
+      if (serial == last_serial) return;
+
+      // Otherwise read the new value...
+      last_serial = serial;
+      char buf[PROP_VALUE_MAX];
+      if (__system_property_read(pi, NULL, buf) > 0) {
         // POSIX and Java disagree about the sign in a timezone string. For POSIX, "GMT+3" means
         // "3 hours west/behind", but for Java it means "3 hours east/ahead". Since (a) Java is
         // the one that matches human expectations and (b) this system property is used directly
@@ -1532,16 +1533,22 @@ localtime_rz(struct state *sp, time_t const *timep, struct tm *tmp)
 #endif
 
 static struct tm *
-localtime_tzset(time_t const *timep, struct tm *tmp, bool setname)
+localtime_tzset(time_t const *timep, struct tm *tmp)
 {
   int err = lock();
   if (err) {
     errno = err;
     return NULL;
   }
-  if (setname || !lcl_is_set)
+
+  // http://b/31339449: POSIX says localtime(3) acts as if it called tzset(3), but upstream
+  // and glibc both think it's okay for localtime_r(3) to not do so (presumably because of
+  // the "not required to set tzname" clause). It's unclear that POSIX actually intended this,
+  // the BSDs disagree with glibc, and it's confusing to developers to have localtime_r(3)
+  // behave differently than other time zone-sensitive functions in <time.h>.
   tzset_unlocked();
-  tmp = localsub(lclptr, timep, setname, tmp);
+
+  tmp = localsub(lclptr, timep, true, tmp);
   unlock();
   return tmp;
 }
@@ -1549,13 +1556,13 @@ localtime_tzset(time_t const *timep, struct tm *tmp, bool setname)
 struct tm *
 localtime(const time_t *timep)
 {
-  return localtime_tzset(timep, &tm, true);
+  return localtime_tzset(timep, &tm);
 }
 
 struct tm *
 localtime_r(const time_t *timep, struct tm *tmp)
 {
-  return localtime_tzset(timep, tmp, false);
+  return localtime_tzset(timep, tmp);
 }
 
 /*

tests/time_test.cpp

@@ -674,3 +674,39 @@ TEST(time, bug_31938693) {
   ASSERT_TRUE(localtime_r(&t, &tm) != nullptr);
   EXPECT_EQ(9, tm.tm_hour);
 }
+
+TEST(time, bug_31339449) {
+  // POSIX says localtime acts as if it calls tzset.
+  // tzset does two things:
+  //  1. it sets the time zone ctime/localtime/mktime/strftime will use.
+  //  2. it sets the global `tzname`.
+  // POSIX says localtime_r need not set `tzname` (2).
+  // Q: should localtime_r set the time zone (1)?
+  // Upstream tzcode (and glibc) answer "no", everyone else answers "yes".
+
+  // Pick a time, any time...
+  time_t t = 1475619727;
+
+  // Call tzset with a specific timezone.
+  setenv("TZ", "America/Atka", 1);
+  tzset();
+
+  // If we change the timezone and call localtime, localtime should use the new timezone.
+  setenv("TZ", "America/Los_Angeles", 1);
+  struct tm* tm_p = localtime(&t);
+  EXPECT_EQ(15, tm_p->tm_hour);
+
+  // Reset the timezone back.
+  setenv("TZ", "America/Atka", 1);
+  tzset();
+
+#if defined(__BIONIC__)
+  // If we change the timezone again and call localtime_r, localtime_r should use the new timezone.
+  setenv("TZ", "America/Los_Angeles", 1);
+  struct tm tm = {};
+  localtime_r(&t, &tm);
+  EXPECT_EQ(15, tm.tm_hour);
+#else
+  // The BSDs agree with us, but glibc gets this wrong.
+#endif
+}