Mail Archives: djgpp-workers/1999/10/18/05:00:08
On Fri, 15 Oct 1999, DJ Delorie wrote:
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
> I have found several bugs in src/libc/posix/signal/itimer.c.
Thanks a lot for working on this and for contributing your changes.
Unfortunately, I bumped into difficulties incorporating your changes
into the DJGPP library sources, for these reasons:
- The current development sources are significantly different from
the version that you used as the baseline, even after applying the
patches you found on the bug-tracker.
- With the current development sources, I cannot reproduce the
problems you report.
- Last, but not least, I didn't always understand what problems,
exactly, are you trying to solve with some of your changes.
Needless to say, lack of a reproducible bug prevented me to find
out what these problems were.
So, reluctantly, I'm forced to ask you to go ``back to the drawing
board'', this time starting with the latest development sources of
both itimer.c and uclock.c, and see what else, if anything, needs to
be changed there, and why. For each problem you identify, please send
a test program to demonstrate the problem, the results you get, and
the OS on which you ran the test program to get those results.
Below please find some comments regarding your suggestions; I hope
they will help you review the current code. (I'm sending you the
latest sources in a separate message.)
> 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).
Please note that the bug-tracker is open to the public; anyone can
submit a patch there. Therefore, what you find on the bug-tracker are
not official patches in any sense, even if they are put there by DJGPP
maintainers. These patches are just quick fixes for the problems
reported by users. Sometimes, further investigation of the problem
reveals additional aspects that need to be fixed differently. Some
problems don't get reported by users, and so the patches that fix them
cannot be found on the bug-tracker. Some patches might even be
incorrect.
The bottom line is that you should always ask for the latest
development sources, or get them by anonymous CVS access (see
http://www.delorie.com/djgpp/cvs.html), before you start digging into
the problems. This will help you avoid solving problems that are
already solved.
> 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.
With the current development sources, this program prints 4.999983 on
MS-DOS, and 4.100002 on Windows 95. Your version of itimer.c together
with the current development sources of uclock.c gives identical
results, i.e. 4.999983 on DOS and 4.100002 on Windows 95.
I don't know why the result on Windows is smaller; one possibility is
that the DPMI functions to get and set the interrupt handler, called
by setitimer, are much slower on Windows (but I didn't try to verify
this hypothesis). In any case, your version of itimer doesn't change
anything in this case.
> 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;
The latest development sources already do the computations like this.
> 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.
This is also fixed in the development sources. However, please note
that your change for this particular problem is not exactly right.
You suggest:
__djgpp_timer_countdown = (next > 0) ? next : 1;
The current development version does this:
__djgpp_timer_countdown = next > 0 ? next - 1 : 0;
This is because the timer interrupt handler (see exceptn.S) checks
whether the countdown variable is zero *before* it decrements it. So
setting it to zero means the timer will expire on the next tick, which
is exactly what we want in this case. In contrast, your version will
wait for one more timer tick before it raises SIGALRM, which isn't
right if the timer has already expired a long time ago.
> 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:
The problem you refer to is already fixed in the development sources,
and with that version your test program produces correct results both
on MS-DOS and Windows 95.
> 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.
Patch #000272 is not part of the current development sources, so the
problem with disabling interrupts doesn't exist there in the first
place.
> Second, operations with long longs in setitimer() could be interrupted;
> I have protected them by disabling interrupts temporarily.
Could you please give a concrete example of how interrupts can cause
any real trouble inside setitimer and getitimer, where you added the
calls to disable() and enable()? I don't like disabling interrupts in
time-critical library functions, because under some environments, the
CLI and STI instructions are trapped by the DPMI server or the
underlying OS, and emulated with code that takes eons to execute. So
I would prefer to look for alternative solutions for whatever problems
you see in the current development sources.
Again, thanks for working on this.
- Raw text -