Skip to content

[libc] Correct edge cases, errno, and floating point exceptions for math functions #61092

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

Closed
lntue opened this issue Mar 1, 2023 · 5 comments
Assignees
Labels

Comments

@lntue
Copy link
Contributor

lntue commented Mar 1, 2023

There are many cases of math functions not returning correct errno and floating point exceptions according to the C standard. The following cases are reported by Fred J. Tydeman:

  • log(1) = -0 for FE_DOWNWARD
  • logb(0) is not raising divide-by-zero
  • sqrt(-infinity) => missing errno == EDOM
  • fmod(-NAN,-NAN) = +NAN
  • remainder(-infinity, -infinity) => no invalid
  • nextafter(-max, -infinity) => no overflow, no inexact
  • fdim(-inf, -inf) => invalid (should be no FP exception)
  • fdim(-max, -den) => inexact (should be no FP exception)
  • fma( -min, +min, +min ) => missing underflow
  • asinf() issue: no inexact
@lntue lntue self-assigned this Mar 1, 2023
@llvmbot
Copy link
Member

llvmbot commented Mar 1, 2023

@llvm/issue-subscribers-libc

@michaelrj-google
Copy link
Contributor

closing as out of date, feel free to reopen if this is still an issue.

@Flandini
Copy link
Contributor

Flandini commented Apr 15, 2024

I checked on these and some still remain. I have fixes for some, tests for some, but I wanted to get a PR in first to add more FPMatchers that include checks for errno and exceptions.

Report Fixed Status/Notes
log(1) = -0 for FE_DOWNWARD Yes log(1) = 0 for all rounding modes. There is a test that log(1) = 0 for all rounding modes.
logb(0) not raising div-by-zero Yes A pole error occurs on zero, errno and exceptions properly set for this case, I'm adding tests in a PR.
sqrt(-infinity) not setting errno == EMOD No The ISA-specific implementations aren't setting EDOM on negative, non-zero numbers. The generic implementation and the sqrt_80_bit_long_double generic implementation do not set EDOM or raise FE_INVALID. Fixes for the generic implementation seem straight forward. I can add a check that the argument is less than zero before the asm sqrt insn in the ISA-specific impls; I would guess the best way to do this is with FPBits to avoid changing FP flags/exceptions?
fmod(-NAN,-NAN) = +NAN ?? Is this wrong? I can't tell from 7.12.10.1.3 or F.10.7.1 what the behavior for two NaN arguments should be. We seem to match glibc on this. This behavior still happens and is already tested for, just a question of if we have the semantics correct?
remainder(-infinity, -infinity) => no invalid, also remquo No Changing to a domain error on first arg infinity or second arg zero and neither are NaN, so raising FE_INVALID and setting errno == EDOM. This matches glibc's implementation now. This is a simple fix, just setting errno and exceptions before returning in a branch, I will make a PR for the fix and a test.
nextafter(-max, -infinity) => no overflow, no inexact Yes Looks fine now. I'm adding tests in a PR.
fdim(-inf, -inf) => invalid (should be no FP exception) No Looks like this is caused by compiler optimizations https://godbolt.org/z/z3x8zGWzb evaluating both sides of a conditional operator on O1-O3 in this code. Posted this in the cxx channel in discord and philnik said: It looks like you have to set FENV_ACCESS to on to inspect the FP exceptions. It's implementation-defined whether it's on or off by default, and in most compilers, including clang, it's off by default. I got the same optimization from an if-statement messing around in compiler explorer, so should we set FENV_ACCESS for safety?
fdim(-max, -den) => inexact (should be no FP exception) No Same reason as above. I tweaked the code to fix the above and added a test, and the exception is gone.
fma(-min, +min, +min) => missing underflow Yes Adding tests in a PR.
asinf() issue: no inexact Yes on the inexact. I've tested with a min subnormal, and inexact is raised. I don't think errno is always set though.

I think it would be nice to have a programmatic way to exercise and test the different pre-processor if-groups; some of these fail the added tests in different ways depending on whether generic implementations are used. There were similar bugs with some FP depending on whether BigInt is used for float128 or not. I did want to learn more about the build, so I'm happy to work on this.

We should probably also make sure we have all edge cases tested for all the math.h functions and strengthen the FP matchers to also check errno and exceptions. Some of the behavior has changed since the initial report, and it would be nice to have tests for all edge case behavior on all the math.h functions. Thoughts on a long-standing issue task list to make sure they're all tested?

@Flandini
Copy link
Contributor

Going to open issues for the cases that still remain.

@michaelrj-google
Copy link
Contributor

Thanks for going through this and opening the issues!

lntue pushed a commit that referenced this issue May 6, 2024
… macros (#88816)

Adds more FP test macros for the upcoming test adds for #61092 and the
issues opened from it: #88768, #88769, #88770, #88771, #88772.

Fix bug in `{EXPECT,ASSERT}_FP_EXCEPTION`. `EXPECT_FP_EXCEPTION(0)`
seems to be used to test that an exception did not happen, but it always
does `EXPECT_GE(... & 0, 0)` which never fails.

Update and refactor tests that break after the above bug fix. An
interesting way things broke after the above change is that
`ForceRoundingMode` and `quick_get_round()` were raising the inexact
exception, breaking a lot of the `atan*` tests.

The changes for all files other than `FPMatcher.h` and
`libc/test/src/math/smoke/RoundToIntegerTest.h` should have the same
semantics as before. For `RoundToIntegerTest.h`, lines 56-58 before the
changes do not always hold since this test is used for functions with
different exception and errno behavior like `lrint` and `lround`. I've
deleted those lines for now, but tests for those cases should be added
for the different nearest int functions to account for this.

Adding @nickdesaulniers for review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants