-
Notifications
You must be signed in to change notification settings - Fork 145
riscv: Fix feholdexcept() #324
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #324 +/- ##
=======================================
Coverage 72.09% 72.09%
=======================================
Files 233 233
Lines 6139 6139
Branches 1607 1607
=======================================
Hits 4426 4426
Misses 1420 1420
Partials 293 293 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing the same thing as BSD.
I'm not sure this works.
https://github.com/freebsd/freebsd-src/blob/master/lib/msun/riscv/fenv.h#L188-L195
The problem is that feholdexcept() does not save floating point environment, but feupdateenv() loads floating point environment. Therefore, in the lrint() function, "fenv_t env" is not initialized and a random value from the stack is loaded into the fcsr register. https://github.com/JuliaMath/openlibm/blob/master/src/s_lrint.c#L59. This throws an "Illegal instruction" exception. |
@inkydragon Thoughts on whether we should merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the feholdexcept() function to correctly store the current floating point environment in *__envp and update its return value to indicate success.
- Added the __rfs(*__envp) call to store the floating point state.
- Changed the return value from -1 to 0 to align with a successful operation.
Comments suppressed due to low confidence (2)
include/openlibm_fenv_riscv.h:193
- Consider adding a brief inline comment explaining the purpose of __rfs in storing the floating point environment for better maintainability.
__rfs(*__envp);
include/openlibm_fenv_riscv.h:197
- Returning 0 indicates success; please confirm that this change fully aligns with the API contract for feholdexcept() in this context.
return (0);
The feholdexcept() function must store the current floating point environment in *__envp.
Related to #321