Bug#888220: ignition-math4: FTBFS on mips(el) and hppa: UNIT_Helpers_TEST fails badly

[James Cowgill]

Control: tags -1 patch

Hi,

On 23/01/18 23:48, Aaron M. Ucko wrote:
> Source: ignition-math4
> Version: 4.0.0+dfsg1-1
> Severity: important
> Tags: upstream
> Justification: fails to build from source
> User: debian-mips at lists.debian.org
> Usertags: mips mipsel
> 
> Builds of ignition-math4 for mips, mipsel, and the non-release
> architecture hppa have been failing as detailed at
> 
> https://buildd.debian.org/status/fetch.php?pkg=ignition-math4&arch=mips&ver=4.0.0%2Bdfsg1-1&stamp=1516376024&raw=0
> https://buildd.debian.org/status/fetch.php?pkg=ignition-math4&arch=mipsel&ver=4.0.0%2Bdfsg1-1&stamp=1516373688&raw=0
> https://buildd.debian.org/status/fetch.php?pkg=ignition-math4&arch=hppa&ver=4.0.0%2Bdfsg1-1&stamp=1516373442&raw=0
> 
> On mips, this test manages to make ctest itself to run out of memory:
> 
>         Start 11: UNIT_Helpers_TEST
>   terminate called after throwing an instance of 'std::bad_alloc'
>     what():  std::bad_alloc
>   Makefile:143: recipe for target 'test' failed
>   make[2]: *** [test] Aborted
> 
> On mipsel and hppa, HelpersTest.Pair spews millions(!) of errors and
> then hits a timeout:
> 
>   11/75 Test #11: UNIT_Helpers_TEST ......................***Timeout 240.12 sec
>   [...]
>   [ RUN      ] HelpersTest.Pair
>   /<<BUILDDIR>>/ignition-math4-4.0.0+dfsg1/src/Helpers_TEST.cc:516: Failure
>         Expected: a
>         Which is: 4294962295
>   To be equal to: c
>         Which is: 4294962296

This is caused by the pairing function in src/Helpers.cc

Firstly, I am not sure I understand why anyone would need this pairing
function exactly. The point of a pairing function is that the inputs can
be infinitely big, but the inputs to these functions are 32-bit
integers. As long as you don't need the specific algorithm, you could
implement something similar using a single shift and an add.

I think the issue is this code relies on infinite floating point
precision in calculating the "sqrt" variable:
>     std::tuple<PairInput, PairInput> Unpair(const PairOutput _key)
>     {
>       // Must explicitly cast so that the _key is not auto cast to a double
>       uint64_t sqrt = static_cast<uint64_t>(
>           std::floor(std::sqrt(static_cast<long double>(_key))));
>       uint64_t sq = sqrt * sqrt;
> 
>       return ((_key - sq) >= sqrt) ?
>         std::make_tuple(static_cast<PairInput>(sqrt),
>                         static_cast<PairInput>(_key - sq - sqrt)) :
>         std::make_tuple(static_cast<PairInput>(_key - sq),
>                         static_cast<PairInput>(sqrt));
>     }

On architectures with > 64-bit long doubles (like Linux x86_64) you get
away with this. On other architectures where long double == double, the
cast to long double and sqrt is too inaccurate to implement this properly.

The attached patch is based on a more accurate integer square root I
found on stack overflow. You should be able to drop your
0004_test_failures_in_non_x64_arches.patch after applying this and
possibly some other #ifdefs I noticed that upstream have added.

James
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pair-accurate.patch
Type: text/x-patch
Size: 696 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/debian-science-maintainers/attachments/20180124/1dd5fbac/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/debian-science-maintainers/attachments/20180124/1dd5fbac/attachment-0001.sig>