Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

negate after converting to unsigned to avoid UB on minimum signed value #1526

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

spoonincode
Copy link
Member

@spoonincode spoonincode commented Aug 17, 2023

Negating the minimum value of a signed integer is undefined behavior since the resulting value cannot be represented by the type (results in a signed overflow). However, casting to an unsigned and then negating that is defined behavior, and I believe the appropriate solution here, as both of those operations are well defined for all inputs.

@spoonincode
Copy link
Member Author

This isn't consensus, though a change in console output would be.. undesirable. There is an existing test for std::numeric_limits<int128_t>::lowest() to give us some confidence that nothing has actually changed on supported platforms,

void test_print::test_printi128() {
int128_t a(1);
int128_t b(0);
int128_t c(std::numeric_limits<int128_t>::lowest());
int128_t d(-87654323456);
printi128(&a);
prints("\n");
printi128(&b);
prints("\n");
printi128(&c);
prints("\n");
printi128(&d);
prints("\n");
}

leap/unittests/api_tests.cpp

Lines 2649 to 2660 in 6132e3b

// test printi128
auto tx6_trace = CALL_TEST_FUNCTION( *this, "test_print", "test_printi128", {} );
auto tx6_act_cnsl = tx6_trace->action_traces.front().console;
size_t start = 0;
size_t end = tx6_act_cnsl.find('\n', start);
BOOST_CHECK_EQUAL( tx6_act_cnsl.substr(start, end-start), U128Str(1) );
start = end + 1; end = tx6_act_cnsl.find('\n', start);
BOOST_CHECK_EQUAL( tx6_act_cnsl.substr(start, end-start), U128Str(0) );
start = end + 1; end = tx6_act_cnsl.find('\n', start);
BOOST_CHECK_EQUAL( tx6_act_cnsl.substr(start, end-start), "-" + U128Str(static_cast<unsigned __int128>(std::numeric_limits<__int128>::lowest())) );
start = end + 1; end = tx6_act_cnsl.find('\n', start);
BOOST_CHECK_EQUAL( tx6_act_cnsl.substr(start, end-start), "-" + U128Str(87654323456) );

@spoonincode
Copy link
Member Author

I discovered two more cases of this behavior and decided to fix them as part of this PR in f68377f, I would like re-review when you have a chance @greg7mdp and @linh2931

WAST parsing is not consensus: it's only used in unit tests. In this particular case the WAST that tripped this error was from the test

BOOST_FIXTURE_TEST_CASE( f32_f64_conversion_tests, validating_tester ) try {

@@ -239,7 +239,7 @@ bool tryParseInt(ParseState& state,UnsignedInt& outUnsignedInt,I64 minSignedValu
return false;
};

outUnsignedInt = isNegative ? UnsignedInt(-I64(u64)) : UnsignedInt(u64);
outUnsignedInt = isNegative ? -UnsignedInt(I64(u64)) : UnsignedInt(u64);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the I64 cast anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does seem completely redundant now, and everything still passes with it removed..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems redundant to me as well. Probably better to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a little hesitant because this case is a little unique due to the u64->u32 truncation and wasn't sure if -UnsignedInt(u64) vs UnsignedInt(-u64) mattered.., but it seems equivalent.

@spoonincode spoonincode merged commit 1ac0605 into main Aug 22, 2023
22 checks passed
@spoonincode spoonincode deleted the negate_after branch August 22, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants