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

Make C-lib math functions introspection more robust #35325

Merged
merged 3 commits into from
Apr 24, 2020
Merged

Make C-lib math functions introspection more robust #35325

merged 3 commits into from
Apr 24, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Apr 23, 2020

On Solaris-like operating systems (such as SmartOS 2020 and OpenIndiana hipster), when built without optimization enabled (e.g. -O2); if the hard-coded value of -0.0 is used for x argument of atan2(3), it produces value of PI correctly, when y is 0.0.

However, it produces an incorrect result if the value of x is soft-coded and a unary operator is used, like -x.

The following program demonstrates the issue:

gcc -xc - -lm <<EOF
#include <stdio.h>
#include<math.h>
int main() {
  double x = 0.0;
  printf("hard-coded %g vs. soft-coded %g\n",
    atan2(0.0, -0.0),
    atan2(0.0, -x));
  return 0;
}
EOF

./a.out

gives us: hard-coded 3.14159 vs. soft-coded 0, with -O2, it produces the correct value (of PI for both hard- and soft- coded cases).

With clang-9 on OI, it gives us: hard-coded 0 vs. soft-coded 0 with and without -O2.

This is most likely the bug in libm, which I have reported in upstream channel.

We already have a fallback implementation under HAVE_COMPATIBLE_ATAN2.
This PR makes the test case more robust, so it fails on Solaris.

@ghost
Copy link

ghost commented Apr 23, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@am11
Copy link
Member Author

am11 commented Apr 23, 2020

cc @jkotas, @janvorli

@janvorli, I have also fixed two CMake endif mismatch warnings with this PR, which was caused by my previous PR. e.g.

CMake Warning (dev) in src/pal/tests/palsuite/exception_handling/pal_sxs/test1/CMakeLists.txt:
  A logical block opening on the line
    /usbkey/home/root/runtime/src/coreclr/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/CMakeLists.txt:25 (if)
  closes on the line
    /usbkey/home/root/runtime/src/coreclr/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/CMakeLists.txt:27 (endif)
  with mis-matching arguments.

@danmoseley
Copy link
Member

@maryamariyan bad label :)

@maryamariyan
Copy link
Member

maryamariyan commented Apr 23, 2020

@danmosemsft I updated the bot labeler earlier today to undo applying labels based on the second fallback model.
for this specific PR however, we dont get to the fallback model at all since the confidence score is 0.92 (pretty high already using the main model).

@tannergooding
Copy link
Member

Which tests caught this? We should have coverage of all the IEEE required inputs/outputs on both the managed and native side. For example:

@danmoseley
Copy link
Member

danmoseley commented Apr 23, 2020

@maryamariyan oh ok. It's actually pretty reasonable from a bot POV.

@am11
Copy link
Member Author

am11 commented Apr 23, 2020

@tannergooding, currently:

@am11
Copy link
Member Author

am11 commented Apr 23, 2020

From cppreference - atan2:

If y is ±0 and x is negative or -0, ±π is returned

this case is violating on Illumos, which palsuite exercises. The incorrect result is produced, when compilers (clang, or gcc) do not make use of use of the builtin atan2, and instead calls the libm function which has this bug. Only gcc with -O2 uses the builtin function. We can force the compiler to skip builtin in all cases with -fno-builtin, then both clang and gcc, with and without -O2 produce 0 (instead of π) for hard- and soft- coded cases.

@am11 am11 changed the title Make atan2 introspection more robust Make C-lib math functions introspection more robust Apr 23, 2020
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!


result = atan2(0.0, -0.0);
result = atan2(y, -x);
if (fabs(pi - result) > 0.0000001) {
Copy link
Member

Choose a reason for hiding this comment

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

This (and others checking against a "close enough" result) should ideally get cleaned up as well. IEEE floating-point is deterministic, if nothing else, and so the result should be exactly Math.PI (which is the nearest representation of "true pi").
I'll log a bug and see about following up after this gets merged.

Copy link
Member

Choose a reason for hiding this comment

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

Logged #35341

@jkotas jkotas merged commit 87706ae into dotnet:master Apr 24, 2020
@am11 am11 deleted the feature/solaris/pal-test-fixes branch April 24, 2020 17:18
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants