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

Float literal 17.328679084777833f0 is not correctly rounded #49689

Closed
c42f opened this issue May 8, 2023 · 5 comments
Closed

Float literal 17.328679084777833f0 is not correctly rounded #49689

c42f opened this issue May 8, 2023 · 5 comments
Labels
bug Indicates an unexpected problem or unintended behavior correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing maths Mathematical functions parser Language parsing and surface syntax

Comments

@c42f
Copy link
Member

c42f commented May 8, 2023

17.328679084777833f0 is not correctly rounded when parsed by the Julia parser:

julia> [big"17.328679084777833" - 17.328679084777833f0
        big"17.328679084777833" - nextfloat(17.328679084777833f0)]
2-element Vector{BigFloat}:
  9.536743173750000000000000000000000000000000000000000000000000000000000819773977e-07
 -9.536743154374999999999999999999999999999999999999999999999999999999999180226023e-07

Noticed over at JuliaLang/JuliaSyntax.jl#134 (comment)

I haven't dug deep into whatever the problem is here (likely in the flisp read code), but the float literal parsing in JuliaSyntax.jl avoids whatever it is by calling jl_strtof_c directly. We see:

julia> (@ccall jl_strtof_c("17.328679084777833"::Ptr{UInt8}, C_NULL::Ptr{Ptr{UInt8}})::Cfloat) - 17.328679084777833f0
1.9073486f-6

CC @oscardssmith

@oscardssmith oscardssmith added bug Indicates an unexpected problem or unintended behavior parser Language parsing and surface syntax maths Mathematical functions labels May 9, 2023
@inkydragon
Copy link
Member

inkydragon commented May 9, 2023

This is probably due to the fact that we performed rounding twice.

julia/src/support/strtod.c

Lines 278 to 281 in 7ef78e4

JL_DLLEXPORT float jl_strtof_c(const char *nptr, char **endptr)
{
return (float) jl_strtod_c(nptr, endptr);
}

xref: #5988

#include <stdlib.h>  
#include <stdio.h>  

int main()
{
    const char* str;
    char* stopstring;
    float ff;
    double dd;

    str = "17.328679084777833";
    ff = strtof(str, &stopstring);
    dd = strtod(str, &stopstring);
    printf("string = %s\n", str);
    printf("strtod = %f\n", dd);
    printf("strtof = %f\n", ff);
    printf("  d->f = %f\n", (float)dd);
}

out

string = 17.328679084777833
strtod = 17.328679
strtof = 17.328680
  d->f = 17.328678

test with

  • win10 + MSVC 2019
  • win10 + gcc 12.2.0 (mingw64)
  • Ubuntu 20.04.6 + gcc 9.4.0 (WSL2)

@oscardssmith
Copy link
Member

yeah, that's just broken. why do we do it that way?

@inkydragon
Copy link
Member

inkydragon commented May 9, 2023

Good news: windows now includes the _strtod_l function, so we no longer need to implement jl_strtod_c ourselves!

Bad news: mingw's _strtof_l function is implemented by _strtod_l

float _strtof_l( const char *nptr, char **endptr, _locale_t _Locale)
{
  const double ret = _strtod_l(nptr, endptr, _Locale);

https://github.com/mirror/mingw-w64/blob/eff726c461e09f35eeaed125a3570fa5f807f02b/mingw-w64-crt/stdio/_strtof_l.c#L12-L14

Note the commit time: _strtof_l was first added 6 months ago

So we still need a precise implementation of strtof.
Will there be a problem if we ignore the locale and use strtof directly?

@vtjnash
Copy link
Member

vtjnash commented May 9, 2023

That seems more-or-less what everyone does. For example, here's the source for gdtoa-strtof from Apple (gdtoa is the upstream distribution that most *nix seem to use): https://opensource.apple.com/source/Libc/Libc-391/gdtoa/FreeBSD/gdtoa-strtof.c.auto.html

@LilithHafner LilithHafner added the correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing label Aug 7, 2023
@oscardssmith
Copy link
Member

Closing now that JuliaSyntax is the default parser (which rounds this correctly).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing maths Mathematical functions parser Language parsing and surface syntax
Projects
None yet
Development

No branches or pull requests

5 participants