RFR for 8043740 (Doubles with large exponents overflow to Infinity incorrectly)
Hi Brian/all, Fix for the above-mentioned bug is complete. If you (or anyone else as appropriate) could please review the changes and let me know of your comments that would be great! Link for the webrev: http://www.sandipan.net/public/webrevs/8043740/webrev.00/ The patch is inlined below my signature. Cheers, SR --- old/src/share/classes/sun/misc/FloatingDecimal.java 2014-06-01 22:33:34.947651253 -0400 +++ new/src/share/classes/sun/misc/FloatingDecimal.java 2014-06-01 22:33:34.827651251 -0400 @@ -1992,7 +1992,10 @@ break expLoop; // stop parsing exponent. } } - int expLimit = BIG_DECIMAL_EXPONENT+nDigits+nTrailZero; + + int expLimit = BIG_DECIMAL_EXPONENT + nDigits + nTrailZero + - (nLeadZero - decPt); + if ( expOverflow || ( expVal > expLimit ) ){ // // The intent here is to end up with --- /dev/null 2014-05-28 21:22:08.061671015 -0400 +++ new/test/java/lang/Double/Bug8043740.java 2014-06-01 22:33:35.227651260 -0400 @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2001, 2014, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8043740 + * @summary Test for Double.parseDouble incorrect normalization of exponent. + */ +public class Bug8043740 { + + public static void main(String[] args) { + String text326 = "00.000000000000000001e326"; // expect 1e308 + double value326 = Double.parseDouble(text326); + if (Double.isInfinite(value326)) { + throw new RuntimeException("String [" + text326 + + "] was incorrectly parsed to Infinity."); + } + } + +}
Hi all - I've made a quick revision to that last patch. Please find inline the latest link + patch. http://www.sandipan.net/public/webrevs/8043740/webrev.01/ Cheers, SR --- old/src/share/classes/sun/misc/FloatingDecimal.java 2014-06-02 09:32:20.000000000 -0400 +++ new/src/share/classes/sun/misc/FloatingDecimal.java 2014-06-02 09:32:19.000000000 -0400 @@ -1994,19 +1994,29 @@ } int expLimit = BIG_DECIMAL_EXPONENT+nDigits+nTrailZero; if ( expOverflow || ( expVal > expLimit ) ){ - // - // The intent here is to end up with - // infinity or zero, as appropriate. - // The reason for yielding such a small decExponent, - // rather than something intuitive such as - // expSign*Integer.MAX_VALUE, is that this value - // is subject to further manipulation in - // doubleValue() and floatValue(), and I don't want - // it to be able to cause overflow there! - // (The only way we can get into trouble here is for - // really outrageous nDigits+nTrailZero, such as 2 billion. ) - // - decExp = expSign*expLimit; + // There is still a chance that the exponent will be + // safe to use: if it would eventually decrease + // due to a negative decExp, and that number is below the limit. + // We check for that here. + if ( ( expSign == 1 && decExp < 0 ) + && ( expVal + decExp ) < expLimit ) { + // Cannot overflow - adding a positive and negative number. + decExp = expVal + decExp; + } else { + // + // The intent here is to end up with + // infinity or zero, as appropriate. + // The reason for yielding such a small decExponent, + // rather than something intuitive such as + // expSign*Integer.MAX_VALUE, is that this value + // is subject to further manipulation in + // doubleValue() and floatValue(), and I don't want + // it to be able to cause overflow there! + // (The only way we can get into trouble here is for + // really outrageous nDigits+nTrailZero, such as 2 billion. ) + // + decExp = expSign*expLimit; + } } else { // this should not overflow, since we tested // for expVal > (MAX+N), where N >= abs(decExp) --- /dev/null 2014-06-02 09:32:21.000000000 -0400 +++ new/test/java/lang/Double/Bug8043740.java 2014-06-02 09:32:20.000000000 -0400 @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2001, 2014, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8043740 + * @summary Test for Double.parseDouble incorrect normalization of exponent. + */ +public class Bug8043740 { + + public static void main(String[] args) { + String text = "00.000000000000000001e326"; + double expected = Double.parseDouble("1e308"); + double value = Double.parseDouble(text); + if (Double.compare(expected, value) != 0) { + throw new RuntimeException("String [" + text + + "] was incorrectly parsed to [" + value + "]"); + } + } + +} Sandipan Razzaque | www.sandipan.net On Sun, Jun 1, 2014 at 10:55 PM, Sandipan Razzaque <me@sandipan.net> wrote:
Hi Brian/all,
Fix for the above-mentioned bug is complete. If you (or anyone else as appropriate) could please review the changes and let me know of your comments that would be great!
Link for the webrev:
http://www.sandipan.net/public/webrevs/8043740/webrev.00/
The patch is inlined below my signature.
Cheers, SR
--- old/src/share/classes/sun/misc/FloatingDecimal.java 2014-06-01 22:33:34.947651253 -0400 +++ new/src/share/classes/sun/misc/FloatingDecimal.java 2014-06-01 22:33:34.827651251 -0400 @@ -1992,7 +1992,10 @@ break expLoop; // stop parsing exponent. } } - int expLimit = BIG_DECIMAL_EXPONENT+nDigits+nTrailZero; + + int expLimit = BIG_DECIMAL_EXPONENT + nDigits + nTrailZero + - (nLeadZero - decPt); + if ( expOverflow || ( expVal > expLimit ) ){ // // The intent here is to end up with --- /dev/null 2014-05-28 21:22:08.061671015 -0400 +++ new/test/java/lang/Double/Bug8043740.java 2014-06-01 22:33:35.227651260 -0400 @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2001, 2014, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8043740 + * @summary Test for Double.parseDouble incorrect normalization of exponent. + */ +public class Bug8043740 { + + public static void main(String[] args) { + String text326 = "00.000000000000000001e326"; // expect 1e308 + double value326 = Double.parseDouble(text326); + if (Double.isInfinite(value326)) { + throw new RuntimeException("String [" + text326 + + "] was incorrectly parsed to Infinity."); + } + } + +}
Hello Sandipan, Finally got this off the back burner … This review request follows this thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-June/027086.html in which you provided a patch (thank you!) for: https://bugs.openjdk.java.net/browse/JDK-8043740 I’ve created an updated webrev here: http://cr.openjdk.java.net/~bpb/8043740/webrev.00/ Aside from minor reformatting there is no update to the proposed FloatingDecimal change. I have not however included the test class Bug8043740 from the contributed patch opting instead to update the existing ParseDouble test by adding a few more strings to the goodStrings array. The changes to FloatingDecimal appear reasonable to me. I am wondering however if lines 2001-2002 should not be changed to include !expOverflow in the conditional: 2001 if (!expOverflow && expSign == 1 && decExp < 0 2002 && (expVal + decExp) < expLimit) { 2003 // Cannot overflow: adding a positive and negative number. 2004 decExp += expVal; I don’t think that it’s possible for both expOverflow and the conditionals at lines 2001-2002 of the webrev to all be true, but the additional test would guarantee branching to the correct block. Thanks, Brian On Jun 2, 2014, at 6:08 AM, Sandipan Razzaque <me@sandipan.net> wrote:
I've made a quick revision to that last patch. Please find inline the latest link + patch.
Hi Brian - Thanks for your review! I think your point about adding !expOverflow to that conditional makes perfect sense. We're only looking to account for expVal > expLimit where decExp would be adjusted downward. Please adjust as appropriate. Cheers, SR Sandipan Razzaque | www.sandipan.net On Fri, Sep 19, 2014 at 4:54 PM, Brian Burkhalter < brian.burkhalter@oracle.com> wrote:
Hello Sandipan,
Finally got this off the back burner …
This review request follows this thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-June/027086.html
in which you provided a patch (thank you!) for:
https://bugs.openjdk.java.net/browse/JDK-8043740
I’ve created an updated webrev here:
http://cr.openjdk.java.net/~bpb/8043740/webrev.00/
Aside from minor reformatting there is no update to the proposed FloatingDecimal change. I have not however included the test class Bug8043740 from the contributed patch opting instead to update the existing ParseDouble test by adding a few more strings to the goodStrings array.
The changes to FloatingDecimal appear reasonable to me. I am wondering however if lines 2001-2002 should not be changed to include !expOverflow in the conditional:
2001 if (!expOverflow && expSign == 1 && decExp < 0 2002 && (expVal + decExp) < expLimit) { 2003 // Cannot overflow: adding a positive and negative number. 2004 decExp += expVal;
I don’t think that it’s possible for both expOverflow and the conditionals at lines 2001-2002 of the webrev to all be true, but the additional test would guarantee branching to the correct block.
Thanks,
Brian
On Jun 2, 2014, at 6:08 AM, Sandipan Razzaque <me@sandipan.net> wrote:
I've made a quick revision to that last patch. Please find inline the latest link + patch. http://www.sandipan.net/public/webrevs/8043740/webrev.01/
Hello, Do the additional cases in the regression tests full cover the proposed revision of the code changes? Thanks, -Joe On 9/20/2014 4:06 PM, Sandipan Razzaque wrote:
Hi Brian -
Thanks for your review!
I think your point about adding !expOverflow to that conditional makes perfect sense. We're only looking to account for expVal > expLimit where decExp would be adjusted downward. Please adjust as appropriate.
Cheers, SR
Sandipan Razzaque | www.sandipan.net
On Fri, Sep 19, 2014 at 4:54 PM, Brian Burkhalter < brian.burkhalter@oracle.com> wrote:
Hello Sandipan,
Finally got this off the back burner …
This review request follows this thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-June/027086.html
in which you provided a patch (thank you!) for:
https://bugs.openjdk.java.net/browse/JDK-8043740
I’ve created an updated webrev here:
http://cr.openjdk.java.net/~bpb/8043740/webrev.00/
Aside from minor reformatting there is no update to the proposed FloatingDecimal change. I have not however included the test class Bug8043740 from the contributed patch opting instead to update the existing ParseDouble test by adding a few more strings to the goodStrings array.
The changes to FloatingDecimal appear reasonable to me. I am wondering however if lines 2001-2002 should not be changed to include !expOverflow in the conditional:
2001 if (!expOverflow && expSign == 1 && decExp < 0 2002 && (expVal + decExp) < expLimit) { 2003 // Cannot overflow: adding a positive and negative number. 2004 decExp += expVal;
I don’t think that it’s possible for both expOverflow and the conditionals at lines 2001-2002 of the webrev to all be true, but the additional test would guarantee branching to the correct block.
Thanks,
Brian
On Jun 2, 2014, at 6:08 AM, Sandipan Razzaque <me@sandipan.net> wrote:
I've made a quick revision to that last patch. Please find inline the latest link + patch. http://www.sandipan.net/public/webrevs/8043740/webrev.01/
Hi Joe, Yes, and then some. There are two cases which fail before applying the patch and neither fails thereafter. One of these cases matches the test in the original patch. The remaining cases pass both before and after but I added them as edges cases anyway. Thanks, Brian On Sep 21, 2014, at 9:52 PM, Joe Darcy <joe.darcy@oracle.com> wrote:
Do the additional cases in the regression tests full cover the proposed revision of the code changes?
Hi Brian, Okay; looks good to go back. Thanks, -Joe On 9/22/2014 8:53 AM, Brian Burkhalter wrote:
Hi Joe,
Yes, and then some. There are two cases which fail before applying the patch and neither fails thereafter. One of these cases matches the test in the original patch. The remaining cases pass both before and after but I added them as edges cases anyway.
Thanks,
Brian
On Sep 21, 2014, at 9:52 PM, Joe Darcy <joe.darcy@oracle.com <mailto:joe.darcy@oracle.com>> wrote:
Do the additional cases in the regression tests full cover the proposed revision of the code changes?
participants (3)
-
Brian Burkhalter
-
Joe Darcy
-
Sandipan Razzaque