Skip to content

feat(libc): implement timelocal - #11843

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
heditrigui:feat/libc-timelocal
Nov 2, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
heditrigui:feat/libc-timelocal

Conversation

@heditrigui

Copy link
Copy Markdown
Contributor

What

Implement timelocal in libc as an alias to libc_mktime_nid_postfix matching FreeBSD/POSIX behavior, and add test coverage in GuestMktime.cpp.

Tested

Ran python tools/check_conventions.py --base upstream/main cleanly (0 errors).
Ran python core/libs/tests/test_public_headers.py --audit-only cleanly.
Ran python tools/progress.py verifying progress increase (libraries 6567/13449 -> 6568/13450).

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Progress report (9f748f4 → 9baa878)

📈 System libraries: 27.27% (+0.01%, +1 implemented functions)

✅ 1 implemented
Library Name
libc timelocal_nid_postfix

Changed libraries (implemented exports)

Library Before After Change
libc 1480/1496 (98.93%) 1481/1497 (98.93%) +0.0007%

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@heditrigui Please check timelocal against FreeBSD 11.4 lib/libc/stdtime/localtime.c. If it sets tm_isdst = -1 before converting, as I expect, do the same (and keep the failure path leaving the struct unchanged apart from that), and add a case under a DST zone such as EST5EDT where the input has tm_isdst 0 or 1 in summer, comparing timelocal with mktime. If the source shows a plain alias, say so in the debt entry and cite the line.

@heditrigui

Copy link
Copy Markdown
Contributor Author

