Skip to content

Conversation

@spahrenk
Copy link
Contributor

@spahrenk spahrenk commented Sep 3, 2024

Description

Provides an additional condition for satisfying ISNUM, which is used by cvpak for screening. Currently, the cvpak unit test fails when run on Mac in release mode. (It succeeds in debug.) The bitmap representation of the float nanf generated by the unit test differs slightly between the two modes. The condition used to determine whether the type represents a numeric nan is

(bitmap & bitmask) != bitmask
with bitmask = 0x7f800000L

It is now also required that
(bitmap & bitmask_mac) != bitmask_mac
bitmask_mac = 0x7fc00000L

@spahrenk spahrenk requested a review from nealkruis September 3, 2024 16:33
@spahrenk spahrenk self-assigned this Sep 3, 2024
@nealkruis
Copy link
Contributor

Here's what I have discovered. In "Release", the NaN is coming through as:

01111111110000000000000000000000

which is interpreted as an NCHOICE.

In "Debug", the NaN is coming through as:

11111111110000000000000000000000

which is just a normal NaN.

The change appears to be triggered by clang's optimization flag (and not specific to apple clang). I posted a Stack Overflow question about this.

I couldn't find anywhere that says that certain NaN representations are reserved, and it seems that how a compiler wants to represent NaN is implementation-specific. I found this article that is pretty helpful in terms of understanding the variations of NaN.

For the purposes of this PR, I suggest that we use an explicit bit set to set nanf in the test code instead of relying on the compiler's interpretation.

We should also create an issue to follow up:

  • Redefine ISNCHOICE to exclude both results
  • Improve the documentation comments around CSE's uses (i.e., exploitations) of NaNs
  • Change these macros to inline functions

@nealkruis
Copy link
Contributor

After reviewing the Stack Overflow responses, I believe we should modify the ISNCHOICE mask to exclude 01111111110000000000000000000000. We should keep the nanf as inf/inf, since this is a legitimate NaN interpretation.

Independently (on a separate branch), we need to review our floating point compiler options across compilers for consistency. If we use the same options in clang that we are using in MSVC, we should see more consistent NaN representation for inf/inf.

@spahrenk can you:

  1. Modify this branch to change the ISNCHOICE mask as described above?
  2. Create new issues for:
    • Improve the documentation comments around CSE's uses (i.e., exploitations) of NaNs (e.g., including bit set examples)
    • Change these macros to inline functions
    • Make floating point options consistent across compilers

Thanks!

Copy link
Contributor

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

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

See comments in PR.

@spahrenk
Copy link
Contributor Author

spahrenk commented Sep 6, 2024

$$ \begin{align*} & \text{quantity} && \text{float} && \text{hex} && \text{binary} \\ & \text{Mac/Release}\enspace nanf && nan && 0x7fc00000 && 0b01111111110000000000000000000000 \\ & \text{Mac/Debug}\enspace nanf && -nan & &0xffc00000 && 0b11111111110000000000000000000000\\ & \text{Windows/Release}\enspace nanf && -nan && 0xffc00000 && 0b11111111110000000000000000000000 \\ & \text{Windows/Debug}\enspace nanf && -nan && 0xffc00000 && 0b11111111110000000000000000000000 \\ & \text{ISNUM mask} && 2139095040 && 0x7f800000 && 0b01111111100000000000000000000000 \\ & \text{ISNCHOICE mask} && 4286578688 && 0xff800000 && 0b11111111100000000000000000000000 \\ \end{align*} $$

@spahrenk
Copy link
Contributor Author

spahrenk commented Sep 9, 2024

$$ \begin{align*} & \text{quantity} && \text{float} && \text{hex} && \text{binary} \\ & \text{Mac/Release, optimization on}\enspace nanf && nan && 0x7fc00000 && 0b01111111110000000000000000000000 \\ & \text{Mac/Release, optimization off}\enspace nanf && -nan & &0xffc00000 && 0b11111111110000000000000000000000\\ \end{align*} $$

@nealkruis nealkruis marked this pull request as ready for review September 23, 2024 15:55
@nealkruis nealkruis merged commit 99bf1e1 into main Sep 23, 2024
@nealkruis nealkruis deleted the modify-nan-mac branch September 23, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants