Skip to content

fix(tests): drop the guest_freebsd_clocks check that the host zone is UTC - #12660

Closed
JJenkx wants to merge 1 commit into
boykopovar:mainfrom
JJenkx:windows/libkernel-clocks-test-host-zone
Closed

JJenkx wants to merge 1 commit into
boykopovar:mainfrom
JJenkx:windows/libkernel-clocks-test-host-zone

Conversation

@JJenkx

@JJenkx JJenkx commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

What

guest_freebsd_clocks fails on any machine whose time zone isn't UTC. This PR removes the check responsible.

KernelTimezoneIsUtc (added by #11546) runs first in main(), before the test sets TZ. It requires sceKernelGettimezone to report {0, 0}. Since #10499, that call reports the host zone's standard offset and daylight saving amount, as frankea measured on a PS5 set to US Eastern (-300, 60). So on a host in another zone, the check sees the host zone and aborts. CI and the machines #11546 was tested on run in UTC, so it passed there.

The check adds no coverage. Both of its assertions are already in TimezoneReportsTheStandardOffsetAndDaylightAmount, which sets TZ first:

  • a null pointer returns 0x8002000E;
  • UTC0 reports {0, 0}.

So this PR deletes KernelTimezoneIsUtc and its call. No library code changes.

Tested

Windows 11 set to Eastern Standard Time, WinLibs GCC 15.2.0 UCRT r7, Release, main 3a2b919816.

TZ main this PR (--repeat until-fail:3)
unset (host zone: Eastern) fails, also in a full ctest -j32 run passes 3/3
EST5EDT fails passes 3/3
UTC0 passes passes 3/3
IST-5:30 fails passes 3/3
CET-1CEST not run passes 3/3

On main, a temporary line-number print in Require locates the abort at the zone check in KernelTimezoneIsUtc.

Conventions pass. tools/pr_overlap.py: three open PRs share the file (#9658 and #12418 among them), and all merge cleanly with this branch.

Not tested: Linux and macOS. The check does the same thing there whenever the host zone isn't UTC.

Agreed landing order — 2026-10-29

Agreed: #12660 lands first, then #12418. babyhuey’s confirmation records #12418’s dependency. This PR has no dependency on #12418. Its non-UTC clock assertions need this timezone correction; #12418’s separate guest-copy fast-path prerequisite remains its author’s responsibility. This records the landing order without adding a new runtime claim.

AI-assisted: yes (Codex).

Alternative-fix overlap — 2026-10-29

The refreshed overlap review identifies #12747 (222a178f7f) as an alternative fix for the same host-timezone assertion, rather than a prerequisite: it initializes UTC inside KernelTimezoneIsUtc, while this PR removes the redundant assertion and its call. #12809 is closed as a duplicate of #12747. These should not all land.

Please choose one of #12660 and #12747 before merging; I favor the deletion here because the remaining clock/offset checks already cover the contract. I am keeping this useful reviewed PR open pending that choice. The separately agreed order with babyhuey remains #12660 first, then #12418 if this alternative is selected. No dependency on #12747 is claimed. The earlier clean merge previews do not establish that these alternatives should be combined.

AI-assisted: yes (Codex).

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 (Claude Code)

🤖 Generated with Claude Code

… UTC

KernelTimezoneIsUtc ran before the test set TZ and required
sceKernelGettimezone to report {0, 0}. Since boykopovar#10499 that call reports
the host zone, as a PS5 does, so the check failed on any machine not
set to UTC. Its null pointer and UTC assertions remain in
TimezoneReportsTheStandardOffsetAndDaylightAmount, which sets TZ.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rooklz rooklz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host-side review for the platforms the body lists as Not tested (Linux, macOS). I made no console measurements.

Harness. I extracted HostZone/HostZoneAt/HostDaylightAmount/sceKernelGettimezone verbatim from core/libs/prx/libkernel/Time/Time.cpp @ 3a2b919816 (lines 763-803 and 844-849). GuestCopyOut_nid_no_patch is shimmed as a memcpy. On top of that I ran main's KernelTimezoneIsUtc body unchanged. TZ is set from outside the process, the way the zone is set before main() reaches any SetTimeZone.

TZ macOS 26 arm64, Apple clang (host /etc/localtime = Europe/London) Ubuntu 24.04 x86_64, GCC, glibc 2.39 (container /etc/localtime = Etc/UTC)
unset {0,60} → abort {0,0} pass
UTC0 / Etc/UTC {0,0} pass {0,0} pass
Europe/London {0,60} → abort {0,60} → abort
EST5EDT / America/New_York {-300,60} → abort {-300,60} → abort
IST-5:30 / Asia/Kolkata {330,0} → abort {330,0} → abort
Africa/Accra, Atlantic/Reykjavik {0,0} pass n/a

This confirms the body's analysis on Linux and macOS too. The deleted check passes only when the host zone has offset 0 and no daylight saving at all. A UK host fails it all year round, because sceKernelGettimezone reports the zone's daylight amount (60), not whether daylight saving is in effect now. So a "GMT" machine is not safe. CI passes only because the runners and containers are Etc/UTC.

Coverage. Nothing is lost:

  • The null → 0x8002000E assertion is repeated at L309.
  • {0,0} under UTC0 is the first RequireTimezone row (L304), and that row now runs with TZ set explicitly.
  • No other check before TimezoneReportsTheStandardOffsetAndDaylightAmount reads the host zone. The gettimeofday zone check at L342 reads guestTimezone, not the host.
  • The EST5EDT/CET-1CEST rows compare the daylight amount, not the current DST state, so they don't depend on the date.

Head ab03f37893 is base 3a2b919816 minus exactly these 7 lines. LGTM.

@babyhuey

Copy link
Copy Markdown
Contributor

Windows confirmation on a host that isn't on UTC, the case this fixes: Windows 11 time zone Eastern Standard Time, currently on daylight time (UTC-04:00), with no TZ set in the environment. WinLibs GCC 15.2, Release, BUILD_TESTING=ON.

run result
guest_freebsd_clocks alone on main 3a2b9198165 aborts (0xc0000409) at 0.2 s
guest_freebsd_clocks alone, this head ab03f378936 merged onto 3a2b9198165 passes 3 of 3 (0.4-0.8 s)
CI's test selection for this head (554 tests) 548 pass, 0 fail, 6 skip

AI-assisted: yes (Claude Code)

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Linux run at this PR's exact head ab03f37893 (ab03f378936924ab4346257b11a93b60bf583ebe), not merged onto anything else; its base on main is 3a2b919816.

Arch Linux, kernel 7.2.9, Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, x86-64. Release, BUILD_TESTING=ON, Ninja -j4, private worktree.

  • ctest -R '^guest_freebsd_clocks$': passes in each of 3 runs (3 passes).

This is an actual Linux build and run of the changed test, alongside the existing host-side review.

@AdnanKhan-ak47 AdnanKhan-ak47 mentioned this pull request Oct 29, 2026
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

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 9658 core/libs/tests/GuestFreebsdClocks.cpp
PR 11042 core/libs/tests/GuestFreebsdClocks.cpp

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Cross-reference: #12418 (clock_gettime/clock_getres EFAULT) adds assertions to guest_freebsd_clocks. On a non-UTC Windows host (Eastern Standard Time), that test aborts on main edf6804cf5 with or without #12418, because of the KernelTimezoneIsUtc check this PR removes. With this head (ab03f37893) merged in as well, it passes 3 of 3 and #12418's negative control fails as expected. Details are on #12418.

AI-assisted: yes (Claude Code).

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed on current main. Full Windows suite on main edf6804cf5 (Windows 11 Enterprise 10.0.22621, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON, no Vulkan driver visible): 2 of 2980 fail, guest_job_manager_worker_count and guest_freebsd_clocks, the same two as on 3a2b919816. The host zone is Eastern Standard Time.

With this head (ab03f37893) merged onto edf6804cf5 (clean merge), guest_freebsd_clocks passes 3 of 3 (ctest --repeat until-fail:3).

AI-assisted: yes (Claude Code).

@babyhuey

Copy link
Copy Markdown
Contributor

@JJenkx #12418 (mine) adds EFAULT assertions to guest_freebsd_clocks, and on a non-UTC host it only passes with this PR's removal of KernelTimezoneIsUtc, as you showed. MarcZX10 asked us to record an order: I'd like this one to land first, and I've marked #12418 as depending on #12660. The two heads merge without conflicts either way, and #12418 is also waiting on a fast path in the guest-copy helpers, so it won't be ready before yours anyway.

AI-assisted: yes (Claude Code)

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

@babyhuey agreed: #12660 first, then #12418. Thanks for recording the dependency on #12418. This PR's own checklist needs no Depends on change, since it doesn't depend on #12418. For whoever merges: #12660 at ab03f37893 still merges cleanly and passes guest_freebsd_clocks 3 of 3 on current main on a non-UTC host (Eastern), and #12418 passes on top of it, as shown on #12418.

AI-assisted: yes (Claude Code).

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Agreed: #12660 lands first, then #12418. babyhuey’s confirmation records #12418’s dependency. This PR has no dependency on #12418. Its non-UTC clock assertions need this timezone correction; #12418’s separate guest-copy fast-path prerequisite remains its author’s responsibility. This records the landing order without adding a new runtime claim.

AI-assisted: yes (Codex).

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

The refreshed overlap review identifies #12747 (222a178f7f) as an alternative fix for the same host-timezone assertion, rather than a prerequisite: it initializes UTC inside KernelTimezoneIsUtc, while this PR removes the redundant assertion and its call. #12809 is closed as a duplicate of #12747. These should not all land.

Please choose one of #12660 and #12747 before merging; I favor the deletion here because the remaining clock/offset checks already cover the contract. I am keeping this useful reviewed PR open pending that choice. The separately agreed order with babyhuey remains #12660 first, then #12418 if this alternative is selected. No dependency on #12747 is claimed. The earlier clean merge previews do not establish that these alternatives should be combined.

AI-assisted: yes (Codex).

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

#12747 merged (19:22 UTC) and fixes the same host-timezone assumption by pinning the zone inside KernelTimezoneIsUtc. On current main 184afa49ad, guest_freebsd_clocks passes 3 of 3 here on a non-UTC host (US Eastern, Windows 11 22621), so this PR is no longer needed. It now also conflicts with main. Closing it as superseded by #12747.

AI-assisted: yes (Claude Code).

@JJenkx JJenkx closed this Oct 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants