Tcl Source Code

View Ticket
Login
Ticket UUID: 3328635
Title: Virtual Time broken on threaded unix
Type: Bug Version: None
Submitter: ferrieux Created on: 2011-06-24 20:37:32
Subsystem: 03. Timer Events Assigned To: aku
Priority: 9 Immediate Severity: Minor
Status: Open Last Modified: 2018-03-08 23:35:39
Resolution: None Closed By: nobody
    Closed on:
Description:
In threaded unix builds, timer events use pthread_cond_timedwait, which needs an absolute target time. The computation of this target is broken, because it adds the (correct) scaled==real delta to the (incorrect) virtual Tcl_GetTime. The fix is to use real time for the latter.

Moreover, in some unices, the clock used for pthread_cond_timedwait can be selected among several, with pthread_condattr_setclock at condvar creation time. One of them, CLOCK_MONOTONIC, has the extra advantage of being insensitive to system clock tweaks. I think that selecting it when possible can only improve things, since it allows:

     - usual, tip-302-sensitive [after] semantics unchanged, since the timer events are also stored as absolute epoch-time  targets by Tcl
     - fully working Virtual Time with clock_gettime(CLOCK_MONOTONIC) giving tip-302-insensitive [after].

I have a provisional patch for platforms with clock_gettime+CLOCK_MONOTONIC, designed to enable Joe Keen's code in http://groups.google.com/group/comp.lang.tcl/browse_frm/thread/8f820c68143850f9# . Some autogoo would be due before committing.

An alternative to 'select monotonic if possible, epoch if not' would be a runtime setting, like a ::tcl::unsupported::monotonicTime linked var. To be discussed.
User Comments: chw added on 2018-03-08 23:35:39:
Another attempt to bring tip 302 somewhat forward. The another attached patch
adds handling for the new "-at" attribute for timed after commands. The semantic
is not like "-robust" as laid out in the tip, but instead based on the clock
command, i.e. specifies a point in this (wall clock) time domain to make that
fact explicit to the user. A few test cases are included as well as an update
to the after.n man page.

The patch is against core-8-6-branch and tested on Debian 9 amd64. It runs
all tests without errors. Win32/64 is not verified.

chw added on 2017-04-13 00:23:12:
From the Tcl'ers Chat for the record (citing myself, sorry):

Regarding TIP#302: I don't remember any man page version of after(n) and Tcl_CreateTimerHandler(3) due to its relative millisecond-ness mentioning absolute time in the wall clock sense for the last 20 years, which makes CLOCK_MONOTONIC a legit time source. After all, any assumptions, that time computations using [clock seconds] et.al. applied to after(n) are correct, are IMO thus cargo-cultivated programming errors. Then there is interp(n) with its IMO wrongly designed time limit based on absolute time. But that certainly came long after after's invention. And why has it to be based on absolute time? And then there is virtualized time? And why was it? And where is it test cased in the core?

chw added on 2017-02-10 21:15:37:
Sooorry, but couldn't resist.

tcl is not dead yet ... https://www.youtube.com/watch?v=oZ2QiyxRiYs&feature=youtu.be&a -- jima, tclers chat, 2017

Time is heavy sometimes; imagine how heavy eternity must be. -- E.M.Cioran, The Book of Delusions, 1936

jan.nijtmans added on 2016-09-26 09:40:47:
Latest patch from Christian committed to branch "tkt3328635-posix-monotonic-clock". Applied to trunk in stead of core-8-6-branch, most likely this change is too intrusive for a patch release.

chw added on 2016-09-25 18:59:38:
Latest version which is smaller and works hopefully on olden Windows < Vista, and is integrated into AndroWish now, can be found in attachment atip302.diff, please review.

chw added on 2016-09-24 17:52:53:
Not sure if this is the right ticket, but my patch to make things more
insensitive against wall clock time changes is somewhat improved now,
see attachment.

dgp added on 2015-05-01 14:04:48:
status?

aku added on 2014-10-09 19:58:08:
Thanks. So this means we do need configure.in/tcl.m4 changes to know of the need for linking against librt, and when.

Have to test on other platforms as well.

ferrieux added on 2014-10-09 19:35:37:
Will test and review asap; in the meantime, regarding the clock_gettime() missing symbol on glibc, my wheezy manpage says:

       Link with -lrt (only for glibc versions before 2.17).

aku added on 2014-10-09 18:52:43:

The updated patch is now committed to branch "tkt3328635-posix-monotonic-clock".

Pushed revision [265e84eb5e]. I am asking for a general review by Joe and Alexandre.

This code does not work yet! (on linux even).

It compiles on my build system (RedHat FC4 Stentz), yet fails to link. It claims that clock_gettime() is missing (*).

Checking with temporarily added #error lines showed that the code believes that both _POSIX_TIMERS and _POSIX_MONOTONIC_CLOCK are defined. From what I read in the discussion here and referenced web site and manpage that should mean that clock_gettime() is available. And it is ipmlied to be in the (g)libc.

The glibc here is 2.3(.6). Should some other library be linked too ?

Or could be a macro and I need a header which contains the proper mapping to the actual function name ?

Further note that I tried to extend the patch to fall back to CLOCK_REALTIME where possible. Please review this part too.

(Ad *): .../build/tcl-8.5/libtcl8.5.so: undefined reference to `clock_gettime'


aku added on 2014-10-09 18:24:11:
The patch was not updated with the discussion about how to configure this.

In the works now.

Working on the Tcl 8.5 branch.

Commit will be on a branch at first, for review (by Joe, who will become a contact here).

aku added on 2014-10-09 17:33:46:
A -nocase grep of the Tcl 8.6 sources does not show any use of 'monotonic' except tclCompile.h where it talks about source offsets put into the byte code.

Ditto for Tcl 8.5.

It seems that the patch is not applied.
Bumping prio for self.

dgp added on 2014-10-09 14:33:28:
status?

dgp added on 2014-10-09 14:33:10:
status?

ferrieux added on 2011-07-24 21:38:18:
Thanks for the POSIX macros: simpler indeed !
For the "#if defined(CLOCK_MONOTONIC)", are we sure that when it's here it's always a macro, not a direct enum ?
Last, is there a typo in your last sentence ? Shouldn't it read "I don't see any reason *not* to preemptively offer this as an option" ?

jenglish added on 2011-07-23 01:51:58:
Also - as a falllback, if _POSIX_MONOTONIC_CLOCK / CLOCK_MONOTONIC is not available, a suitable fallback is CLOCK_REALTIME; this is guaranteed present if _POSIX_TIMERS are available at all, will be no more broken than gettimeofday() (which is what's currently used), and  less broken than the damaged TIP#233 version.

Re:  option to enable/disable this from Tcl scripts or C code -- ISTM this would be an "Old buggy behaviour [Y/N]?" type question, I don't see any reason to preemptively offer this as an option.

jenglish added on 2011-07-23 01:35:28:
You don't need autogoo to detect this, POSIX specifies preprocessor macros you can test instead.  

See <URL: http://pubs.opengroup.org/onlinepubs/7908799/xsh/unistd.h.html >, or `man 7 posixoptions ` on a glibc system, for details.  But basically: #if defined(_POSIX_TIMERS) && defined(_POSIX_MONOTONIC_CLOCK).  Come to think of it, you could just use #if defined(CLOCK_MONOTONIC) ...

ferrieux added on 2011-07-23 00:59:02:
Pasting Roy Keene's kind crash-course on autoconf:

[19:49]<rkeene>AC_DEFUN(TCL_CHECK_CLOCK_BLAH, [ AC_COMPILE_IFELSE(AC_LANG_PROGRAM([[contents-outside-of-main]], [[contents-of-main]]), [ AC_DEFINE(HAVE_CLOCK_GETTIME, [1], [Define to 1 if you have whatever clockgettime is]) ]) ])

Then, as Roy suggests, do the runtime part (is CLOCK_MONOTONIC working here ?) at Tcl init time.

ferrieux added on 2011-06-25 03:50:44:
Actually, if we choose the explicit selection approach, the toggle should rather be accessible at C level, by the extensions able to benefit from it: Tcl_SetClockKind( 0==normal, 1==monotonic). This way, an extension can enable the tweak-unsensitive [after] in a completely self-contained way. If you like this method I'll write the patch shortly.

ferrieux added on 2011-06-25 03:41:03:

File Added - 415876: tim.patch

Attachments: