After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 678181 - Compilation error when using GST_TIME_TO_TIMEVAL()...
Compilation error when using GST_TIME_TO_TIMEVAL()...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.11.x
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-06-15 18:14 UTC by Sebastian Rasmussen
Modified: 2012-06-19 12:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch for removing compilation error and enabling assertion. (1.57 KB, patch)
2012-06-15 18:16 UTC, Sebastian Rasmussen
none Details | Review

Description Sebastian Rasmussen 2012-06-15 18:14:59 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. ;)
Comment 1 Sebastian Rasmussen 2012-06-15 18:16:44 UTC
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.
Comment 2 Tim-Philipp Müller 2012-06-15 18:55:28 UTC
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.
Comment 3 Sebastian Rasmussen 2012-06-15 22:24:40 UTC
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.
Comment 4 Wim Taymans 2012-06-18 13:15:22 UTC
+  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?
Comment 5 Sebastian Rasmussen 2012-06-18 14:26:33 UTC
-> 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).
Comment 6 Wim Taymans 2012-06-18 14:32:00 UTC
(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...
Comment 7 Sebastian Rasmussen 2012-06-18 15:16:43 UTC
(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?
Comment 8 Wim Taymans 2012-06-19 12:08:09 UTC
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.