Bug #32630 for Audio-FLAC-Header: Test failure on several architectures (64-bit problems)

Thu Jan 24 14:46:51 2008 ntyni [...] iki.fi - Ticket created

Subject:

Test failure on several architectures (64-bit problems)

Hi, as seen in Debian bug #462249 <http://bugs.debian.org/462249>, we are seeing test failures on several 32-bit architectures with Perl 5.8.8 and libFLAC 1.2.1. I have been able to reproduce this on hppa, and the test failure is due to differing cuesheet information. I get Show quoted text

> REM FLAC__lead-in 1604068 > REM FLAC__lead-out 170 153200460 > > where it's supposed to be > > REM FLAC__lead-in 88200 > REM FLAC__lead-out 170 153200460

The problem seems to be the newSVpvf() calls around Header.xs:250 - they are using the "UVuf" format, which expands at least here to "lu" (/usr/lib/perl/5.8/CORE/Config.h), while passing in a FLAC__int64 value. This means that the newSVpvf() stdarg function is expecting an "unsigned long" (32 bits) and getting an "unsigned long long" (64 bits). I don't claim to understand exactly what happens here; I suppose the behaviour is undefined, and apparently the caller and the callee are disagreeing about the register where the value should be stored. The wrong value (1604068 above) varies a bit, and seems to be whatever happens to be in r25 just before the function call. One fix is to explicitly cast the value to "unsigned long" in the newSVpvf() calls, but I guess this would lose information on >4G (32 bits) FLAC files. A better solution might be to do the decimal conversion on the C side with sprintf() and the "%llu" format before calling newSV*(). Note that this presumably applies to all the three places in Header.xs which use the "UVuf" format; we're seeing test failures with both the lead-in and the lead-out values on mips. Please let me know if I can help you in any way in fixing this. Thanks for your work on Audio-FLAC-Header, -- Niko Tyni (Debian Perl Group) ntyni@debian.org

Tue Feb 12 14:39:14 2008 ntyni [...] iki.fi - Correspondence added

From:

ntyni [...] iki.fi

On Thu Jan 24 14:46:51 2008, ntyni@iki.fi wrote: Show quoted text

> as seen in Debian bug #462249 <http://bugs.debian.org/462249>, we are > seeing test failures on several 32-bit architectures with Perl 5.8.8 and > libFLAC 1.2.1.

I applied the attached patch to the Debian package, and the tests now succeed on all architectures we support. Please consider including the patch. Again, thanks for your work on Audio-FLAC-Header. Cheers, -- Niko Tyni ntyni@debian.org

fix FTBFS due to 64-bit integer proglems on several 32-bit architectures. -- Niko Tyni <ntyni@debian.org> Mon, 11 Feb 2008 20:44:30 +0200 --- libaudio-flac-header-perl.orig/Header.xs +++ libaudio-flac-header-perl/Header.xs @@ -30,6 +30,9 @@ #include <FLAC/all.h> +/* for PRIu64 */ +#include <inttypes.h> + #define FLACHEADERFLAG "fLaC" #define ID3HEADERFLAG "ID3" @@ -195,6 +198,19 @@ { AV *cueArray = newAV(); + /* + * buffer for decimal representations of uint64_t values + * + * newSVpvf() and sv_catpvf() can't handle 64-bit values + * in some cases, so we need to do the conversion "manually" + * with sprintf() and the PRIu64 format macro for portability + * + * see http://bugs.debian.org/462249 + * + * maximum string length: ceil(log10(2**64)) == 20 (+trailing \0) + */ + char decimal[21]; + /* A lot of this comes from flac/src/share/grabbag/cuesheet.c */ const FLAC__StreamMetadata_CueSheet *cs; unsigned track_num, index_num; @@ -239,7 +255,8 @@ sv_catpvf(indexSV, "%02u:%02u:%02u\n", m, s, f); } else { - sv_catpvf(indexSV, "%"UVuf"\n", track->offset + index->offset); + sprintf(decimal, "%"PRIu64, track->offset + index->offset); + sv_catpvf(indexSV, "%s\n", decimal); } @@ -247,9 +264,11 @@ } } - av_push(cueArray, newSVpvf("REM FLAC__lead-in %"UVuf"\n", cs->lead_in)); - av_push(cueArray, newSVpvf("REM FLAC__lead-out %u %"UVuf"\n", - (unsigned)cs->tracks[track_num].number, cs->tracks[track_num].offset) + sprintf(decimal, "%"PRIu64, cs->lead_in); + av_push(cueArray, newSVpvf("REM FLAC__lead-in %s\n", decimal)); + sprintf(decimal, "%"PRIu64, cs->tracks[track_num].offset); + av_push(cueArray, newSVpvf("REM FLAC__lead-out %u %s\n", + (unsigned)cs->tracks[track_num].number, decimal) ); my_hv_store(self, "cuesheet", newRV_noinc((SV*) cueArray));

Sat Feb 23 16:46:01 2008 daniel [...] cpan.org - Correspondence added

Thanks - fixed in 2.0

Sat Feb 23 16:46:02 2008 The RT System itself - Status changed from 'new' to 'open'

Sat Feb 23 16:46:03 2008 daniel [...] cpan.org - Status changed from 'open' to 'resolved'