GNOME Bugzilla – Bug 348114
[PPC64?] [gcc-4.1.2?] GST_BUFFER_IN_CAPS flag should have been copied !
Last modified: 2008-07-10 15:10:55 UTC
The lib/gdp test fails in test_buffer on PPC64 (p5 buildbot). As of today, it is deactivated on that architecture. Do not close this bug unless: _ the #ifndef is removed _ AND the bug is fixed
I thought this was a compiler bug?
I thought it correct to put an entry in bugzilla for that suppression so we can eventually figure out what's going wrong once and for all. If there's no solution to this issue... I guess we'll be able to close the bug.
The same error comes up in a gst-plugins-bad test. Running suite(s): gdpdepay 66%: Checks: 3, Failures: 1, Errors: 0 elements/gdpdepay.c:349:F:general:test_streamheader: Assertion 'GST_BUFFER_FLAG_IS_SET (outbuffer, GST_BUFFER_FLAG_IN_CAPS)' failed FAIL: elements/gdpdepay I commented it so it doesn't get compiled/run on PPC64 and added in the sourcecode this bug number.
This bug seems to return in form of a failed unit test (didn't look too closely, just looks like the same issue): Running suite(s): streamheader 50%: Checks: 2, Failures: 1, Errors: 0 pipelines/streamheader.c:198:F:general:test_multifdsink_gdp_vorbisenc:0: 'n_in_caps' (0) is not equal to '3' (3)
> pipelines/streamheader.c:198:F:general:test_multifdsink_gdp_vorbisenc:0: Disabled this test as well on PPC64 until this issue is resolved on the build bot.
*** Bug 465280 has been marked as a duplicate of this bug. ***
The elements/gdpdepay failure also happens on ppc32 and s390 with gcc 4.2.3.
Hm, no: Running suite(s): data protocol 80%: Checks: 5, Failures: 1, Errors: 0 libs/gdp.c:150:F:general:tf:0: GST_BUFFER_IN_CAPS flag should have been copied ! FAIL: libs/gdp Not only gdpdepay but also libs/gdp :)
...which is noted in the PPC/s390 part of bug #500833
Ok, took a closer look at it on powerpc (32 bit) again today. First of all we do: GST_WRITE_UINT16_BE (h + 42, GST_BUFFER_FLAGS (buffer) & flags_mask); GST_BUFFER_FLAGS (buffer) is a gint (i.e. 32 bit), so we write the wrong part of it to h on big endian architectures. Putting the result in a guint16 before doesn't help much though. But when compiling with -O0 or putting some debug printf around that line of code "fixes" it... so it really seems to be a compiler bug. This is with gcc 4.3.1 btw...
2008-06-30 Sebastian Dröge <sebastian.droege@collabora.co.uk> * libs/gst/dataprotocol/dataprotocol.c: Don't write to the same region of memory as a uint64 and uint16 as this breaks strict aliasing rules and apparantly breaks on PPC and s390. Thanks to Sjoerd Simons for analysing. Fixes bug #348114.
Just a note: this is a workaround for what really sounds to me like a compiler bug. I'm pretty sure we're not violating strict-aliasing rules (the underlying type is an unsigned char pointer; type-aliasing on those is explicitly permitted). The patch also removes the only 'documentation' I could see for what those 16 bytes actually were, in the protocol.
The GDP packet is documented in the gst-libs/dataprotocol documentation. There this 16 bytes are documented too (first 2 are buffer flags, other's are ABI padding).
I'm wondering: doesn't this imply that we either need to remove the GST_HAVE_UNALIGNED_ACCESS bits in gstutils.h (ie. GST_READ_* GST_WRITE_*) or make sure that both GStreamer and any app/plugin using it is compiled with -fno-strict-aliasing at all times (not sure how one would do that though)? Just adding a warning to the docs doesn't really cut it imho, since the whole point of these macros is that the developer doesn't need to worry/know about things like alignment etc.
Could someone with access to a failing machine attach the disassembled code? I'm leaning toward compiler bug as well.
(In reply to comment #15) > Could someone with access to a failing machine attach the disassembled code? > I'm leaning toward compiler bug as well. One of the Debian gcc maintainers disagrees: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=488563 I'll provide disassembled code of Sjoerd's testcase tomorrow
Never mind. I misread the code. It is an aliasing problem. The patch is correct. I think a warning in the documentation is a good idea.
> I think a warning in the documentation is a good idea. Do you think that's sufficient though? If this issue isn't immediately obvious to a number of core GStreamer hackers, do we really expect users of our API to fully grasp the implications? And even if they do, what are they supposed to do? Hack their own unaligned versions of the GST_READ_* macros? I think it'd be better to change the GST_READ_*/GST_WRITE_* macros to the 'slow but safe' versions and add _FAST variants or something which hackers can use if they know that it's safe to do so.
IMHO the GST_READ/WRITE macros should use the slow and safe version always and everywhere.