Skip to content

Prevent std::ldexp underflowing/overflowing because of hard-coded flo… #1284

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

Merged
merged 1 commit into from
Jul 7, 2025

Conversation

ak-ambi
Copy link
Contributor

@ak-ambi ak-ambi commented Jul 3, 2025

Prevent std::ldexp underflowing/overflowing because of hard-coded float type.
With exceptions disabled, I would expect errno to be set only on real issues. However I was hit by errno set to ERANGE for following example:

T crossover = ldexp(1.0f, iround(99 / -0.654f));

This means that float is not able to fit crossover even for df as small as 99.

Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, and I kicked off the CI run. Any different opinion @jzmaddock?

@mborland mborland dismissed their stale review July 3, 2025 13:45

Not sure why it got marked as requested changes instead of general comments. My mistake

@jzmaddock
Copy link
Collaborator

I think we need a test case, because to my eyes this doesn't actually change anything?

Note that locally I'm unable to reproduce, but that may be because whether ERANGE is set on ldexp underflow is implementation defined. Off the top of my head, I think we need something more like:

T twos_power = iround(T(df / -0.654f), typename policies::normalise<Policy, policies::rounding_error<policies::ignore_error> >::type());
T crossover = twos_power < std::numeric_limits<T>::min_exponent ? 0 : ldexp(1.0f, twos_power);

@ak-ambi
Copy link
Contributor Author

ak-ambi commented Jul 4, 2025

I think we need a test case, because to my eyes this doesn't actually change anything?

I'll try to add test case tomorrow. The notable change is that new code does not cast T to float anymore. Double precision was enough to avoid ERANGE.

Note that locally I'm unable to reproduce, but that may be because whether ERANGE is set on ldexp underflow is implementation defined. Off the top of my head, I think we need something more like:

T twos_power = iround(T(df / -0.654f), typename policies::normalise<Policy, policies::rounding_error<policies::ignore_error> >::type());
T crossover = twos_power < std::numeric_limits<T>::min_exponent ? 0 : ldexp(1.0f, twos_power);

it safe to use min_exponent here? I can see some boost support for FLT_RADIX higher than 2, and min_exponent is min exp for radix as a base.

Other issue is that limit for T is used, but float types are used for iround and ldexp. Main change from my pull request is that T type is used in all calculations.

@jzmaddock
Copy link
Collaborator

Ah yes, or just replacing 1.0f with static_cast(1) would do it. And you're right, my suggestion would fall foul of decimal types, though technically this change just pushes the problem further into the long grass as df > 1021 would still underflow with double.

@ak-ambi
Copy link
Contributor Author

ak-ambi commented Jul 4, 2025

Ok, I think I found the way to make it more resilient using frexp instead of ldexp. No threat of underflow or overflow. Please recheck.

Also I have added the test case that reproduced the problem on my environment. I am using clang compiler.

@ak-ambi ak-ambi requested a review from mborland July 7, 2025 16:49
Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

LGTM. Failures in the drone CI are unrelated

@mborland mborland merged commit 3698582 into boostorg:develop Jul 7, 2025
60 of 61 checks passed
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