[libc] Add `ctime_s` #110676

Rajveer100 · 2024-10-01T13:44:10Z

Definition:

int ctime_s(char *buffer, rsize_t buffer_size, const time_t *t_ptr);

llvmbot · 2024-10-01T13:44:48Z

@llvm/pr-subscribers-libc

Author: Rajveer Singh Bharadwaj (Rajveer100)

Changes

Resolves #110548

Definition:

int ctime_s(char *buffer, size_t buffer_size, const time_t *t_ptr);

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

2 Files Affected:

(added) libc/src/time/ctime_s.cpp (+39)
(added) libc/src/time/ctime_s.h (+21)

diff --git a/libc/src/time/ctime_s.cpp b/libc/src/time/ctime_s.cpp new file mode 100644 index 00000000000000..407e241ab9ef99 --- /dev/null +++ b/libc/src/time/ctime_s.cpp @@ -0,0 +1,39 @@ +//===-- Implementation of ctime_s function --------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ctime_s.h" +#include "src/__support/CPP/limits.h" +#include "src/__support/common.h" +#include "src/__support/macros/config.h" +#include "time_utils.h" +#include <cerrno> + +namespace LIBC_NAMESPACE_DECL { + +using LIBC_NAMESPACE::time_utils::TimeConstants; + +LLVM_LIBC_FUNCTION(int, ctime_s, + (char *buffer, size_t buffer_size, const time_t *t_ptr)) { + if (t_ptr == nullptr || buffer == nullptr || + *time > cpp::numeric_limits<int32_t>::max()) { + return EINVAL; + } + + if (buffer_size < TimeConstants::ASCTIME_MAX_BYTES) { + return ERANGE; + } + + if (time_utils::asctime(time_utils::localtime(t_ptr), buffer, buffer_size) == + nullptr) { + return EINVAL; + } + + return 0; +} + +} // namespace LIBC_NAMESPACE_DECL diff --git a/libc/src/time/ctime_s.h b/libc/src/time/ctime_s.h new file mode 100644 index 00000000000000..023c4b5b3cbc7f --- /dev/null +++ b/libc/src/time/ctime_s.h @@ -0,0 +1,21 @@ +//===-- Implementation header of ctime_s ------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_LIBC_SRC_TIME_CTIME_S_H +#define LLVM_LIBC_SRC_TIME_CTIME_S_H + +#include "hdr/types/time_t.h" +#include "src/__support/macros/config.h" + +namespace LIBC_NAMESPACE_DECL { + +int ctime_s(char *buffer, size_t buffer_size, const time_t *t_ptr); + +} // namespace LIBC_NAMESPACE_DECL + +#endif // LLVM_LIBC_SRC_TIME_CTIME_S_H

nickdesaulniers

Thanks for the patch!

In addition to the implementation, we'll need to add cmake modifications to build it, and unit tests so that we have some level of confidence in the implementation.

libc/src/time/ctime_s.cpp

zimirza

This is great. Thank you for the pull request. Could you also add the following:

We also need to add ctime_s to the cmake file so it will get included in the build, libc/src/time/CMakeLists.txt.
We should also add ctime_s to the different tested architectures in libc/config/<...>/<...>/entrypoints.txt.
newhdrgen: libc/newhdrgen/yaml/time.yaml
spec file: libc/spec/stdc.td
Could you also add some tests? These are in libc/test/src/time/. Make sure to include the tests in the cmake file, libc/test/src/time/CMakeLists.txt. You can run tests for this with ninja or make, ninja libc.test.src.time.ctime_s_test.

Rajveer100 · 2024-10-01T18:47:56Z

In the entry points of some of the architectures, time files are missing, is this intended?

libc/src/time/ctime_s.cpp

michaelrj-google · 2024-10-01T23:20:49Z

I'd recommend not adding entrypoints to any architectures you haven't tested the functions on.

github-actions · 2024-10-02T12:23:31Z

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

libc/spec/stdc.td

zimirza · 2024-10-02T16:14:01Z

I'd recommend not adding entrypoints to any architectures you haven't tested the functions on.

That is correct. I have updated the list in my comment.

Rajveer100 · 2024-10-02T16:19:14Z

I have added the entry points only in places where ctime_r (and other time files) exist.

I believe this should be fine?

libc/src/time/ctime_s.cpp

Rajveer100 · 2024-10-04T07:30:48Z

#111108

Rajveer100 · 2024-10-07T16:16:36Z

@nickdesaulniers @michaelrj-google

michaelrj-google

LGTM

Rajveer100 · 2024-10-08T05:04:14Z

Thanks for the approval, could you land this for me?!

