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

issues with caml_float_of_hex #7690

Closed
vicuna opened this Issue Dec 13, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Dec 13, 2017

Original bug ID: 7690
Reporter: @oandrieu
Assigned to: @nojb
Status: resolved (set by @nojb on 2017-12-19T14:24:12Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.03.0
Category: standard library
Monitored by: @nojb @gasche @dra27

Bug description

There are some overflow issues with the exponent computations in caml_float_of_hex, which results in some invalid conversions. The integer exp variable overflows, which results in the call to ldexp to overflow when it should underflow (and vice versa).

The test "if (e < INT_MIN || e > INT_MAX) return -1;" is ineffective on Win64 since int and long have the same size there: this results in some difference in behavior between win64 and linux.

And (minor point), the function accept spaces between the 'p' and the exponent digits, whereas strtod and the C99 syntax do not allow spaces there.

Steps to reproduce

$ cat f.ml
let hexstring_of_float x =
Printf.sprintf "%h" x

let () =
let s =
try hexstring_of_float (float_of_string Sys.argv.(1))
with _ -> "invalid" in
print_endline s

$ ocamlopt.opt f.ml

$ ./a.out 0x1.0p-2147483648
infinity

$ ./a.out 0x123456789ABCDEF0p2147483647
0x0p+0

$ ./a.out 0x1p2147483648
-> invalid on linux64
-> infinity on win64

$ ./a.out '0x1p 1'
0x1p+1

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 13, 2017

Comment author: @oandrieu

I can provide a GPR if you think these corner cases are worth fixing.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 13, 2017

Comment author: @nojb

Definitely the overflow one needs fixing; a GPR would be much appreciated!
Regarding the second one, I am not sure if it is a bug or not at the moment.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 13, 2017

Comment author: @oandrieu

there, #1528 : #1528

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 19, 2017

Comment author: @nojb

This PR has now been merged, so marking this resolved.

@vicuna vicuna closed this Dec 19, 2017

@vicuna vicuna added the stdlib label Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.