[OpenSmalltalk/opensmalltalk-vm] windows: resync the VM time when the system clock gets changed (#588)
As of today, if the Windows system time is changed, already running VM instances will not reflect this change. With this patch, this bug is fixed. The full bug report is on squeak-dev: http://lists.squeakfoundation.org/pipermail/squeak-dev/2021-June/215908.html
This bug only affects builds for Windows versions prior to Windows 8, never builds do not use the tick variables any longer. See:
https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/57260c215c025e17b57e0...
(However, as #499 is still unmerged, currently, also builds for Windows 10 are affected by the bug. Looks like we were never benefitting from @eliotmiranda's improvements for the Win8 APIs (cf. fae0d31cd52341977d37d150d8d2c9272f9864bf) until today 😆)
Independently of this patch, we will want to fix cross-platform-occurring VM hangs when the system clock is pulled backward. See the bug report on squeak-dev for this, again.
Please review thoroughly!
/cc @marceltaeumel You can view, comment on, or merge this pull request online at:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/588
-- Commit Summary --
* windows: resync the VM time when the system clock gets changed
-- File Changes --
M platforms/win32/vm/sqWin32Heartbeat.c (7) M platforms/win32/vm/sqWin32Window.c (4)
-- Patch Links --
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/588.patch https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/588.diff
@LinqLover pushed 1 commit.
26dda2dec16db8746fda140b214d54c06c29459b only resync system time for relevant windows versions
(By the way, the best thing while you are dealing with timing issues is that after you have committed all your stuff, you set your system time back to normal and realize that you have spent one hour less on this issue than you were assuming ... ^^)
FYI: We wanted to bump the minimal supported version to Windows Vista. Eliot suggested even Windows 8 but we still have a lot of Windows 7 users out there. See PR #499
Why don't we create multiple builds for the different Windows versions?
Hi Christoph, hi all,
On Aug 31, 2021, at 1:53 AM, Christoph Thiede ***@***.***> wrote:
 Why don't we create multiple builds for the different Windows versions?
That would probably cause disruption to naming schemes etc. an easier way would be to change the code so that - if compiled on a system older than Windows 8 the old ticket variable code is used - if compiled on Windows 8 or later then - at startup the Windows is version is cached in a variable and, - depending on the variable either the new or the old code is evaluated
So we keep a single build, and a single version of the code base. Normal builds remain backward compatible. One can still compile on an older system and get a functional, albeit more fragile vm.
Note that one could also use the get symbol from dll api (LoadSymbol? can’t remember) to so that a vm built on an older version can use the new api when run on later versions but IMO this is wasted effort and unnecessary complexity
Finally, one could consider implementing my adaptive approach that ensures a monotonic clock. But the Windows 8 api makes things so simple it’s (arguably) a lot of work for no gain.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.
@LinqLover commented on this pull request.
@@ -625,6 +625,10 @@ MainWndProcW(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
case WM_KILLFOCUS: fHasFocus = 0; return DefWindowProcW(hwnd,message,wParam,lParam); + + case WM_TIMECHANGE: + resyncSystemTime(); +
Do we need to call `DefWinowProcW` here or break?
@j4yk commented on this pull request.
@@ -625,6 +625,10 @@ MainWndProcW(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
case WM_KILLFOCUS: fHasFocus = 0; return DefWindowProcW(hwnd,message,wParam,lParam); + + case WM_TIMECHANGE: + resyncSystemTime(); +
For what it is worth, wine has no default reaction to WM_TIMECHANGE https://github.com/wine-mirror/wine/blob/master/dlls/user32/defwnd.c
One might speculate whether Windows will in the future at some point have a meaningful default reaction to that message (which would speak for DefWindowProc). I doubt it will (which speaks for break).
But anyway I would not keep the fall-through because otherwise the next person to add a message here might overlook it and introduce unexpected behavior whenever WM_TIMECHANGE comes in.
@LinqLover pushed 1 commit.
cd8c50d7839e117f9a12c0d8c3e277f700eae624 Resolve fallthrough
Thank you for the feedback, Jakob, and for the explanation, Eliot. Is there anything else that keeps us from getting this one merged? :-)
Merged #588 into Cog.
vm-dev@lists.squeakfoundation.org
-
Christoph Thiede -
Eliot Miranda
-
Jakob Reschke
-
Marcel Taeumel