Date: Mon, 18 Oct 1999 10:22:44 +0200 (IST) From: Eli Zaretskii X-Sender: eliz AT is To: Sergey Vlasov cc: djgpp-workers AT delorie DOT com, DJ Delorie Subject: Re: [vsu AT au DOT ru: bugs in itimer.c] In-Reply-To: <199910152324.TAA29571@envy.delorie.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Reply-To: djgpp-workers AT delorie DOT com X-Mailing-List: djgpp-workers AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk On Fri, 15 Oct 1999, DJ Delorie wrote: From: "Sergey Vlasov" To: 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.