Repository navigation
avoid signed negation overflow in integer_times_pow10 - #421
Open
sahvx655-wq wants to merge 1 commit into
Open
sahvx655-wq wants to merge 1 commit into
sahvx655-wq wants to merge 1 commit into
Conversation
Negating the most negative int64_t is undefined behaviour. GCC at -O2 uses that to turn the fast-path mantissa bound into a signed compare and returns the wrong sign, so take the magnitude in uint64_t instead.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
int64_toverload ofinteger_times_pow10takes the magnitude with-mantissa, which is signed overflow when the mantissa is the most negativeint64_t. Reading the overload I noticed the tests sweepnumeric_limits<int64_t>::max()but nevermin(), so I ran that value under theFASTFLOAT_SANITIZEflags and it aborts withparse_number.h:506: runtime error: negation of -9223372036854775808 cannot be represented in type 'int64_t'; in a C++20 constant expression the call does not compile at all. It is not only a sanitiser complaint: GCC 16.1 at-O2(aarch64) assumes the negated value is non-negative and emits a signed compare (bgt) for the fast-path mantissa bound, so the wrapped value passes the Clinger gate andinteger_times_pow10(INT64_MIN, 0)returns0x1p+63wherefrom_charson the same digits gives-0x1p+63.Taking the magnitude in
uint64_tis well defined for every input, and clang emits identical assembly for the overload at-O2, so nothing changes for values that already worked. The negation happens inside the overload, so that is the only place it can be put right: the function is documented to accept anyint64_t, and left alone the sign of the result depends on which compiler and optimisation level built the caller. I addedmin()cases beside the existingmax()ones intests/basictest.cpp; against the old header they fail the value check under GCC-O2and abort with SIGABRT in the sanitised build, and they pass with the change.