bug#11825: 24.1.50; float-time no longer accepts negative time values

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Eli Zaretskii]

This bug report will be sent to the Bug-GNU-Emacs mailing list
and the GNU bug tracker at debbugs.gnu.org.  Please check that
the From: line contains a valid email address.  After a delay of up
to one day, you should receive an acknowledgement at that address.

Please write in English if possible, as the Emacs maintainers
usually do not have translators for other languages.

Please describe exactly what actions triggered the bug, and
the precise symptoms of the bug.  If you can, give a recipe
starting from `emacs -Q':

This just caused me 5 hours of debugging: as result of the latest
changes related to nanosecond time resolution, float-time now barfs
when passed negative time values.  I bumped into this when
display-time signaled an error when updating the time on the
mode line.  Here's the Lisp backtrace:

  Debugger entered--Lisp error: (error "Invalid time specification")
    float-time((-1 65476 50000 0))
    timer-until([t 20463 1752 0 0 60 display-time-event-handler nil nil] (20463 1692 50000 0))
    timer-event-handler([t 20463 1752 0 0 60 display-time-event-handler nil nil])

As you see, timer-until was called with its second argument time older
than the next time the timer should trigger.  timer-until does this:

  (float-time (time-subtract time (timer--time timer)))

But when time-subtract returns a negative time value, float-time now
signals an error, where previously it would compute a negative value
in seconds.

I modified timer-until to do this instead:

  (- (float-time time) (float-time (timer--time timer)))

This change is part of revision 108814.  However, I'm filing this bug
report in case we need to be able to pass negative time values to
float-time.

(Once again the MS-DOS port finds subtle bugs in Emacs.)

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
For information about debugging Emacs, please read the file
d:/gnu/bzr/emacs/trunk/etc/DEBUG.


In GNU Emacs 24.1.50.1 (i386-mingw-nt5.1.2600)
 of 2012-06-30 on HOME-C4E4A596F7
Bzr revision: 108813 cyd@gnu.org-20120630142124-gwl5hsvaacmw1t2x
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (3.4) --no-opt --enable-checking --cflags
 -Id:/usr/include/libxml2 -DGLYPH_DEBUG=1 -DBYTE_CODE_METER'

Important settings:
  value of $EMACSDATA: D:/gnu/bzr/emacs/trunk/etc
  value of $EMACSDOC: D:/gnu/bzr/emacs/trunk/etc
  value of $EMACSLOADPATH: D:/gnu/bzr/emacs/trunk/lisp;D:/gnu/bzr/emacs/trunk/leim
  value of $EMACSPATH: D:/gnu/bzr/emacs/trunk/bin
  value of $LANG: ENU
  locale-coding-system: cp1255
  default enable-multibyte-characters: t

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
M-x r e p o r t - e m a c s - b u <tab> <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils time-date tooltip ediff-hook vc-hooks
lisp-float-type mwheel dos-w32 disp-table ls-lisp w32-win w32-vars
tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment
lisp-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer button faces cus-face files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote make-network-process multi-tty emacs)

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Glenn Morris]

Eli Zaretskii wrote:

> However, I'm filing this bug report in case we need to be able to pass
> negative time values to float-time.

FWIW, the commentary of time-date.el says that times can be negative
(and has done for several years,
http://lists.gnu.org/archive/html/emacs-devel/2005-03/msg00681.html).

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Paul Eggert]

On my host (Fedora 15 x86-64) float-time works just fine
with negative values.  Possibly the problem is that
time_t is unsigned on your host, and that float-time converts
its argument through time_t, which fails.  As far as I
can see Emacs has behaved this way for some time,
but it doesn't hurt to generalize float-time so that
it does not fail due to out-of-range integers, so I
installed that into the trunk as bzr 108928.  Please
give it a try on your host to see whether it fixes
the problem.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Eli Zaretskii]

> Date: Fri, 06 Jul 2012 19:11:41 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 11825@debbugs.gnu.org
> 
> On my host (Fedora 15 x86-64) float-time works just fine
> with negative values.  Possibly the problem is that
> time_t is unsigned on your host

Yes, the type used by DJGPP for time_t is unsigned.

> and that float-time converts
> its argument through time_t, which fails.  As far as I
> can see Emacs has behaved this way for some time,
> but it doesn't hurt to generalize float-time so that
> it does not fail due to out-of-range integers, so I
> installed that into the trunk as bzr 108928.  Please
> give it a try on your host to see whether it fixes
> the problem.

Will do, thanks.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Paul Eggert]

On 07/06/2012 11:57 PM, Eli Zaretskii wrote:
> Yes, the type used by DJGPP for time_t is unsigned.

OK, thanks, but in that case I don't see how
the use of EMACS_TIME_SIGN in src/msdos.c can
be correct.  On hosts where time_t is unsigned,
EMACS_TIME_SIGN can never be negative.  That's why
I got rid of all uses of EMACS_TIME_SIGN elsewhere,
replacing subtract-and-then-compare-to-zero with
plain compare.  If time_t can be unsigned in msdos.c,
then msdos.c sys_select can be fixed by using
plain compare rather than subtract-and-then-compare-to-zero.
This will allow us to simplify Emacs further by removing
EMACS_TIME_SIGN entirely.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Eli Zaretskii]

> Date: Sat, 07 Jul 2012 13:30:04 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 11825@debbugs.gnu.org
> 
> On 07/06/2012 11:57 PM, Eli Zaretskii wrote:
> > Yes, the type used by DJGPP for time_t is unsigned.
> 
> OK, thanks, but in that case I don't see how
> the use of EMACS_TIME_SIGN in src/msdos.c can
> be correct.  On hosts where time_t is unsigned,
> EMACS_TIME_SIGN can never be negative.

I'm not sure I understand: are you saying that a time difference can
never be negative on such platforms?  What about the case tv_sec = 0
in both time values and tv_nsec difference is negative?

In any case, this is a different problem.  There's nothing wrong with
sys_select on msdos.c, it seems to work just fine.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Paul Eggert]

On 07/07/2012 07:57 PM, Eli Zaretskii wrote:
> are you saying that a time difference can
> never be negative on such platforms?

Yes, that's right.

> What about the case tv_sec = 0
> in both time values and tv_nsec difference is negative?

In that case, timespec_sub returns the
minimum possible time value, namely zero,
since time_t is unsigned.

> There's nothing wrong with sys_select on msdos.c

Yes, the code works now, I'm just trying to simplify
Emacs by removing the need for EMACS_TIME_SIGN so that
we can remove EMACS_TIME_SIGN.  It's no big deal.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Andreas Schwab]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 07/07/2012 07:57 PM, Eli Zaretskii wrote:
>> are you saying that a time difference can
>> never be negative on such platforms?
>
> Yes, that's right.

That doesn't make sense.  A difference between two timestamps is always
signed.

>> What about the case tv_sec = 0
>> in both time values and tv_nsec difference is negative?
>
> In that case, timespec_sub returns the
> minimum possible time value, namely zero,
> since time_t is unsigned.

Which just means that timespec_sub is completely broken.  It must not
use struct timespec for its return value.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Paul Eggert]

On 07/08/2012 02:00 AM, Andreas Schwab wrote:
> A difference between two timestamps is always signed.

No, not if A is an absolute time, and B is a (nonnegative) offset.
The result is an absolute time, and is unsigned if A was unsigned.
For example:

         EMACS_GET_TIME (t);
         EMACS_SUB_TIME (old, t, EMACS_TIME_FROM_DOUBLE (delay));

This is a common way that EMACS_SUB_TIME is used in Emacs.

On 07/08/2012 08:32 AM, Eli Zaretskii wrote:> E.g., to express something like
>
>   T1 = T2 + T3 - T4
>
> instead of a simple
>
>   t1 = timespec_add (t2, timespec_sub (t3, t4));

That doesn't suffice even if struct timespec uses a signed
time_t -- if overflow occurred in the timespec_sub, the
result might be incorrect.

There is no simple solution here that does not involve
complicating the API.  Luckily, Emacs never needs to compute
expressions like the above, so the problem doesn't need to
be addressed right now.

> this should be prominently explained in systime.h.

OK, sure, I pushed this:

--- src/systime.h       2012-07-07 01:57:42 +0000
+++ src/systime.h       2012-07-08 22:57:42 +0000
@@ -80,7 +80,8 @@
 #define EMACS_GET_TIME(time)               gettime (&(time))

 /* Put into DEST the result of adding SRC1 to SRC2, or of subtracting
-   SRC2 from SRC1.  On overflow, store an extremal value.  */
+   SRC2 from SRC1.  On overflow, store an extremal value: ergo, if
+   time_t is unsigned, return 0 if the true answer would be negative.  */
 #define EMACS_ADD_TIME(dest, src1, src2) ((dest) = timespec_add (src1, src2))
 #define EMACS_SUB_TIME(dest, src1, src2) ((dest) = timespec_sub (src1, src2))

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Juanma Barranquero]

On Mon, Jul 9, 2012 at 1:03 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 07/08/2012 02:00 AM, Andreas Schwab wrote:
>> A difference between two timestamps is always signed.
>
> No, not if A is an absolute time, and B is a (nonnegative) offset.

An offset is not a timestamp, I'd say.

    Juanma

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Paul Eggert]

On 07/08/2012 04:37 PM, Juanma Barranquero wrote:
> An offset is not a timestamp, I'd say.

True, but that's how Emacs uses this interface:
it conflates the notions of timestamps and timestamp
differences, and uses the same EMACS_TIME type for both.

This could be fixed by switching to a more
complicated API that distinguishes between the two
notions, and if Emacs's timestamp computations get more
complicated that's probably a good way to go.
It's not needed for Emacs's current usage, though.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Juanma Barranquero]

On Mon, Jul 9, 2012 at 2:21 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> True, but that's how Emacs uses this interface:
> it conflates the notions of timestamps and timestamp
> differences, and uses the same EMACS_TIME type for both.

I undestand that. I'm just pointing out that Andreas is right: a
difference between two timestamps is a signed value.

    Juanma

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Eli Zaretskii]

> Date: Sun, 08 Jul 2012 16:03:39 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>, 11825@debbugs.gnu.org
> 
> On 07/08/2012 02:00 AM, Andreas Schwab wrote:
> > A difference between two timestamps is always signed.
> 
> No, not if A is an absolute time, and B is a (nonnegative) offset.
> The result is an absolute time, and is unsigned if A was unsigned.
> For example:
> 
>          EMACS_GET_TIME (t);
>          EMACS_SUB_TIME (old, t, EMACS_TIME_FROM_DOUBLE (delay));
> 
> This is a common way that EMACS_SUB_TIME is used in Emacs.

Again, if this facility is limited to this paradigm, I think we should
clearly document that.  The name EMACS_SUB_TIME doesn't tell anything
about that, it certainly sounds like providing a general-purpose
arithmetics on time values.

Anyway, Emacs does subtract absolute time values, at least in Lisp;
see time-subtract.  The current issue started when the result of
time-subtract was passed to float-time, so indirectly such time
differences can sneak into C from Lisp and be handed to these
functions.  And since time-subtract is called by timers, it is very
easy to get into a situation where EMACS_SUB_TIME etc. are used with
second argument not an offset, but an absolute time.

> > this should be prominently explained in systime.h.
> 
> OK, sure, I pushed this:

Thanks.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Paul Eggert]

On 07/08/2012 07:56 PM, Eli Zaretskii wrote:
> since time-subtract is called by timers, it is very
> easy to get into a situation where EMACS_SUB_TIME etc. are used with
> second argument not an offset, but an absolute time.

Yes, one must be careful in C code.  Currently, Emacs always
makes sure that EMACS_TIME_LE (A, B) before invoking
EMACS_SUB_TIME (..., B, A), with the exception of the
special case in msdos.c that I was suggesting to change
to be like the rest of Emacs for ease of maintenance.
But again, it's no big deal -- the code works fine as-is,
including the msdos.c special-case.

For Lisp code the bugs are different.  Lisp code cannot
invoke EMACS_SUB_TIME directly, and all execution paths
that go through EMACS_SUB_TIME are checked as described
above.  The time-subtract function is indeed buggy with
some (unlikely) arguments, but these bugs are entirely due
to Emacs fixnum overflow and are independent of EMACS_SUB_TIME
and of the C representation of time_t.

bug#11825: 24.1.50; float-time no longer accepts negative time values

[Eli Zaretskii]

> Date: Sun, 08 Jul 2012 00:26:47 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 11825@debbugs.gnu.org
> 
> On 07/07/2012 07:57 PM, Eli Zaretskii wrote:
> > are you saying that a time difference can
> > never be negative on such platforms?
> 
> Yes, that's right.
> 
> > What about the case tv_sec = 0
> > in both time values and tv_nsec difference is negative?
> 
> In that case, timespec_sub returns the
> minimum possible time value, namely zero,
> since time_t is unsigned.

??? Really???  Then using these facilities safely and portably will be
extremely inconvenient.  E.g., to express something like

  T1 = T2 + T3 - T4

instead of a simple

  t1 = timespec_add (t2, timespec_sub (t3, t4));

one would need something like

  if (timespec_cmp (t3, t4) < 0)
    {
      struct timespec tem1 = timespec_sub (t4, t3);

      if (timespec_cmp (t2, tem1) < 0)
         /* error -- no way to express negative time */
      else
         t1 = timespec_sub (t2, tem1);
    }
  else
    t1 = timespec_add (t2, timespec_sub (t3, t4);

Am I missing something?

If I'm right, and if there's no way to get better facilities, then at
the very least this should be prominently explained in systime.h.
Right now, this comment, for example:

  /* Put into DEST the result of adding SRC1 to SRC2, or of subtracting
     SRC2 from SRC1.  On overflow, store an extremal value.  */

doesn't come anywhere close to a hint that "an extremal value" could
mean zero when SRC1 < SRC2.  It's very easy to write buggy code
without a clear understanding of this fact, and never find those bugs
in testing, if the system happens to use a signed type for time_t.