[libc] implement localtime #110363

zimirza · 2024-09-28T13:53:21Z

This is an implementation of localtime (currently not finished).

Closes #107597 and #109892

github-actions · 2024-09-28T13:57:21Z

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

libc/test/src/time/localtime_s_test.cpp

libc/src/time/time_utils.cpp

libc/src/time/timezone.cpp

zimirza · 2024-10-08T14:19:27Z

Instead of using /etc/timezone, it is better to parse /etc/localtime.

petrhosek · 2024-10-14T23:58:42Z

libc/src/time/time_utils.cpp

+  char timezone[TimeConstants::TIMEZONE_SIZE];
+  char *env_tz = getenv("TZ");
+  FILE *fp = NULL;
+  if (env_tz) {
+    strncpy(timezone, env_tz, sizeof(timezone));
+    timezone[sizeof(timezone) - 1] = '\0';
+  } else {
+    fp = fopen("/etc/timezone", "rb");
+    if (fp == NULL) {
+      return time_utils::out_of_range();
+    }
+
+    acquire_file(fp, timezone, TimeConstants::TIMEZONE_SIZE);
+  }
+
+  if (fp != NULL && file_usage == 0) {
+    release_file(fp, timezone);
+    return time_utils::out_of_range();
+  }
+
+  int offset = timezone::get_timezone_offset(timezone);


This logic should be moved into an internal utlity function that could be implemented differently for each platform. For example, on baremetal we don't have any filesystem and fopen isn't defined at all. Ideally, this function should return a struct type rather than a string so on platforms like baremetal we can return statically initialized global and avoid having to parse the string on every call (even in the Linux implementation you probably want to memoize it for performance reasons).

Sure, I am currently refactoring the logic by parsing /etc/localtime by using the TZif file format.

I am now using open instead of fopen to be compatible on other platforms.

It's not about open vs fopen, on platforms like baremetal there's no filesystem at all, so any use file descriptors or I/O operations like read is a non-starter. This needs to be refactored so the common logic can be used across all platforms, and the filesystem bits should live in platforms specific implementation (e.g. src/time/linux). On platforms where filesystem is available, we should be using the internal API like LIBC_NAMESPACE::openfile rather than the POSIX API.

Sure, I will refactor I/O operations. I think the best way would be to implement an "interface" such that all platforms could use. Where filesystem is available, I will use LIBC_NAMESPACE::openfile and use the "interface". Is this a good approach? Thank you for the help.

After investigating this, I think I understand the architecture. I will move all Linux to /src/time/linux and will implement a more platform independent approach.

I have now refactored to use the internal API openfile and localtime implementations are in src/time/linux directory. The timezone.cpp/timezone.h files are not in the linux directory. Should these also be there?

petrhosek · 2024-10-15T00:00:39Z

libc/src/time/time_utils.cpp

+volatile int file_usage = 0;
+
+void release_file(FILE *fp, char *timezone) {
+  file_usage = 0;
+  fclose(fp);
+}
+
+void acquire_file(FILE *fp, char *timezone, size_t timezone_size) {
+  while (1) {
+    if (file_usage == 0) {
+      file_usage = 1;
+      break;
+    }
+  }
+
+  if (fgets(timezone, (int)timezone_size, fp) == NULL) {
+    release_file(fp, timezone);
+  }
+}


The file related logic should ideally be wrapped in an RAII type to make sure the file is always closed.

I think I have now implemented an RAII correctly. Could you please check?

I don't see any RAII use, have you pushed all your changes?

I have an implementation in time_utils.cpp lines 30-46, where it is possible to get the file and release the file. Please do let me know if I have misunderstood RAII, so I can update this implementation.

zimirza · 2024-12-26T02:00:48Z

Even though this pull request is not yet finished, I thought it would be great to get a review since this pull request is getting quite big. There are some unit tests that are not working, and have been commented out.

libc/src/time/timezone.cpp

This is an implementation of localtime. Closes llvm#107597 and llvm#109892

format code with clang-format

fix: dst

update test for `localtime_r`

remove unnecessary code

fix: dst

compatibility reasons

statements

nickdesaulniers · 2025-01-07T18:52:35Z

(currently not finished)

Consider marking this as a "draft" in the github UI.

petrhosek · 2025-01-07T19:21:36Z

This is a large change and there are some aspects that will likely require more discussion, like what kind of TZ support we even want and how to do it, whether we could share the implementation with libc++, etc. This could be discussed either on Discourse or at the public LLVM-libc meeting (or both). In the meantime, I think the best path forward would be to split this change into multiple changes, with the first one being just an empty minimal implementation which could be merged while we're discussing the implementation details.

zimirza force-pushed the localtime_107597 branch 2 times, most recently from 6e15b2f to 15a5bef Compare September 28, 2024 14:09

zimirza mentioned this pull request Sep 30, 2024

[clang-tidy] add ctime and localtime to clang-tidy #110366

Draft

graphite-app bot reviewed Oct 6, 2024

View reviewed changes

