localtime_r(3) should act as if it calls tzset(3).
See code comment.
Bug: http://b/31339449
Test: ran tests & benchmarks
Change-Id: I6b6a63750ef41664dc4698207e6a53e77cc28cdf
diff --git a/benchmarks/time_benchmark.cpp b/benchmarks/time_benchmark.cpp
index 4a5c2da..c90dfad 100644
--- a/benchmarks/time_benchmark.cpp
+++ b/benchmarks/time_benchmark.cpp
@@ -40,7 +40,7 @@
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 @@
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);
diff --git a/libc/tzcode/localtime.c b/libc/tzcode/localtime.c
index 74411f7..723e4f2 100644
--- a/libc/tzcode/localtime.c
+++ b/libc/tzcode/localtime.c
@@ -1327,21 +1327,22 @@
// 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 @@
#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)
- tzset_unlocked();
- tmp = localsub(lclptr, timep, setname, tmp);
+
+ // 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, true, tmp);
unlock();
return tmp;
}
@@ -1549,13 +1556,13 @@
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);
}
/*
diff --git a/tests/time_test.cpp b/tests/time_test.cpp
index 914cb61..8c4a8a9 100644
--- a/tests/time_test.cpp
+++ b/tests/time_test.cpp
@@ -674,3 +674,39 @@
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
+}