Signed integer overflow in Constraint Elimination pass

[houngkoungting]

Fix #133154

Refined limit check to use MaxNegative and improved code flow for better readability and maintainability. Applied clang-format for consistent formatting.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[nikic]

Why does fixing this bug require changes to MulOverflow? I would expect it to only require the use of MulOverflow.

What do your changes to MulOverflow fix? I would expect this to come with a change to unit tests.

[houngkoungting]

Hi @nikic

This is my first Pull Request, and I wanted to make sure it adheres to LLVM’s standards and best practices. The main reason for rewriting the entire MulOverflow function is to address the lack of handling for the INT_MIN * -1 overflow case (UB error) in the original implementation. This is a common edge case that, if not properly handled, can result in Undefined Behavior (UB) and cause potential errors.

During the modification process, I also reorganized the function structure to improve readability and maintainability. As this is my first contribution, I wanted to ensure that the implementation is clean and meets the quality expected for LLVM.

I have already run the tests using ninja check-llvm, and all test cases passed successfully. Please feel free to let me know if there are any adjustments or improvements I should make.

Just a quick question: The return type should be bool, correct?

std::enable_if_t<std::is_signed_v, ""T -> bool""> MulOverflow(T X, T Y, T &Result) {

I appreciate your time and guidance, and I look forward to your feedback! @houngkoungting

[nikic]

What makes you think that MulOverflow does not handle INT_MIN * -1 correctly? There is a test for it here:

llvm-project/llvm/unittests/Support/MathExtrasTest.cpp

Lines 667 to 672 in 48441cb

	TYPED_TEST(OverflowTest, MulOverflowToMin) {
	TypeParam Result;
	auto MinValue = std::numeric_limits<TypeParam>::min();
	EXPECT_TRUE(MulOverflow<TypeParam>(MinValue, -1, Result));
	EXPECT_EQ(Result, MinValue);
	}

[houngkoungting]

HI @nikic
It looks like you were absolutely right. I definitely made a mistake during my analysis. Thank you so much for your patience and clarification. I'll be more careful next time.

[houngkoungting]

HI @nikic
It looks like you were absolutely right. I definitely made a mistake during my analysis. Thank you so much for your patience and clarification. I'll be more careful next time.