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
math/big: underflow panic in roundShortest #20867
Comments
Can reproduce on tip and on go1.8.3 so not a recent regression. |
c.f. golang/go#20867 Conversion back to Vector should have normalized but was not. This is now fixed. Signed-off-by: David Symonds <dsymonds@golang.org>
Change https://golang.org/cl/52011 mentions this issue: |
Broken since Go 1.8 so not for Go 1.10 at this point. |
I've been looking into this and have an idea on how I might approach this. Reference: // math/big/ftoa.go
func roundShortest(d *decimal, x *Float) {
// *** SOME CODE ***
// ---PART 1---
mant := nat(nil).set(x.mant)
exp := int(x.exp) - mant.bitLen()
s := mant.bitLen() - int(x.prec+1)
switch {
case s < 0:
mant = mant.shl(mant, uint(-s))
case s > 0:
mant = mant.shr(mant, uint(+s))
}
// ---PART 2---
exp += s
var lower decimal
var tmp nat
lower.init(tmp.sub(mant, natOne), exp)
var upper decimal
upper.init(tmp.add(mant, natOne), exp)
// *** SOME CODE ***
} PART 1 The // s = mant.bitLen() - x.prec - 1
if mant.bitLen()-1 < x.prec { // s negative case
if mant.bitLen() > 0 {
// mant.bitLen() cancels out the -1 so that |mant.bitLen()-1 - x.prec| <= maxUint
mant.shl(mant, x.prec - uint(mant.bitLen - 1))
} else {
mant.shl(mant, x.prec) // x.prec is possibly maxUint32
mant.shl(mant, 1)
}
} else if mant.bitLen()-1 > x.prec { // s pos case.
// bitLen() - (x.prec+1) is no larger than maxInt32 on a 32-bit machine.
mant.shr(mant, x.prec - uint(mant.bitLen) - x.prec -1)
} In the above I have made the assumption that Part 2 We can apply an approach similar to PART 1 here. Change Next step is to deal with the fact that exp has range if x.exp-1 < x.prec { // exp=x.exp - x.prec - 1 is neg
if x.exp-1 >= 0 { // |exp| <= maxUint32
lower.init(tmp.sub(mant, natOne), x.prec - uint(x.exp - 1), neg)
upper.init(tmp.sub(mant, natOne), x.prec - uint(x.exp - 1), neg)
} else { // |exp| can exceed maxUint32
lower.init(tmp.sub(mant, natOne), x.prec, neg)
lower.newShr(x.exp-1)
upper.init(tmp.add(mant, natOne), x.prec, neg)
upper.newShr(x.exp-1)
}
} else { // exp=x.exp - x.prec - 1 is pos
lower.init(tmp.sub(mant, natOne), uint(x.exp) - x.prec - 1, pos)
upper.init(tmp.sub(mant, natOne), uint(x.exp) - x.prec - 1, pos)
}
|
@zairm easiest way to do this is to actually upload a draft patch to gerrit and ask for feedback there. It's much easier to read it and it's also really easy for others to cherrypick it locally and test it. You can write
in the commit message to make sure the patch doesn't get submitted in case it's not ready yet. |
Anyway I feel that any patch that requires to change a substantial amount of code and/or slows down the common code path just to fix the corner case reported in this issue has a low chance to be accepted. That's the reason I gave up on the old (incorrect) patch I had, actually. I coudn't find a simple way to fix this and I didn't want to significantly change |
This is why I was hesitant to start work on it myself. It appears to me there are no easy solutions to something that seems simple and unlikely enough to prohibit even a moderately significant change. I severely hope I'm wrong but my hunch is a fix would require significant changes to |
panics with the following stack trace:
(https://play.golang.org/p/68rBsop-pW)
The text was updated successfully, but these errors were encountered: