From 3b689dcd5947e603d7376a6c6f3679bc9d50e7ed Mon Sep 17 00:00:00 2001 From: hexagon-recursion Date: Wed, 27 Dec 2023 11:36:06 +0300 Subject: [PATCH 1/5] Fix bugs in fixed division --- src/fixed.h | 35 ++++++++++++++++++++--------------- src/test/TestFixed.cpp | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 15 deletions(-) create mode 100644 src/test/TestFixed.cpp diff --git a/src/fixed.h b/src/fixed.h index a602664974a..02d78cef693 100644 --- a/src/fixed.h +++ b/src/fixed.h @@ -149,34 +149,39 @@ class fixedf { } friend fixedf operator/(const fixedf &a, const fixedf &b) { - // 128-bit divided by 64-bit, to make sure high bits are not lost - Sint64 quotient_hi = a.v >> (64 - FRAC); - Uint64 quotient_lo = a.v << FRAC; - Sint64 d = b.v; - int isneg = 0; - Sint64 remainder = 0; - - if (d < 0) { - d = -d; - isneg = 1; + Uint64 abs_a = a.v, abs_b = b.v; + bool is_neg = false; + if (a.v < 0) { + abs_a = -abs_a; + is_neg = !is_neg; } - + if (b.v < 0) { + abs_b = -abs_b; + is_neg = !is_neg; + } + // 128-bit divided by 64-bit, to make sure high bits are not lost + Uint64 quotient_hi = abs_a >> (64 - FRAC); + Uint64 quotient_lo = abs_a << FRAC; + Uint64 remainder = 0; for (int i = 0; i < 128; i++) { Uint64 sbit = (Uint64(1) << 63) & quotient_hi; + // The most significant bit of remainder is 0 because abs_b <= (Uint64(1) << 63) remainder <<= 1; - if (sbit) remainder |= 1; + if (sbit) { + remainder |= 1; + } // shift quotient left 1 { quotient_hi <<= 1; if (quotient_lo & (Uint64(1) << 63)) quotient_hi |= 1; quotient_lo <<= 1; } - if (remainder >= d) { - remainder -= d; + if (remainder >= abs_b) { + remainder -= abs_b; quotient_lo |= 1; } } - return (isneg ? -Sint64(quotient_lo) : quotient_lo); + return is_neg ? -quotient_lo : quotient_lo; } friend bool operator==(const fixedf &a, const fixedf &b) { return a.v == b.v; } friend bool operator!=(const fixedf &a, const fixedf &b) { return a.v != b.v; } diff --git a/src/test/TestFixed.cpp b/src/test/TestFixed.cpp new file mode 100644 index 00000000000..0586ba230f3 --- /dev/null +++ b/src/test/TestFixed.cpp @@ -0,0 +1,33 @@ +#include +#include "doctest.h" +#include "fixed.h" + +TEST_CASE("Fixed") +{ + SUBCASE("operator ==") + { + // 42.0 == 42.0 + CHECK(fixed(42, 1) == fixed(42, 1)); + // 123.0 != 321.0 + CHECK(fixed(123, 1) != fixed(321, 1)); + // 7.0 == 7.0 + CHECK(fixed(0x7'00000000) == fixed(7, 1)); + } + + SUBCASE("operator /") + { + // 3.0 / 3.0 == 1.0 + CHECK(fixed(3, 1) / fixed(3, 1) == fixed(1, 1)); + // -3.0 / 3.0 == -1.0 + // There used to be a bug causing this to produce an incorrect result + CHECK(fixed(-3'00000000) / fixed(3, 1) == fixed(-1'00000000)); + // 0x40000000.00000000 / 0x40000000.00000001 == 0x00000000.ffffffff (because we round down) + // There used to be a bug causing this to produce an incorrect result + CHECK(fixed(0x40000000'00000000) / fixed(0x40000000'00000001) == fixed(0x00000000'ffffffff)); + // fixed(INT_MIN) / fixed(INT_MIN) used to trigger a signed int overflow + Sint64 min = std::numeric_limits::min(); + CHECK(fixed(min) / fixed(min) == fixed(1, 1)); + // fixed(INT_MIN) / -1.0 used to trigger a signed int overflow + CHECK(fixed(min) / fixed(-1'00000000) == fixed(min) / fixed(-1'00000000)); + } +} From 43a8208e806553617e1d471979d560149ec7b8f9 Mon Sep 17 00:00:00 2001 From: hexagon-recursion Date: Fri, 26 Apr 2024 09:01:47 +0300 Subject: [PATCH 2/5] Add copyright --- src/test/TestFixed.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/TestFixed.cpp b/src/test/TestFixed.cpp index 0586ba230f3..8447e78e8e5 100644 --- a/src/test/TestFixed.cpp +++ b/src/test/TestFixed.cpp @@ -1,3 +1,6 @@ +// Copyright © 2008-2024 Pioneer Developers. See AUTHORS.txt for details +// Licensed under the terms of the GPL v3. See licenses/GPL-3.txt + #include #include "doctest.h" #include "fixed.h" From aa8d7235c85f57ba5cb622a1e96fc2c612f35ba4 Mon Sep 17 00:00:00 2001 From: hexagon-recursion Date: Fri, 26 Apr 2024 10:22:12 +0300 Subject: [PATCH 3/5] Use std::abs() Note: this is technically undefined behavior if a.v or b.v is _exactly_ INT64_MIN, but the upside that this compiles to faster code even under -Og --- src/fixed.h | 12 ++---------- src/test/TestFixed.cpp | 12 ++++++------ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/fixed.h b/src/fixed.h index 02d78cef693..ad6b117725d 100644 --- a/src/fixed.h +++ b/src/fixed.h @@ -149,16 +149,8 @@ class fixedf { } friend fixedf operator/(const fixedf &a, const fixedf &b) { - Uint64 abs_a = a.v, abs_b = b.v; - bool is_neg = false; - if (a.v < 0) { - abs_a = -abs_a; - is_neg = !is_neg; - } - if (b.v < 0) { - abs_b = -abs_b; - is_neg = !is_neg; - } + Uint64 abs_a = std::abs(a.v), abs_b = std::abs(b.v); + bool is_neg = int(a.v < 0) ^ int(b.v < 0); // 128-bit divided by 64-bit, to make sure high bits are not lost Uint64 quotient_hi = abs_a >> (64 - FRAC); Uint64 quotient_lo = abs_a << FRAC; diff --git a/src/test/TestFixed.cpp b/src/test/TestFixed.cpp index 8447e78e8e5..dee3feb3e6b 100644 --- a/src/test/TestFixed.cpp +++ b/src/test/TestFixed.cpp @@ -1,7 +1,7 @@ // Copyright © 2008-2024 Pioneer Developers. See AUTHORS.txt for details // Licensed under the terms of the GPL v3. See licenses/GPL-3.txt -#include +#include #include "doctest.h" #include "fixed.h" @@ -27,10 +27,10 @@ TEST_CASE("Fixed") // 0x40000000.00000000 / 0x40000000.00000001 == 0x00000000.ffffffff (because we round down) // There used to be a bug causing this to produce an incorrect result CHECK(fixed(0x40000000'00000000) / fixed(0x40000000'00000001) == fixed(0x00000000'ffffffff)); - // fixed(INT_MIN) / fixed(INT_MIN) used to trigger a signed int overflow - Sint64 min = std::numeric_limits::min(); - CHECK(fixed(min) / fixed(min) == fixed(1, 1)); - // fixed(INT_MIN) / -1.0 used to trigger a signed int overflow - CHECK(fixed(min) / fixed(-1'00000000) == fixed(min) / fixed(-1'00000000)); + // There used to be an edge case that triggered a signed int overflow when negating + // the final output if the truncation to 64 bits and conversio to Sint64 produced INT64_MIN + // because -INT64_MIN is undefined behavour + // Example: 0x00000000.80000000 / -0x00000000.00000001 + CHECK(fixed(0x80000000) / fixed(-1) == fixed(INT64_MIN)); } } From 859bf2652e315d468f47fa8a6076acc8c54c802e Mon Sep 17 00:00:00 2001 From: hexagon-recursion Date: Fri, 26 Apr 2024 12:47:41 +0300 Subject: [PATCH 4/5] Oops: forgot inclide --- src/fixed.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fixed.h b/src/fixed.h index ad6b117725d..fbede318664 100644 --- a/src/fixed.h +++ b/src/fixed.h @@ -6,6 +6,7 @@ #include #include +#include template class fixedf { From 0ed527ccee020bc3a50b113ce48d011e53837b1d Mon Sep 17 00:00:00 2001 From: hexagon-recursion Date: Fri, 26 Apr 2024 12:53:58 +0300 Subject: [PATCH 5/5] Add more tests; Fix a bug in test; Better comments --- src/test/TestFixed.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/test/TestFixed.cpp b/src/test/TestFixed.cpp index dee3feb3e6b..54997a7c694 100644 --- a/src/test/TestFixed.cpp +++ b/src/test/TestFixed.cpp @@ -21,16 +21,21 @@ TEST_CASE("Fixed") { // 3.0 / 3.0 == 1.0 CHECK(fixed(3, 1) / fixed(3, 1) == fixed(1, 1)); - // -3.0 / 3.0 == -1.0 - // There used to be a bug causing this to produce an incorrect result - CHECK(fixed(-3'00000000) / fixed(3, 1) == fixed(-1'00000000)); - // 0x40000000.00000000 / 0x40000000.00000001 == 0x00000000.ffffffff (because we round down) - // There used to be a bug causing this to produce an incorrect result + // Negative numerators used to cause incorrect results + // Example: -3.0 / 3.0 should be -1.0 + CHECK(fixed(-0x3'00000000) / fixed(3, 1) == fixed(-0x1'00000000)); + // Denominators greater then 0x40000000.00000000 or less then -0x40000000.00000000 used to cause incorrect results + // Example: 0x40000000.00000000 / 0x40000000.00000001 should return 0x00000000.ffffffff (because we round down) CHECK(fixed(0x40000000'00000000) / fixed(0x40000000'00000001) == fixed(0x00000000'ffffffff)); // There used to be an edge case that triggered a signed int overflow when negating // the final output if the truncation to 64 bits and conversio to Sint64 produced INT64_MIN // because -INT64_MIN is undefined behavour // Example: 0x00000000.80000000 / -0x00000000.00000001 CHECK(fixed(0x80000000) / fixed(-1) == fixed(INT64_MIN)); + // For completeness: + // 3.0 / -3.0 == -1.0 + CHECK(fixed(3, 1) / fixed(-0x3'00000000) == fixed(-0x1'00000000)); + // -3.0 / -3.0 == 1.0 + CHECK(fixed(-0x3'00000000) / fixed(-0x3'00000000) == fixed(1, 1)); } }