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

big_int_of_string incorrectly parses some hexa literals. #6759

Closed
vicuna opened this issue Jan 23, 2015 · 8 comments
Closed

big_int_of_string incorrectly parses some hexa literals. #6759

vicuna opened this issue Jan 23, 2015 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Jan 23, 2015

Original bug ID: 6759
Reporter: strub
Status: closed (set by @damiendoligez on 2015-03-27T21:24:25Z)
Resolution: fixed
Priority: normal
Severity: major
Version: 4.02.1
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: otherlibs
Tags: patch
Has duplicate: #6875
Monitored by: @gasche @hcarty @yakobowski

Bug description

For instance, using OCaml 4.02.1:

On 32 bit systems:

string_of_big_int (big_int_of_string ("0x1000000000000000")) ;;

  • : string = "0"

On 64 bit systems:

string_of_big_int (big_int_of_string "0x1000000000000000") ;;

  • : string = "1152921504606846976"

string_of_big_int (big_int_of_string "0x100000000000000000") ;;

  • : string = "0"

string_of_big_int (big_int_of_string "0x10000000000000000000000000000000") ;;

  • : string = "0"

File attachments

@vicuna
Copy link
Author

vicuna commented Feb 19, 2015

Comment author: @damiendoligez

The bug is in Nat.sys_nat_of_string.

@vicuna
Copy link
Author

vicuna commented Feb 21, 2015

Comment author: dcassirame

The error is indeed in that function: the loop uses "pint" instead of "pmax" to check how many string digits have been obtained from parsing the input string.

@vicuna
Copy link
Author

vicuna commented Mar 7, 2015

Comment author: @gasche

I looked at this function and I cannot be convinced that the bug is as Didier describes (that said, I don't understand much of the code, which is rather painful to follow). The documentation of make_power_base says that pint is the number of characters that fit an integer, so using pint rather to control the end of an integer reference seems correct.

I just uploaded a patch on the testsuite that checks for the bug. Is anyone (Didier?) interested in proposing a concrete patch that solves the issue?

@vicuna
Copy link
Author

vicuna commented Mar 12, 2015

Comment author: dcassirame

I played around with the code before posting that note, and it seemed that using "pmax" did fix the issue. The patch therefore is trivial, but if your test case fails with it, I guess my impression was wrong.

@vicuna
Copy link
Author

vicuna commented Mar 12, 2015

Comment author: @gasche

I have not tested whether using "pmax" fix the issue, and I can believe you it does. But I don't understand why, and I am not sure that is the right fix (and that it fixes all cases) because it does not make much sense to me. So I don't want to apply it without an explanation of what is going on.

@vicuna
Copy link
Author

vicuna commented Mar 17, 2015

Comment author: dcassirame

I understand. I uploaded the patch for your convenience. I don't quite remember exactly the reason the original code did not work as intended: I think the value used to check overflow is too optimistic as to how many digits can be parsed before filling the temporary variable up; in the case of hexadecimal strings, it falls exactly on the maximum value allowed by this temporary, so the temporary overflows to zero. I agree with you that this algorithm is a quite complex. Using less digits does work, but I cannot say if using pmax is optimal (ie, if using more than pmax will always overflow).
(I remember now that I used Octave to check the results of the function when I did my tests, but I could only check the most significant digits, so this bug definitely deserves more investigation).

@vicuna
Copy link
Author

vicuna commented Mar 27, 2015

Comment author: @damiendoligez

The source of the problem is that make_power_base is seriously broken.

@vicuna
Copy link
Author

vicuna commented Mar 27, 2015

Comment author: @damiendoligez

Fixed in 4.02 branch (rev 15972).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant