Bug 149447 - Some Calc functions called via a FunctionAccess service are accepting type Byte, but treat it as if it is ShortInt (-128..127).
Summary: Some Calc functions called via a FunctionAccess service are accepting type By...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Mike Kaganski
URL:
Whiteboard: target:7.5.0
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-03 19:44 UTC by Wolfgang Jäger
Modified: 2022-06-28 13:16 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Wolfgang Jäger 2022-06-03 19:44:54 UTC
The subject says it all.

To reproduce the issue run the following code stzepwise. 

Sub dings
Dim fa As Object, byH1 As String, byH2 As String, byDbl As Double
Dim by As Byte, by2 As Byte, zero As Long
fa   = CreateUnoService("com.sun.star.sheet.FunctionAccess")
by   = 255 REM or something in 128<= something <= 255
byH1 = fa.callFunction("DEC2HEX", Array(by       , 4))
byH2 = fa.callFunction("DEC2HEX", Array(by + zero, 4))
REM Not a good idea to treat type Byte as if it is ShortInt -128..127.
REM Bad idea to do it in an inconsistent way. 
byDbl = CDbl(by)
by2  = by + 1
End Sub

Well, DEC2HEX iks made for Calc, and there is no type Short (ShortInt), but if it applies conversion, it should do it consistently via Double.
Comment 1 Mike Kaganski 2022-06-03 20:06:33 UTC
I am not sure that is is specific to DEC2HEX. IIRC, there is a special code in the Basic bridge that converts Basic values to the minimal UNO datatype. But I didn't check if this one is related to that code.
Comment 2 Wolfgang Jäger 2022-06-04 06:00:59 UTC
It's the same with EXP in place of DEC2HEX.
It seems the culprit isn't the function code of every single functioon, but the way the FunctionAccess service passes parameters to the functions.

Well, Calc functions don't need to know about numeric types except Double. 
I assumed it was this way, but didn't find much time yet to check.
(Will now be off for a while.)
Comment 3 Wolfgang Jäger 2022-06-24 09:29:31 UTC
Descibing the current state of my experiences and how I interpret them, I would consider to change the subject to:

FunctionAcces.callFunction may attempt to automatically convert a parameter to Double, but wrongly treats the BASIC type Byte as if it is ShortInt (-128..127) then.
Comment 4 Mike Kaganski 2022-06-24 10:25:27 UTC
The problem is the mismatch between Basic's Byte type (unsigned; 0..255) and UNO Byte [1], which is *signed* [2].

Calling a UNO method from Basic *always* includes conversion from Basic types to UNO types; for simple types, that is implemented in sbxToUnoValueImpl [3], which calls getUnoTypeForSbxValue (and that returns the signed type for an SbxBYTE [4]).

This happens even before the next "Choose "smallest" representation for int values because up cast is allowed, downcast not" hackery, so the signed -128..+127 type is used in the following manipulations.

Initially I thought we are doomed (because I imagined we are bound to the TypeClass constants), but immediately in the mentioned "Choose "smallest" representation for int values because up cast is allowed, downcast not" hackery, there are uses of cppu::UnoType<sal_uInt8> - i.e., an UNO type for *unsigned* 8-bit values. So the obvious solution would be to check the value of the passed Basic byte in the getUnoTypeForSbxValue, and if it's greater than 127, use the unsigned 8-bit type, otherwise, use the old signed one (to minimize possible changes).

[1] https://api.libreoffice.org/docs/idl/ref/namespacecom_1_1sun_1_1star_1_1uno.html#a00683ed3ec24b47c36ead10a20d6f328aa7f492d725033c06576ac4ba21007297
[2] https://wiki.openoffice.org/wiki/Documentation/DevGuide/ProUNO/Simple_Types
[3] https://opengrok.libreoffice.org/xref/core/basic/source/classes/sbunoobj.cxx?r=f71606c9&mo=35778&fi=978#978
[4] https://opengrok.libreoffice.org/xref/core/basic/source/classes/sbunoobj.cxx?r=f71606c9&mo=35778&fi=978#841
Comment 5 Mike Kaganski 2022-06-24 10:53:10 UTC
(In reply to Mike Kaganski from comment #4)
> in the mentioned "Choose "smallest"
> representation for int values because up cast is allowed, downcast not"
> hackery, there are uses of cppu::UnoType<sal_uInt8> - i.e., an UNO type for
> *unsigned* 8-bit values.

Oh my. Debugging that, I see that cppu::UnoType<sal_uInt8> corresponds to TypeClass::BOOLEAN, which is handled as TRUE/FALSE type - the sbxToUnoValue clamps the value to SbxValue::GetBool().

Which means that commit 11f9aa4fcb2ad0a5fada8561c7041e7f25a059fc broke all the 128..255 unsigned short/long values passed from Basic to UNO to become 1. Fun. (But I wonder how can we create such types in Basic - which could be the reason why it goes unnoticed for so long...)

So could we pass the 128+ Byte values in TypeClass::SHORT? Maybe. That would mean that *if* the UNO expects a byte, and gets such a short, it will throw an error (see that "up cast is allowed, downcast not")...
Comment 6 Mike Kaganski 2022-06-27 09:47:36 UTC
(In reply to Mike Kaganski from comment #5)
> Which means that commit 11f9aa4fcb2ad0a5fada8561c7041e7f25a059fc broke all
> the 128..255 unsigned short/long values passed from Basic to UNO to become
> 1. Fun. (But I wonder how can we create such types in Basic - which could be
> the reason why it goes unnoticed for so long...)

FTR: e.g., using this code:


sub uints
  fa = CreateUnoService("com.sun.star.sheet.FunctionAccess")
  dim r as new com.sun.star.util.DateTimeRange
'  r.StartNanoSeconds = 200
  r.StartSeconds = 200
'  u32 = r.StartNanoSeconds
  u16 = r.StartSeconds
  msgbox TypeName(u16) & " " & u16 & " " & fa.callFunction("DEC2HEX", Array(u16, 4))
end sub
Comment 7 Commit Notification 2022-06-28 13:16:20 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/120daca4198d8a4775bc5c7330a0c415213b7a0b

tdf#149447: use proper UNO types for marshalling unsigned Basic types

It will be available in 7.5.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.