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

demoting floating float values to unrepresentable types is undefined behavior #75554

Closed
benjaminp opened this issue Sep 6, 2017 · 10 comments
Closed
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@benjaminp
Copy link
Contributor

BPO 31373
Nosy @mdickinson, @vstinner, @benjaminp
PRs
  • bpo-31373: fix undefined floating-point demotions #3396
  • [3.6] bpo-31373: fix undefined floating-point demotions (GH-3396) #3424
  • bpo-31373: remove overly strict float range checks #3486
  • [3.6] bpo-31373: remove overly strict float range checks (GH-3486) #3495
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-09-07.18:35:49.323>
    created_at = <Date 2017-09-06.22:20:42.693>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'demoting floating float values to unrepresentable types is undefined behavior'
    updated_at = <Date 2017-09-12.06:08:52.018>
    user = 'https://github.com/benjaminp'

    bugs.python.org fields:

    activity = <Date 2017-09-12.06:08:52.018>
    actor = 'benjamin.peterson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-09-07.18:35:49.323>
    closer = 'benjamin.peterson'
    components = ['Interpreter Core']
    creation = <Date 2017-09-06.22:20:42.693>
    creator = 'benjamin.peterson'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31373
    keywords = []
    message_count = 10.0
    messages = ['301528', '301602', '301607', '301798', '301799', '301808', '301831', '301840', '301864', '301939']
    nosy_count = 3.0
    nosy_names = ['mark.dickinson', 'vstinner', 'benjamin.peterson']
    pr_nums = ['3396', '3424', '3486', '3495']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31373'
    versions = ['Python 3.6', 'Python 3.7']

    @benjaminp
    Copy link
    Contributor Author

    According to C99, "When a finite value of real floating type is converted to an integer type other than _Bool, the fractional part is discarded (i.e., the value is truncated toward zero). If the value of the integral part cannot be represented by the integer type,the behavior is undefined." So, e.g., (long)x, where x is a double is in general undefined behavior without a range check. We have several cases in CPython where we need to fix this.

    @benjaminp benjaminp added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Sep 6, 2017
    @benjaminp
    Copy link
    Contributor Author

    New changeset a853a8b by Benjamin Peterson in branch 'master':
    bpo-31373: fix undefined floating-point demotions (bpo-3396)
    a853a8b

    @benjaminp
    Copy link
    Contributor Author

    New changeset b036232 by Benjamin Peterson (Miss Islington (bot)) in branch '3.6':
    [3.6] fixes bpo-31373: fix undefined floating-point demotions (GH-3396) (bpo-3424)
    b036232

    @mdickinson
    Copy link
    Member

    There's a (to my mind) unfortunate change in behaviour here. Under normal IEEE 754 rules, some C double values larger than FLT_MAX still round to FLT_MAX under conversion to float.

    Python 3.6:

    >>> import struct
    >>> x = 3.40282356e38
    >>> struct.pack("<f", x)
    b'\xff\xff\x7f\x7f'

    Following the changes in this PR:

    >>> import struct
    >>> x = 3.40282356e38
    >>> struct.pack("<f", x)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OverflowError: float too large to pack with f format

    The original behaviour is correct with respect to IEEE 754; the new behaviour is not. I think this is a case where the cure is worse than the disease.

    @benjaminp
    Copy link
    Contributor Author

    I agree that's bad. Do you know a way to get the IEEE 754 rounding behavior without invoking C undefined behavior?

    @mdickinson
    Copy link
    Member

    Do you know a way to get the IEEE 754 rounding behavior without invoking C undefined behavior?

    One option is to hard-code the actual boundary, which is 2**128 * (1 - 2**-25) (as opposed to FLT_MAX, which is 2**128 * (1 - 2**-24)): values equal to or larger than 2**128 * (1 - 2**-25) in absolute value should raise. But that means assuming IEEE 754 and round-ties-to-even, which isn't an outrageous assumption but does make the solution feel a bit fragile.

    An alternative would be to scale values in the range (FLT_MAX, 2.0 * FLT_MAX] by 0.5 before doing the conversion, something like this:

            if (fabs(x) > FLT_MAX && !Py_IS_INFINITY(x)) {
              double half_x = 0.5 * x;
              if (half_x > FLT_MAX) {
                goto Overflow;
              }
              float half_y = (float)half_x;
              if (half_y > 0.5 * FLT_MAX) {
                goto Overflow;
              }
            }

    @benjaminp
    Copy link
    Contributor Author

    I'm going to undo the changes to getargs.c and floatobject.c. I think the pytime.c change is still correct because the doubles are explicitly rounded before conversion (and the old code checked for error > 1.0).

    It's hard to win here I think. The clang undefined behavior sanitizer uses FLT_MAX to make its determination:
    runtime error: value 3.40282e+38 is outside the range of representable values of type 'float'

    So to satisfy it and preserve the old behavior, we would presumably have to implement the rounding ourselves, which doesn't seem like fun.

    Annex F of C99, which defines the requirements for C implementations using IEE754, says, "the conversions for floating types provide the IEC 60559 conversions between floating-point precisions.". Those are, of course, fully defined. It seems like the undefined behavior sanitizer is being overzealous when the target supports IEEE754.

    @mdickinson
    Copy link
    Member

    It's hard to win here I think.

    Agreed.

    It seems like the undefined behavior sanitizer is being overzealous when the target supports IEEE754.

    Also agreed.

    @benjaminp
    Copy link
    Contributor Author

    New changeset 2bb69a5 by Benjamin Peterson in branch 'master':
    bpo-31373: remove overly strict float range checks (bpo-3486)
    2bb69a5

    @benjaminp
    Copy link
    Contributor Author

    New changeset cb356c2 by Benjamin Peterson in branch '3.6':
    [3.6] bpo-31373: remove overly strict float range checks (GH-3486) (bpo-3495)
    cb356c2

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants