Repository navigation
feat(libc): implement asctime_r - #12841
Conversation
|
@heditrigui The implementation is correct and small: The entry for strftime and asctime says these "follow a payload process on a PS5 (firmware 13.60) in the C locale" and you added |
|
@oneandonlydean Thanks! Kept \�sctime_r\ out of the measured list in \libc.md, added the note that its formatting is shared with \�sctime\ without console measurement and that returning |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
rooklz
left a comment
rooklz
left a comment
There was a problem hiding this comment.
Reviewed head d690c78347 against main 157b02c4f1. I ran this on macOS arm64 with Apple clang. No console was used.
What I checked
- The NID is right:
python3 tools/nid_names.py --name asctime_rgives488p6whx47U.mainhas no otherasctime_r. git merge-tree --write-tree upstream/main <head>merges cleanly, andgit diff --checkis clean.check_conventions.py --base upstream/mainreports 0 errors but 1 warning:docs/dev/debt/libc.md:202: [debt-length] ... 1047 characters. The line was 909 characters onmain, so this PR pushes it over the 1000 limit.pr_overlap.py --branch <head>(1400 PRs): there are no same-export conflicts. Three PRs (#10895, #11843, #12593) touch the neighbouring debt lines 201-203. #10409 (CONFLICTING with main) touchesTime.cppand both tests, but it doesn't changeasctime.- I built a standalone ASan+UBSan harness from the verbatim
asctime_r/asctimefromTime.cpp,clampedField/tableName, and the PR'sCheckAsctimerows fromGuestStrftime, plus the null-tm and null-buffer rows. It passes. The fixed26is safe, because every field is clamped (Fri Oct 99 99:99:99 9999\nis 25 characters plus the NUL).
Mutants (single edits to the new function):
| Mutant | Result |
|---|---|
| Drop the null-buffer check | killed (ASan) |
snprintf size 25 |
killed |
snprintf size 64 |
survives, but it's equivalent (output never exceeds 26) |
Drop errno = EINVAL |
killed |
| Null tm returns nullptr | killed |
| Success returns nullptr | killed |
Requested change: a null buffer should throw instead of returning nullptr. The debt line says "the null-buffer result is assumed". CONTRIBUTING asks for unknown behaviour to throw and be recorded, not for an assumed result. FreeBSD's asctime_r doesn't check the buffer at all, so nullptr isn't attested anywhere. Suggested change:
- if (buffer == nullptr) return nullptr;
+ if (buffer == nullptr) throw std::invalid_argument("asctime_r: null buffer");<stdexcept> is already included in Time.cpp. The test in GuestTimeConversion.cpp would become:
- Require(asctime_r_nid_postfix(&roundTrip, nullptr) == nullptr, "asctime_r accepted a null buffer");
+ bool nullBufferThrew = false;
+ try {
+ asctime_r_nid_postfix(&roundTrip, nullptr);
+ } catch (const std::invalid_argument&) {
+ nullBufferThrew = true;
+ }
+ Require(nullBufferThrew, "asctime_r accepted a null buffer");The debt sentence then shrinks below the limit (1047 → 987 characters):
`asctime_r` shares the `asctime` formatting, unmeasured; a null buffer throws
I checked this variant in the same harness: it passes, and the current head fails the new row.
Minor point: the "Tested" section lists only check_conventions. Please add which ctest targets (GuestStrftime, GuestTimeConversion) you ran and on which OS.
d690c78 to
e908061
Compare
|
@rooklz Thanks for the thorough review! Updated �sctime_r to throw std::invalid_argument("asctime_r: null buffer") when given a null buffer per CONTRIBUTING conventions, updated the unit test in GuestTimeConversion.cpp to assert that it throws, shortened the debt note in docs/dev/debt/libc.md to 987 characters, and updated the PR body with the test targets and platform details. |
rooklz
left a comment
rooklz
left a comment
There was a problem hiding this comment.
Re-reviewed head e908061e32 against main 3adb909203 on macOS arm64 with Apple clang. I didn't use a console.
The change since my last review matches what I asked for. A null buffer now throws std::invalid_argument before anything is written. There's a test row for it, and the debt sentence is short again: line 202 is 987 characters, and check_conventions passes with no warnings.
- Standalone ASan+UBSan harness. It takes
asctime_r/asctimeverbatim fromTime.cppwithclampedField/tableName/the name tables, plus everyCheckAsctimerow fromGuestStrftime, the null-tm and null-buffer rows fromGuestTimeConversion, and one extra widest-output row (Fri Oct 99 99:99:99 9999\n, 25 characters plus the NUL, which fits the fixed26). It passes. - Mutants: 7 of 7 killed. They were: the null-buffer throw dropped,
snprintfsize 25,errno = EINVALdropped, null-tm returningnullptr, the success path returningnullptr, the seconds field swapped for minutes, and the year modulus 100000. git diff --check,merge-treeagainst3adb909203andcheck_conventions --base upstream/mainare all clean.pr_overlapprobed 1407 PRs and found no conflicting files and no same exports. The only ones listed are PRs that already conflict withmainand shareApi.hpp/libc.md.
LGTM.
AI-assisted: yes
|
Windows run of this head (
AI-assisted: yes (Claude Code). |
|
Reviewed at The latest main merge resolved the conflict in docs/dev/debt/libc.md by rewriting lines 226 and 227 with their formatting stripped:
Line 227 (the wcsftime entry) is main's text, which this PR didn't change before. Could you take both lines from |
|
@oneandonlydean Thank you for catching that! The conflict resolution inadvertently stripped the backticks and evaluated the escape sequences. Updated in head
|
✅ AMD GPU test: passedPR head
Unstable on main (8, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (11, not counted against this PR)Passed on retry (3, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (8 IDs in 777 tests)Skipped (62)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
Implement POSIX / FreeBSD \�sctime_r\ (NID \488p6whx47U) in \libc, formatting broken-down time into a caller-supplied buffer with identical formatting to \�sctime. Refactor \�sctime\ to delegate to \�sctime_r\ via a thread-local buffer, throw \std::invalid_argument\ when given a null buffer, and verify formatting and error handling in \GuestStrftime\ and \GuestTimeConversion.
Tested
Checklist