Skip to content

Conversation

@khwilliamson
Copy link
Member

  1. Configure for all but ancient perls tells us if a UV can always losslelssly fit into an NV. Use that if available.
  2. Configure for all but ancient perls tells us the mantissa width. Use that if available instead of assuming it is 53.
  3. The code for uniqnum assumes this is a 2's complement machine. Change to not assume that.
  4. There were several undefined behaviors marked by running ASan on this code; all stemmed from using signed values, where unsigned would work. There was an issue with abs(IV_MIN) not being representable in an IV on a 2's complement machine; and left shifting a negative value is undefined.
  5. I found myself drawing a picture to try to understand the behavior of item 4. And I think the picture is clearer than the previous comments. so I changed to use it.

Since 5.6, Configure calculates if all possible UV/IV's values can be
contained losslessly in an NV.  Fallback to our previous determination
for earlier perls.
Use the value calculated by Configure, falling back to 53 if not
available.

It became available during 5.7 development
It is a good assumption; but not a fully accurate one, and it's trivial
to fix.
There were several undefined behaviors marked by running ASan on this
code; all stemmed from using signed values, where unsigned would work.
There was an issue with abs(IV_MIN) not being representable in an IV on
a 2's complement machine; and left shifting a negative value is
undefined.

This commit solves this by converting to use an unsigned value, and
stealing the macro now in the perl core to take the abs(IV_MIN) without
undefined behavior.

I found myself drawing a picture to try to understand the behavior of
this.  And I think the picture is clearer than the previous text. so I
changed to use it.  And the intermediate results that were there really
just interrupted the flow of the picture, so I removed them.
@mauke
Copy link
Contributor

mauke commented Aug 17, 2025

The code for uniqnum assumes this is a 2's complement machine. Change to not assume that.

NB: Starting with C23, C requires two's complement.

@sisyphus
Copy link
Contributor

sisyphus commented Aug 18, 2025

This PR builds and tests fine for me on MS Windows, on a wide variety of perls across versions 5.16.3 to 5.43.1 - including 32-bit, 64-bit, -Duselongdouble, -Dusequadmath.
It was only 51 builds, mainly focusing on builds from over the last 3 years - so there are plenty of holes (some of them quite large) in the coverage.

One thing I noticed was that t/uniqnum.t produced the following warning (but only in the 5.42.0 and 5.43.1 builds) :

Possible precedence problem between ! and numeric ne (!=) at t/uniqnum.t line 215.

I don't think that's even related to the changes made by this PR, but line 215 is:

$nanish != 0 && !$nanish != $NaN;

I was able to silence the warning by changing that line to

$nanish != 0 && (!$nanish) != $NaN;

I think that retains the original (and intended) behaviour.

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