[Libreoffice-commits] core.git: sc/qa

[Stephan Bergmann]

On 08/12/2015 05:53 PM, Eike Rathke wrote:
> On Wednesday, 2015-08-12 17:48:57 +0200, Eike Rathke wrote:
>>>> The new test code triggers a division by zero now (as seen with
>>>> -fsanitize=undefined):
>>>
>>> First, that shouldn't matter as it produces a double INF value.
>>
>> I wonder though why that didn't propagate into the += operation and
>> whether that behavior is really undefined.. the test should had failed
>> already.
>
> Seeing that it already tested for an error things should be correct.
>
> So, should fsanitize maybe be taught about double infinity? ;)

...or, developers be taught about the C++ Standard? ;)  It is quite 
clear on this:  "If the second operand of / or % is zero the behavior is 
undefined." ([expr.mul]/4)

Now, ISO/IEC 60559 may mandate the result of division by zero to be 
infinity under certain circumstances (if the dividend is finite and 
non-zero) an NaN otherwise, when operating in its default exception 
handling mode, and C++ implementations may follow that, and we may even 
require that at least certain parts of the LO code base must be executed 
in conformance to ISO/IEC 60559 in its default exception handling mode.

However, in other parts of the LO code base it might be unexpected that 
floating-point division by zero happens, and its occurrence might 
indicate a bug (cf. "git log --grep -fsanitize=float-divide-by-zero"), 
so I'm reluctant to disable -fsanitize=float-divide-by-zero wholesale.

For those places where we apparently require floating-point division to 
adhere to ISO/IEC 60559 in its default exception handling mode, would it 
be possible to "dress those up" (like in the below ad-hoc patch), or 
would that be unreasonable?


> diff --git a/sc/source/core/tool/interpr3.cxx b/sc/source/core/tool/interpr3.cxx
> index ce3dc91..a0e02b1 100644
> --- a/sc/source/core/tool/interpr3.cxx
> +++ b/sc/source/core/tool/interpr3.cxx
> @@ -30,11 +30,12 @@
>  #include "scmatrix.hxx"
>  #include "globstr.hrc"
>
> -#include <math.h>
> +#include <cmath>
>  #include <vector>
>  #include <algorithm>
>  #include <boost/math/special_functions/log1p.hpp>
>  #include <comphelper/random.hxx>
> +#include <config_options.h>
>
>  using ::std::vector;
>  using namespace formula;
> @@ -2821,6 +2822,23 @@ void ScInterpreter::ScFTest()
>      PushDouble(2.0*GetFDist(fF, fF1, fF2));
>  }
>
> +namespace {
> +
> +double divide(double a, double b) {
> +#if !ENABLE_RUNTIME_OPTIMIZATIONS
> +    if (b == 0) {
> +        if (std::isfinite(a) && a != 0) {
> +            return std::copysign(INFINITY, a);
> +        } else {
> +            return NAN;
> +        }
> +    }
> +#endif
> +    return a / b;
> +}
> +
> +}
> +
>  void ScInterpreter::ScChiTest()
>  {
>      if ( !MustHaveParamCount( GetByte(), 2 ) )
> @@ -2850,7 +2868,7 @@ void ScInterpreter::ScChiTest()
>              {
>                  double fValX = pMat1->GetDouble(i,j);
>                  double fValE = pMat2->GetDouble(i,j);
> -                fChi += (fValX - fValE) * (fValX - fValE) / fValE;
> +                fChi += divide((fValX - fValE) * (fValX - fValE), fValE);
>              }
>              else
>              {