GNOME Bugzilla – Bug 623003
Major problems with calls to gst_util_uint64_scale()
Last modified: 2010-06-28 10:27:01 UTC
The change from 0.10.29.1 to 0.10.29.2 broken many calls to gst_util_uint64_scale() for some reason. The function now gets invalid values passed, e.g. in the videorate unit test. In that test the fps_n/fps_d of the caps are used for scaling the frame number. The caps are parsed correctly, the values are stored correctly in the instance struct but when passed to the scaling function at least the last two parameters are wrong: they are 0! This only happens on 32 bit x86, not x86_64
And this only happens in base 0.10.29.2. base 0.10.29.1 with core 0.10.29.2 works fine for example.
For the videorate test it's enough to rebuild videotestsrc without -DG_DISABLE_CAST_CHECKS to get around the assertion. Any ideas? :)
Or building with disabled cast checks and -O0... FYI the cast check macro only changes this in gtype.h: #ifndef G_DISABLE_CAST_CHECKS # define _G_TYPE_CIC(ip, gt, ct) \ ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) # define _G_TYPE_CCC(cp, gt, ct) \ ((ct*) g_type_check_class_cast ((GTypeClass*) cp, gt)) #else /* G_DISABLE_CAST_CHECKS */ # define _G_TYPE_CIC(ip, gt, ct) ((ct*) ip) # define _G_TYPE_CCC(cp, gt, ct) ((ct*) cp) #endif /* G_DISABLE_CAST_CHECKS */ Compiler error?
--- a/gst/videotestsrc/gstvideotestsrc.c +++ b/gst/videotestsrc/gstvideotestsrc.c @@ -730,6 +730,9 @@ gst_video_test_src_do_seek (GstBaseSrc * bsrc, GstSegment * src->n_frames = 0; } if (src->rate_numerator) { + g_print ("%p %p\n", src, bsrc); + g_print ("%d %d\n", src->rate_numerator, src->rate_denominator); + g_print ("%llu\n", gst_util_uint64_scale (100, 2, 10)); src->running_time = gst_util_uint64_scale (src->n_frames, src->rate_denominator * GST_SECOND, src->rate_numerator); } else { This gives correct output for all g_prints and makes everything work. Removing all g_prints breaks it again, keeping one of them (no matter which) fixes it again. Adding src->running_time = gst_util_uint64_scale (100, 2, 10); instead of the g_prints makes it work too.
From looking at the generated asm the difference here is, that without the workaround gcc reuses the parameters from the stack. Which assumes of course that the stack content is not modified but IIRC gst_util_uint64_scale does this unless it's using int128 instructions.
Not marking gst_util_uint64_scale() as G_GNUC_CONST fixes this. The problem is, that these functions are not really const when using the C int128 implementation. Reason is, that they call functions that are not const because they have pointer arguments and change/examine these pointer arguments. The docs explicitly say this: Expands to the GNU C const function attribute if the compiler is gcc. Declaring a function as const enables better optimization of calls to the function. A const function doesn't examine any values except its parameters, and has no effects except its return value. See the GNU C documentation for details. Note A function that has pointer arguments and examines the data pointed to must not be declared const. Likewise, a function that calls a non-const function usually must not be const. It doesn't make sense for a const function to return void. Not sure if we can mark the function as G_GNUC_PURE, probably not because the functions have other effects than the return values. Patch follows
Created attachment 164789 [details] [review] utils: Don't use G_GNUC_CONST for the uint64 scaling functions They are actually *not* const functions because on architectures without int128 instructions the parameters were changed. gcc re-used the parameters on the stack for multiple calls though and the changed parameters were used for the second call then. Fixes bug #623003.
commit c076eb6bb0037b759d81d8b10b0094c70b8712e0 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Jun 28 10:20:39 2010 +0200 utils: Don't use G_GNUC_CONST for the uint64 scaling functions They are actually *not* const functions because on architectures without int128 instructions the parameters were changed. gcc re-used the parameters on the stack for multiple calls though and the changed parameters were used for the second call then. Fixes bug #623003.