zimirza · 2024-10-08T06:21:50Z

This looks good to me as well.

nickdesaulniers

This patch doesn't compile for me. Mind taking a look?

libc/src/time/ctime_s.h

libc/src/time/ctime_s.cpp

Rajveer100 · 2024-10-08T16:38:35Z

@nickdesaulniers
Which OS are you in currently?

nickdesaulniers · 2024-10-08T16:43:09Z

linux

Rajveer100 · 2024-10-11T12:08:56Z

Why are there no entry points for time files in Darwin (macOS)?

michaelrj-google · 2024-10-11T23:15:28Z

MacOS currently has minimal support since we don't have any users who need it. I'm hoping that soon we'll get a buildbot working for MacOS, which will help with keeping it in a working state.

There are also some issues around fullbuild on MacOS, which if I remember correctly boil down to "Apple doesn't want you calling syscalls directly, they want you to use their libc".

Rajveer100 · 2024-10-12T05:17:46Z

Makes sense, I did face errors when trying to do so.

Rajveer100 · 2024-11-15T11:17:34Z

Sorry for the number of iterations in code review. Stick with it though!

No worries! Apart from the constraint violation part, I have done rest of the changes.

libc/src/time/ctime_s.cpp

libc/test/src/time/ctime_s_test.cpp

libc/src/time/ctime_s.h

libc/test/src/time/ctime_s_test.cpp

libc/include/time.yaml

libc/src/time/ctime_s.h

Rajveer100 · 2025-01-08T13:56:59Z

I have made the changes, although a little unsure about the build failure?

nickdesaulniers

Sorry all of the intricacies around STDC_WANT_LIB_EXT1. Annex K is somewhat taboo, and you're laying much of the groundwork here for supporting it.

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

libc/hdr/types/errno_t.h

libc/src/time/CMakeLists.txt

libc/src/time/ctime_s.cpp

libc/test/src/time/ctime_s_test.cpp

libc/include/time.yaml

Resolves llvm#110548 Definition: ```c++ int ctime_s(char *buffer, rsize_t buffer_size, const time_t *t_ptr); ```

nickdesaulniers

ok, thanks for working through the feedback. I think this is ready to land. Do you need us to merge this for you?

Rajveer100 · 2025-01-10T05:47:50Z

Sure, I don't have access to merge :)

nickdesaulniers

I'm going to reread those two WG 21 papers to better familiarize myself towards the argument for Annex K.

For now, I'm going to request changes to block this from landing until I make a decision wrt. Annex K support. I believe we currently don't support Annex K at all, so I need to think hard about whether we intend to open this can of worms.

nickdesaulniers · 2025-01-10T18:13:32Z

libc/src/time/ctime_s.cpp

