Repository navigation
Avoid overflowing remainder for minimum-value GCD with -1 - #40
Conversation
Return one before the minimum-value shortcuts evaluate MIN % -1. The result is representable, but the intermediate signed division is not. Cover both argument orders in runtime and constexpr tests and document the return-value representability requirement. Generated-by: OpenAI Codex
| if(a == (std::numeric_limits<Integer>::min)()) | ||
| return a == static_cast<Integer>(0) ? gcd_detail::gcd_traits<Integer>::abs(b) : boost::integer::gcd(static_cast<Integer>(a % b), b); | ||
| return a == static_cast<Integer>(0) ? gcd_detail::gcd_traits<Integer>::abs(b) | ||
| : b == static_cast<Integer>(-1) ? static_cast<Integer>(1) |
There was a problem hiding this comment.
I think, this will be incorrect if Integer is unsigned.
There was a problem hiding this comment.
Thanks, that needed to be explicit. As written it could not misfire for an unsigned Integer: numeric_limits<Integer>::min() is zero there, so a == min() means a == 0, and the zero test returned abs(b) before b == static_cast<Integer>(-1) was evaluated. But that depended on the order of the two tests and nothing in the code said so.
9f1a9c8 makes the -1 test conditional on std::numeric_limits<Integer>::is_signed, in both branches, and adds tests for gcd(0, max) and gcd(max, 0) on the unsigned types: at run time in gcd_unsigned_test, and as static_asserts for unsigned and uint64_t in gcd_constexpr14_test.cpp.
To be clear about what the new tests show: they pass with and without the is_signed condition, so they pin the existing behaviour rather than demonstrate a failure in the previous commit. They do fail if the -1 test is moved ahead of the zero test without the condition (gcd(0, 255) then returns 1 for unsigned char), which is the mistake the condition now rules out.
Tested with GCC 13.3: all of libs/integer/test passes with C++11, 14, 17 and 20, and with C++14 and C++20 under UBSan and ASan.
static_cast<Integer>(-1) is the largest value of an unsigned type, not a unit. The comparison could not be reached for such a type, because its minimum is zero and the zero test comes first, but nothing in the code said so. State the condition with numeric_limits<Integer>::is_signed. Add tests for gcd(0, max) and gcd(max, 0) on unsigned types, at run time and in constant expressions.
|
Thanks. |
Related to #33.
gcd(INT_MIN, -1)and the reversed call have the representable result1, but the minimum-value shortcuts evaluateINT_MIN % -1first. That overflows the built-in signed division operation and also prevents constant evaluation.Return
1for the negative unit before taking that remainder. Keep the existing zero handling for unsigned types. Extend the runtime function/evaluator tests and the constexpr tests for both argument orders. Document that the nonnegative result must fit the return type; this does not resolve the unrepresentablegcd(MIN, MIN)case in #33.Validation on Linux:
1without a diagnostic.math.gcd, including exhaustive 8-bit pairs and minimum-value boundaries. The native driver runs under UBSan.common_factor_testexecutable passes, including GMP and fixed/unbounded multiprecision controls. The constexpr test also compiles with those flags.git diff --checkpasses. Native Windows and 32-bit builds were not run; generated documentation was not rebuilt.OpenAI Codex assisted with investigation, implementation and tests. dawnr checked the minimum-value shortcut's guard in a signed-64-bit integer abstraction with seven provers. That check does not prove the GCD algorithm, generic types or compiled C++ implementation.