fix(cl):same float-to-int cast overflow behavior with go#1544
fix(cl):same float-to-int cast overflow behavior with go#1544xushiwei merged 6 commits intogoplus:mainfrom
Conversation
Summary of ChangesHello @luoliwoshang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the float-to-integer conversion mechanism within the compiler's SSA (Static Single Assignment) form. The primary goal is to prevent undefined behavior in LLVM when converting out-of-range floating-point values to narrow integer types. By introducing an intermediate 64-bit conversion and subsequent truncation, the change guarantees that Go's specified wrap/truncate semantics are consistently applied, improving the reliability and correctness of type conversions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a critical issue with float-to-integer conversions that could lead to undefined behavior in LLVM. The approach of routing conversions for narrow integer types through a 64-bit intermediate representation before truncation is sound and correctly implements Go's wrap/truncate semantics while avoiding LLVM's UB. The new castFloatToInt function neatly encapsulates this logic. I have one suggestion to further improve the code's maintainability.
Code Review SummaryThis PR successfully fixes the LLVM undefined behavior for float-to-int conversions on narrow integer types. The implementation is functionally correct and improves security by ensuring deterministic, specification-compliant conversion semantics. Key strengths:
Main suggestion: Add function documentation to explain the rationale for the two-step conversion approach, especially given this addresses issue #1538. See inline comment for details. |
3bdcc83 to
081d4cb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1544 +/- ##
=======================================
Coverage 91.01% 91.01%
=======================================
Files 45 45
Lines 11958 11971 +13
=======================================
+ Hits 10883 10896 +13
Misses 899 899
Partials 176 176 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Now that issue #1538 is fixed by PR #1544, remove the skip marker (;) from expect.txt to enable the cast test. The test runs successfully with no output (it only panics on error). Closes #1561 Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: luoliwoshang <[email protected]>
Summary
Fixed
Problem
The repro from issue #1538 panics because LLVM's
fptosi/fptouion narrow integer types yields poison for out-of-range inputs. That poison is later used and results diverge from Go's expected behavior.Root Cause
ssa/expr.gousedllvm.CreateFPToSI/FPToUIdirectly with the destination integer type. For small widths, the conversion is undefined/poison when the input float is outside the representable range.Fix
ssa/expr.go).References
fptouisemantics (poison on overflow): https://llvm.org/docs/LangRef.html#fptoui-to-instructionfptosisemantics (poison on overflow): https://llvm.org/docs/LangRef.html#fptosi-to-instructionFiles Touched
Verification
Notes