+ return EINVAL;
+
+ if (buffer_size < time_constants::ASCTIME_MAX_BYTES ||
+ buffer_size > RSIZE_MAX) {


Hmm...so I went to test this with GCC and found that:

/android0/llvm-project/libc/src/time/ctime_s.cpp:30:21: error: ‘RSIZE_MAX’ was not declared in this scope; did you mean ‘SIZE_MAX’? 30 | buffer_size > RSIZE_MAX) { | ^~~~~~~~~ | SIZE_MAX

Looking into it, I found that clang's stdint provides RSIZE_MAX (example: /usr/lib/llvm-16/lib/clang/16/include/stdint.h), but GCC does not (example: /usr/lib/gcc/x86_64-linux-gnu/14/include/stdint.h).

Grepping gcc's sources for __STDC_LIB_EXT1__ turns up no hits (one hit in the test suite that looks irrelevant). So looking into why there's seemingly no support for Annex K turned up this mailing list thread: https://gcc.gnu.org/pipermail/gcc/2019-December/231068.html

Which links to this paper documenting in explicit detail glibc developers experience with trying to implement Annex K: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1967.htm

95f1de3 added support for RSIZE_MAX and rsize_t without mentioning who was interested in Annex K support.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2336.pdf looks like a more recent rebuttal to n1967.

llvmbot added the libc label Oct 1, 2024

nickdesaulniers reviewed Oct 1, 2024

View reviewed changes

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

zimirza suggested changes Oct 1, 2024

View reviewed changes

lntue reviewed Oct 1, 2024

View reviewed changes

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

Rajveer100 force-pushed the libc-ctime_s branch from 269dd2c to 6db439c Compare October 2, 2024 12:19

Rajveer100 requested a review from nickdesaulniers October 2, 2024 12:22

Rajveer100 force-pushed the libc-ctime_s branch from 6db439c to bef8896 Compare October 2, 2024 12:24

Rajveer100 requested review from lntue and zimirza October 2, 2024 12:29

SchrodingerZhu reviewed Oct 2, 2024

View reviewed changes

libc/spec/stdc.td Outdated Show resolved Hide resolved

Rajveer100 force-pushed the libc-ctime_s branch from bef8896 to c7692cc Compare October 2, 2024 18:31

michaelrj-google reviewed Oct 2, 2024

View reviewed changes

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

michaelrj-google approved these changes Oct 7, 2024

View reviewed changes

zimirza approved these changes Oct 8, 2024

View reviewed changes

nickdesaulniers requested changes Oct 8, 2024

View reviewed changes

libc/src/time/ctime_s.h Outdated Show resolved Hide resolved

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

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

Rajveer100 force-pushed the libc-ctime_s branch from 00248c1 to 1cede39 Compare November 17, 2024 07:53

Rajveer100 requested a review from nickdesaulniers November 17, 2024 07:53

Rajveer100 force-pushed the libc-ctime_s branch from 1cede39 to f7971dd Compare November 19, 2024 07:28

nickdesaulniers reviewed Nov 19, 2024

View reviewed changes

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

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

Rajveer100 force-pushed the libc-ctime_s branch 2 times, most recently from 73a5d71 to 73ad63f Compare November 20, 2024 12:22

Rajveer100 requested a review from nickdesaulniers November 20, 2024 12:27

nickdesaulniers reviewed Nov 20, 2024

View reviewed changes

libc/src/time/ctime_s.h Outdated Show resolved Hide resolved

libc/src/time/ctime_s.h Outdated Show resolved Hide resolved

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

Rajveer100 requested a review from nickdesaulniers November 26, 2024 11:31

Rajveer100 force-pushed the libc-ctime_s branch from 73ad63f to fd7a972 Compare January 7, 2025 10:49

nickdesaulniers requested changes Jan 7, 2025

View reviewed changes

libc/include/time.yaml Outdated Show resolved Hide resolved

libc/include/time.yaml Outdated Show resolved Hide resolved

libc/src/time/ctime_s.h Outdated Show resolved Hide resolved

Rajveer100 force-pushed the libc-ctime_s branch 6 times, most recently from a5c4480 to 2c47f7d Compare January 8, 2025 13:30

nickdesaulniers requested changes Jan 8, 2025

View reviewed changes

Rajveer100 force-pushed the libc-ctime_s branch 3 times, most recently from 77dc895 to 5006fad Compare January 9, 2025 10:30

Rajveer100 requested a review from nickdesaulniers January 9, 2025 10:43

[libc] Add ctime_s

1a7dd36

Resolves llvm#110548 Definition: ```c++ int ctime_s(char *buffer, rsize_t buffer_size, const time_t *t_ptr); ```

Rajveer100 force-pushed the libc-ctime_s branch from 5006fad to 1a7dd36 Compare January 9, 2025 18:52

nickdesaulniers approved these changes Jan 9, 2025

View reviewed changes

nickdesaulniers requested changes Jan 10, 2025

View reviewed changes

zimirza mentioned this pull request Apr 30, 2025

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

Open

Uh oh!

[libc] Add ctime_s #110676

Are you sure you want to change the base?

[libc] Add ctime_s #110676

Uh oh!

Conversation

Rajveer100 commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

llvmbot commented Oct 1, 2024

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

zimirza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Rajveer100 commented Oct 1, 2024

Uh oh!

michaelrj-google commented Oct 1, 2024

github-actions bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimirza commented Oct 2, 2024

Rajveer100 commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rajveer100 commented Oct 4, 2024

Rajveer100 commented Oct 7, 2024

michaelrj-google left a comment

Choose a reason for hiding this comment

Rajveer100 commented Oct 8, 2024

zimirza commented Oct 8, 2024

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Rajveer100 commented Oct 8, 2024

nickdesaulniers commented Oct 8, 2024

Rajveer100 commented Oct 11, 2024

michaelrj-google commented Oct 11, 2024

Rajveer100 commented Oct 12, 2024

Rajveer100 commented Nov 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rajveer100 commented Jan 8, 2025

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers left a comment

Choose a reason for hiding this comment

Rajveer100 commented Jan 10, 2025

nickdesaulniers left a comment

Choose a reason for hiding this comment

nickdesaulniers Jan 10, 2025

Choose a reason for hiding this comment

Labels

7 participants

[libc] Add `ctime_s` #110676

[libc] Add `ctime_s` #110676

Rajveer100 commented Oct 1, 2024 •

edited

Loading

zimirza left a comment •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

Rajveer100 commented Oct 2, 2024 •

edited

Loading