GNOME Bugzilla – Bug 678181
Compilation error when using GST_TIME_TO_TIMEVAL()...
Last modified: 2012-06-19 12:08:09 UTC
When trying to compile code looking like this: GTimeVal tv; GST_TIME_TO_TIMEVAL (GST_CLOCK_TIME_NONE, tv); or like this: GTimeVal tv; GST_TIME_TO_TIMEVAL (4294967296000000000ULL, tv); I will, on a 32-bit machine, get an error message: error: overflow in implicit constant conversion [-Werror=overflow] Note that (surprisingly) no error is given for this code: GTimeVal tv; GstClockTime tmp = 4294967296000000000ULL; GST_TIME_TO_TIMEVAL (tmp, tv); Neither piece of code will give any kind of compile-time or run-time error on a 64-bit machine. After reading the code and discussing this on #gstreamer it seems as if the consensus is that GTimeVal can not represent undefined clock times or times larger than 2^32 - 1 seconds. This means that the first snippet of code above is just plain wrong, however the two other snippets of code are correct in the sense that they represent valid (but large) values of GstClockTime. This prompted me to created the attached patch which both truncates the resulting timeval values to that of its members (which means that no compile-time errors are generated for 32-bit machines) and adds a g_assert () explaining what the problem is (on 32 and 64-bit machines alike): assertion failed: ("Value of time " "GST_CLOCK_TIME_NONE" " is out of timeval's range" && ((GstClockTime) (((GstClockTime) -1))) < (((GstClockTime) (G_MAXUINT32 - 1U)) * GST_SECOND)) assertion failed: ("Value of time " "4294967296000000000ULL" " is out of timeval's range" && ((GstClockTime) (4294967296000000000ULL)) < (((GstClockTime) (G_MAXUINT32 - 1U)) * GST_SECOND)) assertion failed: ("Value of time " "tmp" " is out of timeval's range" && ((GstClockTime) (tmp)) < (((GstClockTime) (G_MAXUINT32 - 1U)) * GST_SECOND)) GST_TIME_TO_TIMEVAL() is only used once in gstreamer, gst-plugins-* and gst-* -- the public function gst_poll_wait(). In this function GST_CLOCK_TIME_NONE is handled specially but other out of range values are passed directly go GST_TIME_TO_TIMEVAL(). To make sure I haven't broken anything I have run make check on git HEAD of gstreamer, gst-plugins-* and gst-* and there are as many failing tests before as there are after my patch. ;)
Created attachment 216536 [details] [review] Proposed patch for removing compilation error and enabling assertion. I added a quite verbose assert, and if that is not desired I'm happy to provide a patch that just asserts on the expression itself. Just let me know.
I don't see how a run-time assert is better than a compiler error (you can do a compile-time assert btw, there's a macro for that too). Where is GTimeVal needed anyway? Perhaps we should just purge it from our API entirely, including those macros? I *think* it's only used in connection with g_cond_timed_wait in various _TIMED_WAIT() macros? g_cond_timed_wait() is deprecated in GLib anyway now, so we should probably not use it in our API.
A run-time assert that catches _all_ cases (both out of range constants as in snippet #1 and variable values as in snippet #2) is better than a compile time error that catches most cases (out of range constants). If I could come up with a way to catch out of range variable values at compile-time I would certainly advocate that, but I can't. I will need to get back next week (when I can study the code) and motivate why GTimeVal is used at all.
+ g_assert ("Value of time " #t " is out of timeval's range" && \ + ((GstClockTime) (t)) < \ + (((GstClockTime) (G_MAXUINT32 - 1U)) * GST_SECOND)); \ It should be G_MAXLONG - 1U, right?
-> Wim: No, I believe that G_MAXUINT32 is the correct value. The reason being that if the assert would fail on a 32-bit machine you want it to fail on a 64-bit machine too so that bugs on 32-bit machines will not go unnoticed for people only compiling for 64-bit machines. G_MAXLONG would have different values depending on if it is compiled for 32- or 64-bit machines in case I'm haven't misunderstood things. -> Tim: My employer uses it in a unit test for a proprietary (so far at least) element. So yeah, I could absolutely see why this patch would only make marginal sense to you. I could only see that this macro is used in the implementation of gstpoll(), and there you might just as well use the struct timeval directly. So what I'm trying to say is that if you decide to keep G_TIME_TO_TIMEVAL() around it ought to complain (on all architectures) when it received bogus arguments, or to remove it completely which is also fine although it requires a little bit of work for us (let me know if you want me to try to submit a removal patch).
(In reply to comment #5) > -> Wim: No, I believe that G_MAXUINT32 is the correct value. The reason being > that if the assert would fail on a 32-bit machine you want it to fail on a > 64-bit machine too so that bugs on 32-bit machines will not go unnoticed for > people only compiling for 64-bit machines. G_MAXLONG would have different > values depending on if it is compiled for 32- or 64-bit machines in case I'm > haven't misunderstood things. Except that things work correctly on 64 bits when using G_MAXLONG. Artificially restricting a feature on 64bits would be like saying that you can't allocate more than 4GB of memory on 64bits because it would fail on 32 bits...
(In reply to comment #6) > Except that things work correctly on 64 bits when using G_MAXLONG. Artificially > restricting a feature on 64bits would be like saying that you can't allocate > more than 4GB of memory on 64bits because it would fail on 32 bits... That is also a valid point of course. So do you want me to resubmit the patch with G_MAXUINT32 replaced by G_MAXLONG? I might also change the comment above the macro that refers to 2^32 - 1 so it also mentions what the restriction is on a 64-bit machine. Or do you want a removal patch?
I simplified it some more, please test. commit 68bfaa45f9d7d8b20cef0899f367de06269cd37e Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Tue Jun 19 14:05:21 2012 +0200 clock: assert about timestamp overflows Assert when converting to timeval and timespec about overflows. This can happen on platforms with 32bits long. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=678181 commit 74cf8103922377af465cc33c803e55ed42ea45bf Author: Sebastian Rasmussen <sebrn@axis.com> Date: Mon Jun 18 15:28:20 2012 +0200 clock: fix compiler warning Cast to the right value, it might indeed overflow but we want the compiler to ignore that.