strconv: fix rounding in FormatFloat fallback path

Float formatting uses a multiprecision fallback path where Grisu3
algorithm fails. This has a bug during the rounding phase: the
difference between the decimal value and the upper bound is examined
byte-by-byte and doesn't properly handle the case where the first
divergence has a difference of 1.

For instance (using an example from #29491), for the number
498484681984085570, roundShortest examines the three decimal values:

lower: 498484681984085536
d:     498484681984085568
upper: 498484681984085600

After examining the 16th digit, we know that rounding d up will fall
within the bounds unless all remaining digits of d are 9 and all
remaining digits of upper are 0:

d:     ...855xx
upper: ...856xx

However, the loop forgets that d and upper have already diverged and
then on the next iteration sees that the 17th digit of d is actually
lower than the 17th digit of upper and decides that we still can't round
up:

d:     ...8556x
upper: ...8560x

Thus the original value is incorrectly rounded down to
498484681984085560 instead of the closer (and equally short)
498484681984085570.

Thanks to Brian Kessler for diagnosing this bug.

Fix it by remembering when we've seen divergence in previous digits.

This CL also fixes another bug in the same loop: for some inputs, the
decimal value d or the lower bound may have fewer digits than the upper
bound, yet the iteration through the digits starts at i=0 for each of
them. For instance, given the float64 value 1e23, we have

d:      99999999999999991611392
upper: 100000000000000000000000

but the loop starts by comparing '9' to '1' rather than '0' to '1'.

I haven't found any cases where this second bug causes incorrect output
because when the digit comparison fails on the first loop iteration the
upper bound always has more nonzero digits (i.e., the expression
'i+1 < upper.nd' is always true).

Fixes #29491

Change-Id: I58856a7a2e47935ec2f233d9f717ef15c78bb2d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/157697
Run-TryBot: Caleb Spare <cespare@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rémy Oudompheng <remyoudompheng@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

2 files changed, 56 insertions, 11 deletions

diff --git a/src/strconv/ftoa.go b/src/strconv/ftoa.go
index 432521b24f..8ce6ef30b4 100644
--- a/src/strconv/ftoa.go
+++ b/src/strconv/ftoa.go
@@ -289,39 +289,80 @@ func roundShortest(d *decimal, mant uint64, exp int, flt *floatInfo) {
 	// would round to the original mantissa and not the neighbors.
 	inclusive := mant%2 == 0
 
+	// As we walk the digits we want to know whether rounding up would fall
+	// within the upper bound. This is tracked by upperdelta:
+	//
+	// If upperdelta == 0, the digits of d and upper are the same so far.
+	//
+	// If upperdelta == 1, we saw a difference of 1 between d and upper on a
+	// previous digit and subsequently only 9s for d and 0s for upper.
+	// (Thus rounding up may fall outside the bound, if it is exclusive.)
+	//
+	// If upperdelta == 2, then the difference is greater than 1
+	// and we know that rounding up falls within the bound.
+	var upperdelta uint8
+
 	// Now we can figure out the minimum number of digits required.
 	// Walk along until d has distinguished itself from upper and lower.
-	for i := 0; i < d.nd; i++ {
+	for ui := 0; ; ui++ {
+		// lower, d, and upper may have the decimal points at different
+		// places. In this case upper is the longest, so we iterate from
+		// ui==0 and start li and mi at (possibly) -1.
+		mi := ui - upper.dp + d.dp
+		if mi >= d.nd {
+			break
+		}
+		li := ui - upper.dp + lower.dp
 		l := byte('0') // lower digit
-		if i < lower.nd {
-			l = lower.d[i]
+		if li >= 0 && li < lower.nd {
+			l = lower.d[li]
+		}
+		m := byte('0') // middle digit
+		if mi >= 0 {
+			m = d.d[mi]
 		}
-		m := d.d[i]    // middle digit
 		u := byte('0') // upper digit
-		if i < upper.nd {
-			u = upper.d[i]
+		if ui < upper.nd {
+			u = upper.d[ui]
 		}
 
 		// Okay to round down (truncate) if lower has a different digit
 		// or if lower is inclusive and is exactly the result of rounding
 		// down (i.e., and we have reached the final digit of lower).
-		okdown := l != m || inclusive && i+1 == lower.nd
+		okdown := l != m || inclusive && li+1 == lower.nd
 
+		switch {
+		case upperdelta == 0 && m+1 < u:
+			// Example:
+			// m = 12345xxx
+			// u = 12347xxx
+			upperdelta = 2
+		case upperdelta == 0 && m != u:
+			// Example:
+			// m = 12345xxx
+			// u = 12346xxx
+			upperdelta = 1
+		case upperdelta == 1 && (m != '9' || u != '0'):
+			// Example:
+			// m = 1234598x
+			// u = 1234600x
+			upperdelta = 2
+		}
 		// Okay to round up if upper has a different digit and either upper
 		// is inclusive or upper is bigger than the result of rounding up.
-		okup := m != u && (inclusive || m+1 < u || i+1 < upper.nd)
+		okup := upperdelta > 0 && (inclusive || upperdelta > 1 || ui+1 < upper.nd)
 
 		// If it's okay to do either, then round to the nearest one.
 		// If it's okay to do only one, do it.
 		switch {
 		case okdown && okup:
-			d.Round(i + 1)
+			d.Round(mi + 1)
 			return
 		case okdown:
-			d.RoundDown(i + 1)
+			d.RoundDown(mi + 1)
 			return
 		case okup:
-			d.RoundUp(i + 1)
+			d.RoundUp(mi + 1)
 			return
 		}
 	}
diff --git a/src/strconv/ftoa_test.go b/src/strconv/ftoa_test.go
index 055fef99aa..755c986b86 100644
--- a/src/strconv/ftoa_test.go
+++ b/src/strconv/ftoa_test.go
@@ -137,6 +137,10 @@ var ftoatests = []ftoaTest{
 	{383260575764816448, 'f', 0, "383260575764816448"},
 	{383260575764816448, 'g', -1, "3.8326057576481645e+17"},
 
+	// Issue 29491.
+	{498484681984085570, 'f', -1, "498484681984085570"},
+	{-5.8339553793802237e+23, 'g', -1, "-5.8339553793802237e+23"},
+
 	// rounding
 	{2.275555555555555, 'x', -1, "0x1.23456789abcdep+01"},
 	{2.275555555555555, 'x', 0, "0x1p+01"},