flatsurf / e-antic

Embedded algebraic number fields

https://flatsurf.github.io/e-antic/libeantic/

GNU Lesser General Public License v3.0

12 stars 11 forks source link

Make renf_elem_class comparisons more relaxed #211

Closed jamesjer closed 2 years ago

jamesjer commented 3 years ago

Fixes https://github.com/flatsurf/e-antic/issues/194. With GDB, I can see that in the cases mentioned in that issue, renf_elem_cmp_ui returns 2, rather than 1. With this commit, all tests pass on i386 Fedora Rawhide.

saraedum commented 3 years ago

@videlec if that's the case, then the documentation in https://github.com/flatsurf/e-antic/blob/master/libeantic/e-antic/renf_elem.h#L145 is wrong.

saraedum commented 3 years ago

(@jamesjer sorry, I totally missed this PR.)

codecov[bot] commented 3 years ago

Codecov Report

Merging #211 (ecadf9e) into master (e437124) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   92.05%   92.10%   +0.05%     
==========================================
  Files         105      105              
  Lines        2052     2053       +1     
==========================================
+ Hits         1889     1891       +2     
+ Misses        163      162       -1

Impacted Files	Coverage Δ
libeantic/srcxx/renf_elem_class.cpp	`97.69% <100.00%> (ø)`
libeantic/e-antic/renf_class.hpp	`100.00% <0.00%> (+50.00%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e437124...ecadf9e. Read the comment docs.

saraedum commented 2 years ago

I tried to understand how this is possible. And I am not much wiser.

So renf_elem_cmp_ui relies on renf_elem_cmp_fmpq this in turn calls a number of functions to determine the actual integer return value:

renf_elem_sgn which returns fmpz_sgn or arf_sgn or 0
fmpq_sgn which returns fmpz_sgn of its numerator
_fmpq_cmp which returns fmpz_sgn or fmpz_cmp or fmpz_cmp_si
arf_sgn which only returns -1, 0, 1.

So, indirectly also:

fmpz_sgn which essentially returns mpz_sgn
fmpz_cmp which does not make a guarantee about returning only -1, 0, 1 in its documentation but in the actual implementation returns mpz_sgn or mpz_cmp.
mpz_cmp which does not make a guarantee about reurning only -1, 0, 1 in its documentation but in the actual implementations returns only -1, 0, 1.
mpz_sgn which claims only to return -1, 0, 1.

saraedum commented 2 years ago

Clearly, some of these implementations are relying on the fact that some _cmp only returns -1, 0, 1 even though that's not part of the API.

But that does not seem to be the source of the issue here.

saraedum commented 2 years ago

I'll try out the instructions you left on the issue to dig a bit deeper. However, I guess the solution will be this PR + making sure that we are not relying on the assumption that cmp returns -1, 0, 1 anywhere else.

saraedum commented 2 years ago

@jamesjer I followed your instructions linked in the issue and I cannot reproduce the error you get.

PASS: renfxx/t-cmp
PASS: renfxx/t-constructor
PASS: renfxx/t-floor
PASS: renfxx/t-get
PASS: renfxx/t-get_num_den
PASS: renfxx/t-get_str
PASS: renfxx/t-hash
PASS: renfxx/t-predicates
PASS: renfxx/t-num_content
PASS: renfxx/t-pow
PASS: renfxx/t-stream
============================================================================
Testsuite summary for libeantic 1.0.3
============================================================================
# TOTAL: 47
# PASS:  47
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
make[5]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic/test'
make[4]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic/test'
make[3]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic/test'
make[2]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic/test'
make[2]: Entering directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic'
make[2]: Nothing to be done for 'check-am'.
make[2]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic'
make[1]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3/libeantic'
make[1]: Entering directory '/builddir/build/BUILD/e-antic-1.0.3'
make[1]: Nothing to be done for 'check-am'.
make[1]: Leaving directory '/builddir/build/BUILD/e-antic-1.0.3'
+ RPM_EC=0
++ jobs -p
+ exit 0
Processing files: e-antic-1.0.3-2.fc36.i686
Executing(%doc): /bin/sh -e /var/tmp/rpm-tmp.MAvcCv
+ umask 022
+ cd /builddir/build/BUILD
+ cd e-antic-1.0.3
+ DOCDIR=/builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/doc/e-antic
+ export LC_ALL=C
+ LC_ALL=C
+ export DOCDIR
+ /usr/bin/mkdir -p /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/doc/e-antic
+ cp -pr AUTHORS /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/doc/e-antic
+ cp -pr README.md /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/doc/e-antic
+ RPM_EC=0
++ jobs -p
+ exit 0
Executing(%license): /bin/sh -e /var/tmp/rpm-tmp.NZ5ZOA
+ umask 022
+ cd /builddir/build/BUILD
+ cd e-antic-1.0.3
+ LICENSEDIR=/builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/licenses/e-antic
+ export LC_ALL=C
+ LC_ALL=C
+ export LICENSEDIR
+ /usr/bin/mkdir -p /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/licenses/e-antic
+ cp -pr COPYING /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/licenses/e-antic
+ cp -pr COPYING.LESSER /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386/usr/share/licenses/e-antic
+ RPM_EC=0
++ jobs -p
+ exit 0
Provides: e-antic = 1.0.3-2.fc36 e-antic(x86-32) = 1.0.3-2.fc36 libeantic.so.1 libeanticxx.so.1
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires: libantic.so.0 libarb.so.2 libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1.3) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4) libc.so.6(GLIBC_2.34) libc.so.6(GLIBC_2.4) libeantic.so.1 libflint.so.16 libgcc_s.so.1 libgcc_s.so.1(GCC_3.0) libgcc_s.so.1(GCC_3.3.1) libgcc_s.so.1(GLIBC_2.0) libgmp.so.10 libstdc++.so.6 libstdc++.so.6(CXXABI_1.3) libstdc++.so.6(CXXABI_1.3.5) libstdc++.so.6(CXXABI_1.3.9) libstdc++.so.6(GLIBCXX_3.4) libstdc++.so.6(GLIBCXX_3.4.11) libstdc++.so.6(GLIBCXX_3.4.14) libstdc++.so.6(GLIBCXX_3.4.18) libstdc++.so.6(GLIBCXX_3.4.21) libstdc++.so.6(GLIBCXX_3.4.29) libstdc++.so.6(GLIBCXX_3.4.9) rtld(GNU_HASH)
Obsoletes: python3-pyeantic < 1.0.1-2
Processing files: e-antic-devel-1.0.3-2.fc36.i686
Provides: e-antic-devel = 1.0.3-2.fc36 e-antic-devel(x86-32) = 1.0.3-2.fc36
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires: libeantic.so.1 libeanticxx.so.1
Processing files: e-antic-debugsource-1.0.3-2.fc36.i686
Provides: e-antic-debugsource = 1.0.3-2.fc36 e-antic-debugsource(x86-32) = 1.0.3-2.fc36
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Processing files: e-antic-debuginfo-1.0.3-2.fc36.i686
Provides: debuginfo(build-id) = 1d32cb15cdb78f466351179eeac0258cf7e1f948 debuginfo(build-id) = 83694d7bdc71cfc323b6ee380689f9d7aa96ad4d e-antic-debuginfo = 1.0.3-2.fc36 e-antic-debuginfo(x86-32) = 1.0.3-2.fc36 libeantic.so.1.0.2-1.0.3-2.fc36.i386.debug libeanticxx.so.1.0.2-1.0.3-2.fc36.i386.debug
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Recommends: e-antic-debugsource(x86-32) = 1.0.3-2.fc36
Checking for unpackaged file(s): /usr/lib/rpm/check-files /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386
Wrote: /builddir/build/RPMS/e-antic-devel-1.0.3-2.fc36.i686.rpm
Wrote: /builddir/build/RPMS/e-antic-1.0.3-2.fc36.i686.rpm
Wrote: /builddir/build/RPMS/e-antic-debugsource-1.0.3-2.fc36.i686.rpm
Wrote: /builddir/build/RPMS/e-antic-debuginfo-1.0.3-2.fc36.i686.rpm
Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.geXZAw
+ umask 022
+ cd /builddir/build/BUILD
+ cd e-antic-1.0.3
+ /usr/bin/rm -rf /builddir/build/BUILDROOT/e-antic-1.0.3-2.fc36.i386
+ RPM_EC=0
++ jobs -p
+ exit 0

saraedum commented 2 years ago

That's really odd. What am I doing differently?

I also checked the sources and your patch is not in there yet.

saraedum commented 2 years ago

I see, the t-cmp check is already patched in that tarball.

saraedum commented 2 years ago

Hm…but the test still passes when I revert your patch:

commit 2e0198d85dd48e32ce6bce13d49afd5019d1f036
Author: Jerry James <loganjerry@gmail.com>
Date:   Fri Jul 16 11:57:51 2021 -0600

    Add -negative-int patch to work around 32-bit test failure

diff --git a/e-antic-negative-int.patch b/e-antic-negative-int.patch
new file mode 100644
index 0000000..27730a9
--- /dev/null
+++ b/e-antic-negative-int.patch
@@ -0,0 +1,11 @@
+--- a/libeantic/test/renfxx/t-cmp.cpp  2021-06-25 08:06:12.000000000 -0600
++++ b/libeantic/test/renfxx/t-cmp.cpp  2021-07-16 11:49:56.363242945 -0600
+@@ -94,7 +94,7 @@ TEMPLATE_TEST_CASE("Relational Operators
+     const auto& K = GENERATE_REF(take(16, renf_classs(state)));
+
+     auto a = GENERATE_REF(take(8, renf_elem_classs(state, K)));
+-    T b = GENERATE(0, std::numeric_limits<T>::min(), std::numeric_limits<T>::max());
++    T b = GENERATE(0, 0, std::numeric_limits<T>::max());
+
+     check_relop(a, b);
+ }

saraedum commented 2 years ago

Thanks for proposing a fix here @jamesjer. I extended your approach a bit and merged it in #220. Should be fixed now.