Mail Archives: djgpp-workers/1999/10/15/19:51:27
------- Start of forwarded message -------
From: "Sergey Vlasov" <vsu AT au DOT ru>
To: <dj AT delorie DOT com>
Subject: bugs in itimer.c
Date: Fri, 15 Oct 1999 17:19:58 +0300
Content-Type: text/plain;
charset="Windows-1251"
X-Priority: 3
X-MSMail-Priority: Normal
X-MimeOLE: Produced By Microsoft MimeOLE V4.72.3110.3
I have found several bugs in src/libc/posix/signal/itimer.c.
I am using DJGPP 2.02 and have installed patches #000271, #000272, #000274
for itimer.c and #000295 for uclock.c (from the bug reporting Web page).
This is the test program:
====== Cut ======
#include <stdio.h>
#include <sys/time.h>
#include <time.h>
int main(void)
{
struct itimerval init_val = { { 0, 0 }, { 5, 0 } };
struct itimerval val;
(void) uclock(); /* Avoid waiting inside setitimer() in Win95 */
setitimer (ITIMER_REAL, &init_val, NULL);
getitimer (ITIMER_REAL, &val);
printf("%ld.%06ld\n", (long)val.it_value.tv_sec,
(long)val.it_value.tv_usec);
return 0;
}
====== Cut ======
Compile: gcc -g -Wall -o ittest.exe ittest.c
This program should print a value close to 5.000000 (actually, less than 5.0
by the time spent in getitimer() and setitimer()). However, when I tried,
it printed something about 4.001453, which is obviously wrong.
The bug is in itimer.c: getitimer() incorrectly calculates tv_usec field
when converting from uclock_t to struct timeval. It should do something like
this (uclock_t uclk ==> struct timeval *value):
int quot = uclk / UCLOCKS_PER_SEC;
int rem = uclk % UCLOCKS_PER_SEC;
value->tv_sec = quot;
value->tv_usec = (unsigned long)rem * 3433 / 4096;
I have fixed this conversion in itimer.c.
There is another problem: If the SIGTIMR processing is delayed for some
time, the GetNextEvent() function might return a negative value (which means
it's too late). In this case, __djgpp_timer_countdown will be set to a
very large value, effectively disabling the timer. I have changed two checks
for zero to `next > 0' to prevent this. I think delaying SIGALRM or SIGPROF
in this case is better than completely stopping the timer.
The time checks in timer_action() were also incorrect. Signals were raised
only when it was at least 64k uclocks (one timer interrupt) late. I checked
it with the following test program:
====== Cut ======
#include <stdio.h>
#include <sys/time.h>
#include <time.h>
#include <signal.h>
volatile int sigtimr = 0;
void
_my_handler(int sig)
{
sigtimr = 1;
}
int main(void)
{
/* 49.5 timer ticks. The delay should be 50 ticks (2.746275 seconds) */
struct itimerval init_val = { { 0, 0 }, { 2, 718812 } };
uclock_t u_start, u_end;
(void) uclock();
signal(SIGALRM, _my_handler);
setitimer (ITIMER_REAL, &init_val, NULL);
do { /* nothing */ } while ( !sigtimr );
u_start = uclock();
setitimer (ITIMER_REAL, &init_val, NULL);
sigtimr = 0;
do { /* nothing */ } while ( !sigtimr );
u_end = uclock();
printf("Time = %f\n", (double)(u_end - u_start) / UCLOCKS_PER_SEC);
return 0;
}
====== Cut ======
Compile: gcc -g -O2 -Wall -o ittest2.exe ittest2.c
This program should print the time really spent waiting for SIGALRM.
First wait loop is for synchronizing with timer interrupts. Without the
previous fix (`next > 0'), this program just hangs, waiting forever for
the second SIGALRM. With the fix it at least completes, but the time
reported is consistently larger than it should be (2.856 instead of 2.746).
After removing `+ 65535L' from timer_action(), the delay is correct.
The patch #000272 (which fixed race conditions in getitimer()) has
introduced another bug: if the value of `which' was invalid, getitimer()
returned with interrupts disabled! I have corrected this also.
There was also some race conditions still remaining after the patch
#000272. First, getitimer() was using the static variable u_now; I have
changed it to use a local variable instead. In fact, I completely removed
this static variable; it was asking for trouble.
Second, operations with long longs in setitimer() could be interrupted;
I have protected them by disabling interrupts temporarily.
I have also removed the superfluous `u_now = uclock();' in getitimer().
It was needed only when getitimer() was called from setitimer().
After making all these changes, my first test program prints about 4.999976,
which seems to be correct. The test program from djtst202.zip prints
either 24 or 25 (the same as with the original version). Of course,
functions became somewhat slower (mostly due to disabling interrupts),
but correct (I hope so :-)
Here is my new itimer.c. Because there were so many patches for it,
I'm sending the complete file. (`diff -u' against 2.02 is larger than
the file itself :-)
====== Cut: beginning of itimer.c ======
/* Copyright (C) 1995 Charles Sandmann (sandmann AT clio DOT rice DOT edu)
setitimer implmentation - used for profiling and alarm
BUGS: ONLY ONE AT A TIME, first pass code
This software may be freely distributed, no warranty.
Changed to work with SIGALRM & SIGPROF by Tom Demmer.
Gotchas:
- It relies on uclock(), which does not work as expected under Windows 9x.
- It screws up some debuggers (see DJGPP FAQ list 2.11, question 12.10).
- Both is true for the old version, too.
*/
#include <libc/stubs.h>
#include <sys/time.h>
#include <errno.h>
#include <dpmi.h>
#include <signal.h>
#include <go32.h>
#include <dos.h>
static uclock_t r_exp, r_rel, /* When REAL expires & reload value */
p_exp, p_rel; /* When PROF expires & reload value */
static inline void uclock_to_timeval(uclock_t uclk, struct timeval *value)
{
int quot, rem;
/* The following asm does:
* quot = uclk / UCLOCKS_PER_SEC;
* rem = uclk % UCLOCKS_PER_SEC;
* with only one instruction!
*/
asm ("idivl %3": "=a" (quot), "=d" (rem): "A" (uclk), "r" (UCLOCKS_PER_SEC));
value->tv_sec = quot;
value->tv_usec = (unsigned long)rem * 3433 / 4096;
}
int
getitimer(int which, struct itimerval *value)
{
uclock_t exp, rel;
int save_istate;
/* Potential race condition here with timer_action.
These critical sections must run with interrupts disabled */
if (which == ITIMER_REAL)
{
save_istate = disable();
if (r_exp)
{
exp = r_exp - uclock();
rel = r_rel;
}
else
exp = rel = 0;
if (save_istate)
enable();
}
else if (which == ITIMER_PROF)
{
save_istate = disable();
if (p_exp)
{
exp = p_exp - uclock();
rel = p_rel;
}
else
exp = rel = 0;
if (save_istate)
enable();
}
else
{
errno = EINVAL;
return -1;
}
uclock_to_timeval(exp, &value->it_value);
uclock_to_timeval(rel, &value->it_interval);
return 0;
}
extern unsigned __djgpp_timer_countdown;
extern __dpmi_paddr __djgpp_old_timer;
extern int __djgpp_timer_hdlr;
static char timer_on = 0;
/* Set back IRQ0 handler to default values and disable own signal
handler */
static void
stop_timer(void)
{
if(!timer_on)
return;
__djgpp_timer_countdown = -1;
__dpmi_set_protected_mode_interrupt_vector(8, &__djgpp_old_timer);
timer_on = 0;
signal(SIGTIMR, SIG_DFL);
}
/* Returns the time to the next event in UCLOCK_PER_SEC u_now must be
set by calling routine. Return 0 if no event pending. */
static inline uclock_t
GetNextEvent(uclock_t u_now)
{
if (r_exp && p_exp)
return (r_exp < p_exp ? r_exp - u_now : p_exp - u_now );
else if (r_exp)
return r_exp - u_now;
else if (p_exp)
return p_exp - u_now;
else
return 0;
}
/* Handler for SIGTIMR */
static void
timer_action(int signum)
{
int do_tmr=0,do_prof=0;
uclock_t u_now, next;
u_now = uclock();
/* Check the real timer */
if (r_exp && (r_exp <= u_now) )
{
do_tmr = 1;
if (r_rel)
r_exp += r_rel;
else
r_exp = 0;
}
/* Check profile timer */
if (p_exp && (p_exp <= u_now))
{
do_prof = 1;
if (p_rel)
p_exp += p_rel;
else
p_exp = 0;
}
/* Now we have to schedule the next interrupt, if any pending */
if ((next = GetNextEvent(u_now)) != 0)
{
next /= 65536L;
__djgpp_timer_countdown = (next > 0) ? next : 1 ;
}
else
stop_timer();
if (do_tmr)
raise(SIGALRM);
if (do_prof)
raise(SIGPROF);
}
static void
start_timer(uclock_t u_now)
{
uclock_t next;
__dpmi_paddr int8;
next = GetNextEvent(u_now);
next /= 65536L;
__djgpp_timer_countdown = (next > 0) ? next : 1;
if (timer_on)
return;
timer_on = 1;
signal(SIGTIMR, timer_action);
__dpmi_get_protected_mode_interrupt_vector(8, &__djgpp_old_timer);
int8.selector = _my_cs();
int8.offset32 = (unsigned) &__djgpp_timer_hdlr;
__dpmi_set_protected_mode_interrupt_vector(8, &int8);
}
int
setitimer(int which, struct itimerval *value, struct itimerval *ovalue)
{
uclock_t *t_exp, *t_rel;
uclock_t exp, rel;
uclock_t u_now;
int save_istate;
if ( ( which != ITIMER_REAL ) && ( which != ITIMER_PROF ) )
{
errno = EINVAL;
return -1;
}
u_now = uclock();
if (ovalue)
{
if (getitimer(which,ovalue))
return -1; /* errno already set */
}
t_exp = which == ITIMER_REAL ? &r_exp: &p_exp;
t_rel = which == ITIMER_REAL ? &r_rel: &p_rel;
/* Potential race condition here with timer_action.
These critical sections must run with interrupts disabled */
if ((value->it_value.tv_sec|value->it_value.tv_usec)==0 )
{
save_istate = disable();
/* Disable this timer */
*t_exp = *t_rel = 0;
/* If both stopped, stop timer */
if (( p_exp | r_exp ) == 0 )
{
stop_timer();
}
if (save_istate)
enable();
return 0;
}
/* Rounding errors ?? First multiply and then divide could give
Overflow. */
exp = value-> it_value.tv_sec * UCLOCKS_PER_SEC
+ ((uclock_t)value->it_value.tv_usec * 4096) / 3433;
rel = value-> it_interval.tv_sec * UCLOCKS_PER_SEC
+ ((uclock_t)value->it_interval.tv_usec * 4096) / 3433;
if (exp)
exp += u_now;
save_istate = disable();
*t_exp = exp;
*t_rel = rel;
start_timer(u_now);
if (save_istate)
enable();
return 0;
}
====== Cut: end of itimer.c ======
- --- Sergey Vlasov <vsu AT au DOT ru>
------- End of forwarded message -------
- Raw text -