libc/test/src/time/localtime_s_test.cpp Outdated Show resolved Hide resolved

graphite-app bot reviewed Oct 6, 2024

View reviewed changes

libc/src/time/time_utils.cpp Outdated Show resolved Hide resolved

graphite-app bot reviewed Oct 6, 2024

View reviewed changes

libc/src/time/time_utils.cpp Outdated Show resolved Hide resolved

zimirza force-pushed the localtime_107597 branch from 95ad2ec to 4e12c67 Compare October 6, 2024 20:38

graphite-app bot reviewed Oct 8, 2024

View reviewed changes

libc/src/time/time_utils.cpp Outdated Show resolved Hide resolved

zimirza force-pushed the localtime_107597 branch from 469939e to 47a3589 Compare October 8, 2024 13:07

graphite-app bot reviewed Oct 8, 2024

View reviewed changes

libc/src/time/timezone.cpp Outdated Show resolved Hide resolved

petrhosek reviewed Oct 14, 2024

View reviewed changes

petrhosek reviewed Oct 15, 2024

View reviewed changes

zimirza force-pushed the localtime_107597 branch from 5c51ae3 to 316e0a9 Compare December 23, 2024 03:59

zimirza marked this pull request as ready for review December 26, 2024 02:00

zimirza force-pushed the localtime_107597 branch 2 times, most recently from eca1d44 to 1456c67 Compare December 26, 2024 06:08

petrhosek reviewed Dec 26, 2024

View reviewed changes

libc/src/time/timezone.cpp Outdated Show resolved Hide resolved

petrhosek reviewed Dec 26, 2024

View reviewed changes

libc/src/time/timezone.cpp Outdated Show resolved Hide resolved

zimirza force-pushed the localtime_107597 branch from dada1e2 to ea3d0b9 Compare January 1, 2025 01:31

zimirza added 8 commits January 4, 2025 01:34

[libc] implement localtime

7e7a30a

This is an implementation of localtime. Closes llvm#107597 and llvm#109892

[libc] implement localtime

bfacc6f

format code with clang-format

[libc] implement localtime

6d565ce

fix: dst

[libc] implement localtime

7df4948

fix: dst

[libc] implement localtime

ce4b882

update test for `localtime_r`

[libc] implement localtime

3c0e852

remove unnecessary code

[libc] implement localtime

c5f841e

fix: dst

localtime_s is for windows only, implement localtime_s on gnu/linux for

6ca9533

compatibility reasons

zimirza and others added 15 commits January 4, 2025 01:34

format code with clang-format

41b397b

fix: dependencies for timezone for tests

3900559

fix: tests

52a8a7e

refactor: fix tests

68671c6

refactor: localtime

40f1878

uncomment tests for asctime

4d07cc9

undo asctime

e0da6dc

format code with clang-format

498bd97

undo asctime

ff20268

fix: ctime_r

4c8e204

format code with clang-format

f229f83

refactor: use openfile

83206c7

fix: add header file for time_utils in localtime

27151cc

fix localtime

1584628

fix: fixed tests for asctime and removed unnecessary header include

6a84f62

statements

zimirza force-pushed the localtime_107597 branch from 2f14147 to 6a84f62 Compare January 3, 2025 20:34

zimirza added 11 commits January 4, 2025 15:37

refactor: move linux specific implementation to libc/src/time/linux/

47ec45b

format code with clang-format

3d1dc2e

fix: tests

7d71495

fix: tests

49fe17a

fix: add if macro for linux implementations

1953d82

fix: check if timer variable is nullptr

539552f

fix: return type for localtime_s implementation

1f0f358

fix: tests for difftime

9ec5990

use LIBC_TARGET_OS for platform independent implementations

a604387

fix tests for clock

f7dc9a2

add localtime

c7e8bce

nickdesaulniers self-requested a review January 7, 2025 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc] implement localtime #110363

[libc] implement localtime #110363

zimirza commented Sep 28, 2024

github-actions bot commented Sep 28, 2024 •

edited

Loading

zimirza commented Oct 8, 2024

petrhosek Oct 14, 2024

zimirza Oct 15, 2024

zimirza Dec 26, 2024

petrhosek Dec 26, 2024

zimirza Dec 28, 2024

zimirza Dec 28, 2024

zimirza Jan 1, 2025

petrhosek Oct 15, 2024

zimirza Dec 26, 2024

petrhosek Dec 26, 2024

zimirza Jan 1, 2025

zimirza commented Dec 26, 2024

nickdesaulniers commented Jan 7, 2025

petrhosek commented Jan 7, 2025

[libc] implement localtime #110363

Are you sure you want to change the base?

[libc] implement localtime #110363

Conversation

zimirza commented Sep 28, 2024

github-actions bot commented Sep 28, 2024 • edited Loading

zimirza commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimirza commented Dec 26, 2024

nickdesaulniers commented Jan 7, 2025

petrhosek commented Jan 7, 2025

github-actions bot commented Sep 28, 2024 •

edited

Loading