Summary: | vg_scheduler.c:786 (idle): Assertion `delta >= 0' failed. | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | Peter Seiderer <seiderer123> |
Component: | general | Assignee: | Julian Seward <jseward> |
Status: | RESOLVED FIXED | ||
Severity: | crash | CC: | jeremy |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | Compiled Sources | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Attachments: |
Test for timeout wraparound
Suggested patch against timeout wraparound New suggested patch for coregrind/vg_scheduler.c Update: Suggested patch against timeout wraparound Updated patch against valgrind-2.2.0 |
Description
Peter Seiderer
2004-03-05 20:20:43 UTC
This trouble with this is that poll() takes a signed integer timeout, and considers anything <0 to be infinite. Is the problem that delta becomes negative, or that we should map all negative values to -1? With 'tp->time' and 'now' defined as UInt and delta as Int the above calculation 'delta = tp->time - now;' will produce false results if 'tp->time - now' exceeds INT_MAX. The suggested patch tries to avoid this by changing the definition of delta from Int to UInt (and move the assertion a little bit upwards). If delta must be kept as Int the result of 'tp->time - now' must be checked and/or limited not to exceed INT_MAX. But the real (or a further) problem seems to be the handling of huge timeouts given to pthread_cond_timedwait, which wrap around due to the limited range of the internal time representation in 1/1000 seconds: pthread_cond_timedwait with a timeout of now + 5 seconds calls add_timeout with time = 5000 pthread_cond_timedwait with timeout of now + 4294968 seconds (49 days) calls add_timeout with time = 704 See attached test2.c program: gcc -Wall test2.c time valgrind --tool=corecheck ./a.out 5 real 0m5.196s time valgrind --tool=corecheck ./a.out 4294968 real 0m0.898s Created attachment 5070 [details]
Test for timeout wraparound
Created attachment 5071 [details]
Suggested patch against timeout wraparound
Suggested patch to avoid timeout wraparound from pthread_cond_timedwait
(plus fix of wrong convertion from tv_usec to ms).
Created attachment 5086 [details]
New suggested patch for coregrind/vg_scheduler.c
Sorry, did not see the need for a signed delta because of the poll call while
doing the first suggested patch.
Here now a new, less invasive patch,limiting delta to INT_MAX and moving
the 'wake up' assert some lines down to give a overwrite
by clo_signal_polltime a chance.
Looks good. I've give it a closer look and probably apply it. Created attachment 5620 [details]
Update: Suggested patch against timeout wraparound
Fix bug in previous patch, the line
- ull_ms_now = 1000ULL * ((unsigned long long int)(ms_now));
should read
+ ull_ms_now = ((unsigned long long int)(ms_now));
Peter
Created attachment 7695 [details]
Updated patch against valgrind-2.2.0
Updated patch against valgrind-2.2.0 (both previous patches in one).
- Fix calculation of delta in vg_scheduler.c (the assertion).
- Fix timestamp calculation of for pthread_cond_timedwait in
vg_libpthread.c (short 'real' timeouts for long given timeouts).
This patch has now been committed, minus the piece that moved the assertion as I don't believe that to be correct or necessary. We should have something to wake up for regardless of whether we are having to route signals or not. Hi Tom Hughes, I was looking at your patch for the head branch for this bug. Committed by Tom Hughes on 2004/10/17, 15:18:22 in branch HEAD http://cvs-digest.org/index.php?diff&file=valgrind/coregrind/vg_scheduler.c,v&version=1.191 And there is a problem with valgrind/coregrind/vg_libpthread.c fix at "Line 1375 int __pthread_mutex_timedlock(pthread_mu". You need to set ull_ms_now and ull_ms_end before the if statement. Like you did at Line 1548. So you just need to add the following. ull_ms_now = ((unsigned long long int)(ms_now)); ull_ms_end = ull_ms_now + (ull_ms_end_after_1970 - ull_ms_now_after_1970); Thanks for the fix. It works great for me. David Those lines were put in the next day when the overnight builds failed. |