Bug 143974 - Basic function CStr is failing to properly convert integers after 41 steps in a for loop
Summary: Basic function CStr is failing to properly convert integers after 41 steps in...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
7.3.0.0 alpha0+
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Andreas Heinisch
URL:
Whiteboard: target:7.3.0 target:7.2.3
Keywords: bibisectRequest, regression
Depends on:
Blocks:
 
Reported: 2021-08-20 12:17 UTC by Julian Ragan
Modified: 2021-09-30 13:20 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
MsgBox screenshot with result. (23.31 KB, image/png)
2021-08-20 12:17 UTC, Julian Ragan
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Ragan 2021-08-20 12:17:25 UTC
Created attachment 174445 [details]
MsgBox screenshot with result.

While testing one of my macros I have discovered a bug.
If you run the following macro:

Sub Main
Dim Txt As String
    For i = 1 To 50
        Txt = Txt + CStr(i)
        If (i mod 10 = 0) Then
            Txt = Txt + CHR(10)
        Else
            Txt = Txt + ", "
        End If
    Next i
    MsgBox Txt
End Sub

You will notice, that values 42, 44, 47, 49 have been extended with lots of zeros and one at the end.

Version: 7.3.0.0.alpha0+ / LibreOffice Community
Build ID: 1b06e7e9e8c467de3450077fe8b90be6b7f73e4b
CPU threads: 2; OS: Linux 5.11; UI render: default; VCL: gtk3
Locale: pl-PL (pl_PL.UTF-8); UI: en-US
Calc: threaded
Comment 1 [REDACTED] 2021-08-20 15:50:45 UTC
The title of this report mentions "...convert integers..." but obviously variable "i" is not an integer, but a floating point value. Explicitly using "Dim i As Long" prevents the observed behavior to occur, i.e. use modified macro:

Sub Main
Dim Txt As String
Dim i As Long

    For i = 1 To 50
        Txt = Txt + CStr(i)
        If (i mod 10 = 0) Then
            Txt = Txt + CHR(10)
        Else
            Txt = Txt + ", "
        End If
    Next i
    MsgBox Txt
End Sub


Not sure, whether variables having no explicit type declaration are correctly assumed to have a floating point type. Hence this is not a statement "bug or not a bug".
Comment 2 [REDACTED] 2021-08-20 15:57:16 UTC
Repro in 

Version: 7.3.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: d001836139b28d367cc70b64a13519d173066df1
CPU threads: 1; OS: Windows 10.0 Build 19043; UI render: Skia/Raster; VCL: win
Locale: en-US (en_DE); UI: de-DE
Calc: threaded
Comment 3 Julian Ragan 2021-08-23 06:31:00 UTC
I would say a bug, since previously CStr did trim those extra zeros, I have been running code like this for at least 3 years now.

The variable type on older version is also Variant/Double, but results are as with explicitly declared integer type.
Comment 4 Andreas Heinisch 2021-09-28 08:40:58 UTC
No repro in:
Version: 7.2.1.2 (x86) / LibreOffice Community
Build ID: 87b77fad49947c1441b67c559c339af8f3517e22
CPU threads: 4; OS: Windows 10.0 Build 18362; UI render: Skia/Raster; VCL: win
Locale: de-DE (de_DE); UI: de-DE
Calc: CL
Comment 5 Andreas Heinisch 2021-09-28 17:22:57 UTC
Regression of Bug 107953
Comment 6 Andreas Heinisch 2021-09-28 17:34:55 UTC
Consider this code where the index starts from 42:

Sub Main
Dim Txt As String
    For i = 42 To 50
        Txt = Txt + CStr(i)
        If (i mod 10 = 0) Then
            Txt = Txt + CHR(10)
        Else
            Txt = Txt + ", "
        End If
        Msgbox Typename(i)
    Next i
    MsgBox Txt
End Sub

The first time, the index is an Integer and after the increment it is a double, hence this error. The fix of Bug 107953 made it visible, but I think the culprit is https://gerrit.libreoffice.org/c/core/+/104696 (Bug 85371).

Codepointer:
https://opengrok.libreoffice.org/xref/core/basic/source/runtime/runtime.cxx?r=adb38e36#2641
Comment 7 Andreas Heinisch 2021-09-28 20:10:42 UTC
Consider this small snippet:


Sub Main

  i = 44
  MsgBox TypeName(i) ' prints integer
  i = i + 1
  MsgBox TypeName(i) ' prints double

End Sub

Because of [1], but the calculations are correct (the variable contains 47.000000000000000). However, the homebrew myftoa in [2] imho does not correctly calculate the desired precision. I even missed some precision scaling in [3] for Bug 107953.

Mike what is your opinion about a fix for this problem? Can this be addressed using the rtl_math_doubleToString as in Bug 130725?

[1] https://opengrok.libreoffice.org/xref/core/basic/source/sbx/sbxvalue.cxx?r=010e99c7#1084
[2] https://opengrok.libreoffice.org/xref/core/basic/source/sbx/sbxscan.cxx?r=ef38b9af#320
[3] https://opengrok.libreoffice.org/xref/core/basic/source/sbx/sbxscan.cxx?r=ef38b9af#810
Comment 8 Mike Kaganski 2021-09-28 20:30:31 UTC
(In reply to Andreas Heinisch from comment #7)

Your analysis is completely correct. It's OK to use doubles; doubles allow to work with integers up to 2^52. The problem is in conversion to string. And we *badly* need an accurate method for that.

Unfortunately, conversion of double to string is a difficult thing, affected by multitude of factors like decimal/thousand separators, national numerals, and so on. So there's no function in STL that would fit our variety of different needs perfectly.

But myftoa looks a candidate for eradication. It's simple enough that rtl_math_doubleToString could replace (and then we would deal with one less imperfect implementations).
Comment 9 Commit Notification 2021-09-30 07:28:18 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ed166025123a403fbd679377387a45e097f09d6e

tdf#143575, tdf#143974 - Use rtl::math::doubleToUString to convert numbers to strings

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Commit Notification 2021-09-30 09:34:58 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/8294268307d98d3ea04f18a2ed89212cec516845

tdf#143575, tdf#143974 - Remove custom precision format in BASIC

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Commit Notification 2021-09-30 13:13:57 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "libreoffice-7-2":

https://git.libreoffice.org/core/commit/da0d6541b29f4dd3cd223e15be14dabc2704fb83

tdf#143575, tdf#143974 - Use rtl::math::doubleToUString to convert numbers to strings

It will be available in 7.2.3.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2021-09-30 13:20:15 UTC
Andreas Heinisch committed a patch related to this issue.
It has been pushed to "libreoffice-7-2":

https://git.libreoffice.org/core/commit/bc2896c98795d356d2c1b690d431b94aebd5b5bb

tdf#143575, tdf#143974 - Remove custom precision format in BASIC

It will be available in 7.2.3.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.