New issue
Advanced search Search tips
Starred by 4 users
Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment
Check strtod()/std::stod() implementations, see if dmg_fp can be removed
Project Member Reported by pkasting@chromium.org, Mar 9 2016 Back to list
From an email thread regarding whether we need dmg_fp:

Some Googling turns up http://www.exploringbinary.com/how-strtod-works-and-sometimes-doesnt/ , which ends with links to several articles about cases which various non-dmg_fp implementations got wrong at the time:

http://www.exploringbinary.com/incorrectly-rounded-conversions-in-visual-c-plus-plus/
http://www.exploringbinary.com/incorrectly-rounded-conversions-in-gcc-and-glibc/
http://www.exploringbinary.com/incorrect-decimal-to-floating-point-conversion-in-sqlite/
http://www.exploringbinary.com/double-rounding-errors-in-floating-point-conversions/

I would say we should do the following:
* Go through the examples on those pages and see if std::strtod() is correct on all the current toolchains we support.  The articles suggest, for example, that GCC/glibc have been fixed as of December 2013 (which may be too new for us to have everywhere) while MSVC had not as of 2013.
* If these are not correct, then also check that we have the latest dmg_fp sources.  In the process of writing the article the author found various bugs in dmg_fp that were subsequently fixed.
 
Comment 1 by hua...@chromium.org, Mar 10 2016
Owner: hua...@chromium.org
Comment 2 by hua...@chromium.org, Mar 10 2016
Status: Assigned
Cc: scottmg@chromium.org
Yes, it's only ~9k binary for strtod (might be more on other platforms?), but that code is a bit weird and has been a bit of a hassle (warnings, fuzzing, etc.). So if we can get rid of it now, that would be nice.
Comment 4 by hua...@chromium.org, Mar 14 2016
Cc: hua...@chromium.org
Owner: ----
Status: Available
Ah okay.  I'm busy with other stuff, and it's a good firs bug.  Leaving this one open then.
Project Member Comment 5 by bugdroid1@chromium.org, Jun 15 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d7634d843a6b981d4b9bebe8f94412b6bccc3be

commit 8d7634d843a6b981d4b9bebe8f94412b6bccc3be
Author: scottmg <scottmg@chromium.org>
Date: Wed Jun 15 17:52:45 2016

Add strtod test cases

Adds test cases from
http://www.exploringbinary.com/how-strtod-works-and-sometimes-doesnt/.

These currently fail on (see ps#1):
  - linux_chromium_gn_chromeos_rel
  - linux_chromium_asan_rel_ng
  - linux_chromium_chromeos_ozone_rel_ng
  - linux_chromium_rel_ng
  - cast_shell_linux

but despite history appear to be fine on Windows (as of VS2015). As they
were a bit of fiddling to get up and running, and we don't appear to have
any similar tests for dmg_fp, add these tests so that we can hopefully
drop dmg_fp in the future once std::strtod works more consistently
everywhere. https://crbug.com/95729 suggests another solution where it'd
be good to have these tests.

BUG=593512,615142, 616062 , 588726 ,95729

Review-Url: https://codereview.chromium.org/2044643005
Cr-Commit-Position: refs/heads/master@{#399948}

[modify] https://crrev.com/8d7634d843a6b981d4b9bebe8f94412b6bccc3be/base/strings/string_number_conversions_unittest.cc

Comment 6 by kcwu@chromium.org, Sep 26 2016
If we want to replace dmg_fp, be careful about feature mismatch between libraries. For example,
std::strtod accepts leading spaces but dmg_fp doesn't.
std::strtod accepts hex numbers (0x prefix) but dmg_fp doesn't (by our setup)

Project Member Comment 7 by bugdroid1@chromium.org, Oct 14 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bd66b6c465395634479807d17b5d1cf9f5ac6d6

commit 7bd66b6c465395634479807d17b5d1cf9f5ac6d6
Author: kcwu <kcwu@chromium.org>
Date: Fri Oct 14 22:37:28 2016

Add boundary test case to StringToDouble

The max presentable double is HUGE_VAL=(2^53-1)*(2^971). And
(2^53)*(2^971) is considered overflow.

Added cases above and below (2^53-0.5)*(2^971). The value above is
overflow and the value below is not.

std::strtod on linux doesn't pass the above case (it returns inf correctly
but doesn't set errno=ERANGE).

BUG=593512

Review-Url: https://codereview.chromium.org/2415353002
Cr-Commit-Position: refs/heads/master@{#425496}

[modify] https://crrev.com/7bd66b6c465395634479807d17b5d1cf9f5ac6d6/base/strings/string_number_conversions_unittest.cc

Note that this would likely be web exposed if we did it too, we need to be cautious about the edge cases like whitespace and hex as kcwu@ mentions.
Summary: Check strtod()/std::stod() implementations, see if dmg_fp can be removed (was: Check std::strtod() implementations, see if dmg_fp can be removed)
I just noticed today that strtod() is the C function and std::stod() is the C++11 one.  They look to have a slightly different interface (the latter expects std::string, among other things); I don't know if they have different conversino behavior.
Project Member Comment 10 by sheriffbot@chromium.org, Nov 1
Labels: Hotlist-Recharge-Cold
Status: Untriaged
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Recharge-Cold
Status: Available
Sign in to add a comment