Re: [Libreoffice-commits] core.git: sc/qa
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
{
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice-commits] core.git: sc/qa
Hi, On Thursday, 2015-08-13 11:36:11 +0200, Stephan Bergmann wrote: ...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) Lame excuses of a standards committee not being able to agree on something or force anyone to change implementation ;-) 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. Calc gracefully handles infinity and NaN in the interpreter (well, as long as the compiler/hardware for such operations actually generates either and doesn't decide to return 0 instead because it's undefined ;) and in intermediate or final results sets an error if detected. We don't strive to handle +-infinity and NaN in further calculations, but we detect them as error condition. 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. Agreed. 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? We already have ScInterpreter::div() that uses sc::div() from sc/inc/math.hxx creating a NaN with a div/0 error value bit pattern for x/0, I'll merge the 0/0 part of your patch to flag the different condition, thanks for the detail. Using that in the place in question though will make it necessary to adapt the unit test, which maybe shouldn't rely on the exact error value anyway. Maybe we should even just bail out early if /0 occurs. I'll take a look. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GPG key ID 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A Better use 64-bit 0x6A6CD5B765632D3A here is why: https://evil32.com/ Care about Free Software, support the FSFE https://fsfe.org/support/?erack pgpGPCPIGnqiQ.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice-commits] core.git: sc/qa
Hi Stephan, On Wednesday, 2015-08-12 14:42:18 +0200, Stephan Bergmann 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. Second, we might want to either use the div() function there to get the #DIV/0 error instead of a general floating point error, or explicitly check fValE for 0 and push errDivisionByZero and bail out if so. Thanks for heads-up Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GPG key ID 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A Better use 64-bit 0x6A6CD5B765632D3A here is why: https://evil32.com/ Care about Free Software, support the FSFE https://fsfe.org/support/?erack pgpmgiZOk4ngy.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice-commits] core.git: sc/qa
Hi, On Wednesday, 2015-08-12 17:36:09 +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. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GPG key ID 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A Better use 64-bit 0x6A6CD5B765632D3A here is why: https://evil32.com/ Care about Free Software, support the FSFE https://fsfe.org/support/?erack pgp7w05wNyDy2.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice-commits] core.git: sc/qa
Hi again, 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? ;) Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GPG key ID 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A Better use 64-bit 0x6A6CD5B765632D3A here is why: https://evil32.com/ Care about Free Software, support the FSFE https://fsfe.org/support/?erack pgpPmyqPsdfro.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice-commits] core.git: sc/qa
On 08/12/2015 12:22 PM, Łukasz Hryniuk wrote: commit 0a726cb29936b61b8f05b0863e24db212a0e6166 Author: Łukasz Hryniuk lukasz.hryn...@wp.pl Date: Tue Aug 11 23:18:28 2015 +0200 tdf#89387 test for CHITEST function Change-Id: Ifff9367e56c357f3d5026ecbf7e984368428e074 Reviewed-on: https://gerrit.libreoffice.org/17658 Tested-by: Jenkins c...@libreoffice.org Reviewed-by: Eike Rathke er...@redhat.com Tested-by: Eike Rathke er...@redhat.com The new test code triggers a division by zero now (as seen with -fsanitize=undefined): sc/source/core/tool/interpr3.cxx:2853:59: runtime error: division by zero #0 0x2b123f28fb19 in ScInterpreter::ScChiTest() sc/source/core/tool/interpr3.cxx:2853:59 #1 0x2b123f3488f9 in ScInterpreter::Interpret() sc/source/core/tool/interpr4.cxx:4243:43 #2 0x2b123df7857b in ScFormulaCell::InterpretTail(ScFormulaCell::ScInterpretTailParameter) sc/source/core/data/formulacell.cxx:1746:9 #3 0x2b123df692aa in ScFormulaCell::Interpret() sc/source/core/data/formulacell.cxx:1479:13 #4 0x2b123df4cc0f in ScFormulaCell::MaybeInterpret() sc/source/core/data/formulacell.cxx:2368:9 #5 0x2b123cb5547e in ScColumn::GetString(int, rtl::OUString) const sc/source/core/data/column3.cxx:2459:9 #6 0x2b123e4e3224 in ScTable::GetString(short, int, rtl::OUString) const sc/source/core/data/table2.cxx:1457:9 #7 0x2b123d3cb450 in ScDocument::GetString(ScAddress const) const sc/source/core/data/document.cxx:3379:5 #8 0x2b123b84edc9 in Test::testFuncCHITEST() sc/qa/unit/ucalc_formula.cxx:5668:12 #9 0x2b123b37486d in CppUnit::TestCallerTest::runTest() workdir/UnpackedTarball/cppunit/include/cppunit/TestCaller.h:166:6 #10 0x2b11f2f9753d in CppUnit::TestCaseMethodFunctor::operator()() const workdir/UnpackedTarball/cppunit/src/cppunit/TestCase.cpp:32:5 #11 0x2b120b24b346 in (anonymous namespace)::Protector::protect(CppUnit::Functor const, CppUnit::ProtectorContext const) test/source/vclbootstrapprotector.cxx:57:14 #12 0x2b11f2f535d7 in CppUnit::ProtectorChain::ProtectFunctor::operator()() const workdir/UnpackedTarball/cppunit/src/cppunit/ProtectorChain.cpp:20:12 #13 0x2b12025beab6 in (anonymous namespace)::Prot::protect(CppUnit::Functor const, CppUnit::ProtectorContext const) unotest/source/cpp/unobootstrapprotector/unobootstrapprotector.cxx:88:12 #14 0x2b11f2f535d7 in CppUnit::ProtectorChain::ProtectFunctor::operator()() const workdir/UnpackedTarball/cppunit/src/cppunit/ProtectorChain.cpp:20:12 #15 0x2b11fe91cda8 in (anonymous namespace)::Prot::protect(CppUnit::Functor const, CppUnit::ProtectorContext const) unotest/source/cpp/unoexceptionprotector/unoexceptionprotector.cxx:63:16 #16 0x2b11f2f535d7 in CppUnit::ProtectorChain::ProtectFunctor::operator()() const workdir/UnpackedTarball/cppunit/src/cppunit/ProtectorChain.cpp:20:12 #17 0x2b11f2edc111 in CppUnit::DefaultProtector::protect(CppUnit::Functor const, CppUnit::ProtectorContext const) workdir/UnpackedTarball/cppunit/src/cppunit/DefaultProtector.cpp:15:12 #18 0x2b11f2f535d7 in CppUnit::ProtectorChain::ProtectFunctor::operator()() const workdir/UnpackedTarball/cppunit/src/cppunit/ProtectorChain.cpp:20:12 #19 0x2b11f2f4f595 in CppUnit::ProtectorChain::protect(CppUnit::Functor const, CppUnit::ProtectorContext const) workdir/UnpackedTarball/cppunit/src/cppunit/ProtectorChain.cpp:77:18 #20 0x2b11f3016b1b in CppUnit::TestResult::protect(CppUnit::Functor const, CppUnit::Test*, std::string const) workdir/UnpackedTarball/cppunit/src/cppunit/TestResult.cpp:181:10 #21 0x2b11f2f9416a in CppUnit::TestCase::run(CppUnit::TestResult*) workdir/UnpackedTarball/cppunit/src/cppunit/TestCase.cpp:91:5 #22 0x2b11f2f9aaa3 in CppUnit::TestComposite::doRunChildTests(CppUnit::TestResult*) workdir/UnpackedTarball/cppunit/src/cppunit/TestComposite.cpp:64:5 #23 0x2b11f2f99a0d in CppUnit::TestComposite::run(CppUnit::TestResult*) workdir/UnpackedTarball/cppunit/src/cppunit/TestComposite.cpp:23:3 #24 0x2b11f2f9aaa3 in CppUnit::TestComposite::doRunChildTests(CppUnit::TestResult*) workdir/UnpackedTarball/cppunit/src/cppunit/TestComposite.cpp:64:5 #25 0x2b11f2f99a0d in CppUnit::TestComposite::run(CppUnit::TestResult*) workdir/UnpackedTarball/cppunit/src/cppunit/TestComposite.cpp:23:3 #26 0x2b11f30549b6 in CppUnit::TestRunner::WrappingSuite::run(CppUnit::TestResult*) workdir/UnpackedTarball/cppunit/src/cppunit/TestRunner.cpp:47:5 #27 0x2b11f3014a89 in CppUnit::TestResult::runTest(CppUnit::Test*) workdir/UnpackedTarball/cppunit/src/cppunit/TestResult.cpp:148:3 #28 0x2b11f3055ee4 in CppUnit::TestRunner::run(CppUnit::TestResult, std::string const) workdir/UnpackedTarball/cppunit/src/cppunit/TestRunner.cpp:96:3 #29 0x4fc78e in (anonymous namespace)::ProtectedFixtureFunctor::run() const sal/cppunittester/cppunittester.cxx:276:13 #30 0x4f773b in sal_main() sal/cppunittester/cppunittester.cxx:379:14 #31