@oneandonlydean Checked FreeBSD 11 (contrib/tzcode/stdtime/localtime.c lines 2126-2132): timelocal indeed sets tmp->tm_isdst = -1 before calling mktime. Updated timelocal to set tm_isdst = -1 (preserving input fields on failure), cited lines 2126-2132 in the technical debt entry, and added CheckTimelocalDst in GuestMktime.cpp verifying under EST5EDT that summer input with tm_isdst=0 produces the active EDT timestamp (1 hour earlier than mktime's forced standard-time result).

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 10409 core/libs/tests/GuestMktime.cpp L126-127
PR 12593 docs/dev/debt/libc.md L235

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.

Pull request Shared files not compared
PR 5021 core/libs/prx/libc/src/Time.cpp
docs/dev/debt/libc.md
PR 5303 docs/dev/debt/libc.md
PR 6565 docs/dev/debt/libc.md
PR 7081 docs/dev/debt/libc.md
PR 7386 docs/dev/debt/libc.md
PR 7447 docs/dev/debt/libc.md
PR 7715 docs/dev/debt/libc.md
PR 8150 docs/dev/debt/libc.md
PR 8965 docs/dev/debt/libc.md
PR 9340 docs/dev/debt/libc.md
PR 9371 docs/dev/debt/libc.md
PR 9447 core/libs/prx/libc/src/Time.cpp
docs/dev/debt/libc.md
PR 9526 docs/dev/debt/libc.md
PR 9929 docs/dev/debt/libc.md
PR 9964 docs/dev/debt/libc.md
PR 10113 docs/dev/debt/libc.md
PR 10226 docs/dev/debt/libc.md
PR 10245 docs/dev/debt/libc.md
PR 10250 docs/dev/debt/libc.md
PR 10253 docs/dev/debt/libc.md
PR 10265 docs/dev/debt/libc.md
PR 10274 docs/dev/debt/libc.md
PR 10315 docs/dev/debt/libc.md
PR 10325 docs/dev/debt/libc.md
PR 10328 docs/dev/debt/libc.md
PR 10345 docs/dev/debt/libc.md
PR 10349 docs/dev/debt/libc.md
PR 10405 docs/dev/debt/libc.md
PR 10506 docs/dev/debt/libc.md
PR 10510 docs/dev/debt/libc.md
PR 10512 docs/dev/debt/libc.md
PR 10565 docs/dev/debt/libc.md
PR 10987 core/libs/prx/libc/src/Time.cpp
docs/dev/debt/libc.md
PR 11260 docs/dev/debt/libc.md
PR 11262 docs/dev/debt/libc.md
PR 11266 docs/dev/debt/libc.md
PR 11332 docs/dev/debt/libc.md
PR 11831 core/libs/prx/libc/include/Api.hpp
docs/dev/debt/libc.md
PR 11907 docs/dev/debt/libc.md
PR 12057 docs/dev/debt/libc.md
PR 12122 docs/dev/debt/libc.md
PR 12292 docs/dev/debt/libc.md
PR 12397 docs/dev/debt/libc.md
PR 12514 docs/dev/debt/libc.md
PR 12560 docs/dev/debt/libc.md
PR 12574 docs/dev/debt/libc.md
PR 12627 docs/dev/debt/libc.md
PR 13390 docs/dev/debt/libc.md
PR 13804 docs/dev/debt/libc.md
PR 14212 docs/dev/debt/libc.md
PR 14373 core/libs/prx/libc/include/Api.hpp
docs/dev/debt/libc.md
PR 14585 core/libs/prx/libc/include/Api.hpp
docs/dev/debt/libc.md
PR 14610 core/libs/prx/libc/include/Api.hpp
docs/dev/debt/libc.md

@maskym

maskym commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Windows result for head 2a46544885, merged into main 75f3702aed (the PR's Tested section names no Windows run).

Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, BUILD_TESTING=ON, Ninja. Build clean.

guest_mktime aborts in the new CheckTimelocalDst with mktime normalized tm_isdst was not 0 for forced standard time. The checks before it pass: under TZ=EST5EDT the summer instant has tm_isdst == 1, timelocal returns summerTimer, and mktime with tm_isdst = 0 returns summerTimer + 3600. What fails is the expectation that mktime leaves tm_isdst at 0 afterwards: libc_mktime (Time.cpp:583-585) does not take the normalized fields from the host std::mktime, it rebuilds the struct with localtime_s of the resulting time, and that instant is in DST, so tm_isdst comes back as 1 on this host. timelocal itself behaves as the PR describes (tm_isdst 1 in summer, -1 and otherwise untouched fields on the unrepresentable date; those checks come after the abort, so I verified them only by reading). The tree also carried #11793, #11773, #11890 and #11846, which touch neither Time.cpp nor GuestMktime.cpp. Not run on a PS5.

AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine.

@heditrigui

Copy link
Copy Markdown
Contributor Author

@maskym Thanks for running and pointing out the Windows mktime struct rebuild behavior with localtime_s in Time.cpp! Dropped the over-specified forMktime.tm_isdst == 0 assertion, keeping the DST-vs-standard conversion return values and forTimelocal.tm_isdst == 1 verification.

@maskym

maskym commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Retested head 2dc798856c on Windows, merged into main 2ee8e9751a together with #11848's 581e0c0454 (both merged cleanly).

Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, BUILD_TESTING=ON, Ninja. Build clean.

ctest --test-dir build -R "^guest_mktime$": passed in 0.04 s. The earlier failure at the tm_isdst == 0 expectation no longer occurs with the narrowed check. Not run on a PS5.

AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine.

@B-Howell

Copy link
Copy Markdown
Contributor

Linux result (the runs so far are Windows only; the EST5EDT case goes through glibc's zone handling here, not UCRT's).

Setup: Ubuntu 22.04 under WSL2, GCC 11.4.0, glibc 2.35, Ninja Release, BUILD_TESTING=ON, AMD Ryzen 7 7800X3D. Head 2dc798856cc1540a2f16a52f8bdd982f4661b6ce merged alone onto main 951399570d4a43e714ee10ed1f9d3ee8c796e749 (clean).

Test: ctest -R '^guest_mktime$' --repeat until-fail:10: passes all 10 runs, including CheckTimelocalDst.

No control: timelocal is a new export, so restoring Time.cpp from main leaves the test unable to link rather than showing a behaviour difference.

Not tested: macOS, any title.

AI-assisted (Claude Code).

@oneandonlydean oneandonlydean self-assigned this Oct 28, 2026
@rooklz rooklz mentioned this pull request Oct 29, 2026
12 tasks done
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 29, 2026
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 29, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@heditrigui Thank you for adding the summer-time case under EST5EDT. On the Windows run for 4307d56 (job 558, guest_mktime) it stops at this line in CheckTimelocalDst:

Require(mktimeResult == summerTimer + 3600, "mktime did not respect forced standard time tm_isdst=0");

The two checks before it pass on Windows (localtime_s reports summer time, and timelocal returns the summer timer), so timelocal itself looks right. What fails is the extra comparison that mktime with tm_isdst = 0 lands one hour later: the shared mktime hands the broken-down time to the host std::mktime, and the Windows runtime does not give the glibc result for a forced-standard-time request inside a DST period. That behaviour belongs to mktime, not to this change, and the Linux run cannot see it because glibc honours tm_isdst = 0.

Please make the test independent of that host behaviour. Either drop the mktimeResult == summerTimer + 3600 requirement, or compile it only where the host honours it (#ifndef _WIN32), and keep the timelocal checks on every host: the result equals summerTimer when the input says tm_isdst = 0, the normalized tm_isdst is 1, and the failure path leaves the other fields alone. If you want to know what Windows returns there, printing mktimeResult - summerTimer in the failure message would show it on the next run.

No change to timelocal_nid_postfix or the debt entry is needed for this. If you would rather record that mktime ignores a forced tm_isdst = 0 on Windows, that is a separate libc debt line and can come in its own PR.

@heditrigui

Copy link
Copy Markdown
Contributor Author

@oneandonlydean Thanks! Guarded the host mktime forced standard time comparison under #ifndef _WIN32\ in \GuestMktime.cpp\ as suggested, keeping the test independent of Windows host \mktime\ behavior while retaining the \ imelocal\ summer normalization and unrepresentable date checks on every platform. Rebased cleanly onto latest upstream/main.

@maskym

maskym commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Retested head 9baa8782a7 on Windows, merged into main 9f748f4d8e (the head changed since my earlier runs: the separate "drop mktime normalized isdst assertion" commit is gone, leaving the two timelocal commits).

Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, BUILD_TESTING=ON, Ninja. Build clean.

The tree also carried the open PR #13455, merged cleanly. guest_mktime passes 3 of 3 runs on this head, including the summer DST comparison, where the first head I tested aborted. Not run on a PS5.

AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine.

@oneandonlydean
oneandonlydean merged commit 02ae9ee into boykopovar:main Nov 2, 2026
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants