Wrong double to integer conversions on ARM 32-bit #141
Description
Hi Przemek,
I've noticed a hbtest failure on Raspberry Pi in this unrelated message, where Chr( -10000000.0 ) returned Chr( 0 ) instead of the expected Chr( 128 ).
After investigating, I've found that the issue is caused by Harbour compiler and runtime code casting double values to unsigned integers, which hits undefined behaviour and on the ARM32/GCC platform is zeroes the value, while on other platforms it behaves in the Cl*pper compatible way. (ARM64 not tested)
The problem is described here in more detail: https://stackoverflow.com/questions/10541200/is-the-behaviour-of-casting-a-negative-double-to-unsigned-int-defined-in-the-c-s (As per the article, it also affects WinCE on ARM.)
The suggested solution is to cast to signed integer first. Either ( int ) ( unsigned int ) ( int ) dValue or simply ( int ) dValue seems to work on tested platforms (linux/arm/32-bit gcc, darwin/x86-64/clang, win/x86/mingw, win/x64/mingw)
One reason I'm unsure about the best possible patch, is that in the VM library, the correct code is explicitly guarded off for __GCC__, but even that doesn't seem to be completely consistent.
Those guards were originally added by you in this xHarbour commit and diff, and reached Harbour repo via ef0883b and 68c738b:
* xharbour/source/vm/extend.c * xharbour/source/vm/itemapi.c + added casting for GCC in double to long conversion - it's necessary when the double value is not in long what can be caused by storing ULONG value inside double
Here's my proposed patch, that I could confirm fixing the problem on ARM32:
diff --git a/src/common/expropt2.c b/src/common/expropt2.c
index e1dc778..94619b6 100644
--- a/src/common/expropt2.c
+++ b/src/common/expropt2.c
@@ -544,7 +544,7 @@ PHB_EXPR hb_compExprReduceMinus( PHB_EXPR pSelf, HB_COMP_DECL )
if( pRight->value.asNum.NumType == HB_ET_LONG )
pSelf->value.asDate.lDate = pLeft->value.asDate.lDate - ( long ) pRight->value.asNum.val.l;
else
- pSelf->value.asDate.lDate = pLeft->value.asDate.lDate - ( long ) ( unsigned long ) pRight->value.asNum.val.d;
+ pSelf->value.asDate.lDate = pLeft->value.asDate.lDate - ( long ) ( unsigned long ) ( long ) pRight->value.asNum.val.d;
pSelf->value.asDate.lTime = 0;
pSelf->ExprType = HB_ET_DATE;
pSelf->ValType = HB_EV_DATE;
@@ -747,7 +747,7 @@ PHB_EXPR hb_compExprReducePlus( PHB_EXPR pSelf, HB_COMP_DECL )
if( pLeft->value.asNum.NumType == HB_ET_LONG )
pSelf->value.asDate.lDate = pRight->value.asDate.lDate + ( long ) pLeft->value.asNum.val.l;
else
- pSelf->value.asDate.lDate = pRight->value.asDate.lDate + ( long ) ( unsigned long ) pLeft->value.asNum.val.d;
+ pSelf->value.asDate.lDate = pRight->value.asDate.lDate + ( long ) ( unsigned long ) ( long ) pLeft->value.asNum.val.d;
pSelf->value.asDate.lTime = 0;
pSelf->ExprType = HB_ET_DATE;
pSelf->ValType = HB_EV_DATE;
@@ -800,7 +800,7 @@ PHB_EXPR hb_compExprReducePlus( PHB_EXPR pSelf, HB_COMP_DECL )
if( pRight->value.asNum.NumType == HB_ET_LONG )
pSelf->value.asDate.lDate = pLeft->value.asDate.lDate + ( long ) pRight->value.asNum.val.l;
else
- pSelf->value.asDate.lDate = pLeft->value.asDate.lDate + ( long ) ( unsigned long ) pRight->value.asNum.val.d;
+ pSelf->value.asDate.lDate = pLeft->value.asDate.lDate + ( long ) ( unsigned long ) ( long ) pRight->value.asNum.val.d;
pSelf->value.asDate.lTime = 0;
pSelf->ExprType = HB_ET_DATE;
pSelf->ValType = HB_EV_DATE;
@@ -2073,7 +2073,7 @@ HB_BOOL hb_compExprReduceCHR( PHB_EXPR pSelf, HB_COMP_DECL )
}
else
{
- pExpr->value.asString.string = ( char * ) HB_UNCONST( hb_szAscii[ ( unsigned int ) pArg->value.asNum.val.d & 0xff ] );
+ pExpr->value.asString.string = ( char * ) HB_UNCONST( hb_szAscii[ ( unsigned int ) ( int ) pArg->value.asNum.val.d & 0xff ] );
pExpr->value.asString.dealloc = HB_FALSE;
pExpr->nLength = 1;
}
@@ -2101,7 +2101,7 @@ HB_BOOL hb_compExprReduceBCHAR( PHB_EXPR pSelf, HB_COMP_DECL )
pExpr->value.asString.string =
( char * ) HB_UNCONST( hb_szAscii[ ( pArg->value.asNum.NumType == HB_ET_LONG ?
( unsigned int ) pArg->value.asNum.val.l :
- ( unsigned int ) pArg->value.asNum.val.d ) & 0xff ] );
+ ( unsigned int ) ( int ) pArg->value.asNum.val.d ) & 0xff ] );
pExpr->value.asString.dealloc = HB_FALSE;
pExpr->nLength = 1;
diff --git a/src/vm/extend.c b/src/vm/extend.c
index baa586f..1bc38fe 100644
--- a/src/vm/extend.c
+++ b/src/vm/extend.c
@@ -489,7 +489,7 @@ int hb_parni( int iParam )
else if( HB_IS_LONG( pItem ) )
return ( int ) pItem->item.asLong.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( int ) ( unsigned int ) pItem->item.asDouble.value;
#else
return ( int ) pItem->item.asDouble.value;
@@ -517,7 +517,7 @@ int hb_parnidef( int iParam, int iDefValue )
else if( HB_IS_LONG( pItem ) )
return ( int ) pItem->item.asLong.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( int ) ( unsigned int ) pItem->item.asDouble.value;
#else
return ( int ) pItem->item.asDouble.value;
@@ -545,7 +545,7 @@ long hb_parnl( int iParam )
else if( HB_IS_INTEGER( pItem ) )
return ( long ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( long ) ( unsigned long ) pItem->item.asDouble.value;
#else
return ( long ) pItem->item.asDouble.value;
@@ -573,7 +573,7 @@ long hb_parnldef( int iParam, long lDefValue )
else if( HB_IS_INTEGER( pItem ) )
return ( long ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( long ) ( unsigned long ) pItem->item.asDouble.value;
#else
return ( long ) pItem->item.asDouble.value;
@@ -601,7 +601,7 @@ HB_ISIZ hb_parns( int iParam )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_ISIZ ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_ISIZ ) ( HB_SIZE ) pItem->item.asDouble.value;
#else
return ( HB_ISIZ ) pItem->item.asDouble.value;
@@ -629,7 +629,7 @@ HB_ISIZ hb_parnsdef( int iParam, HB_ISIZ nDefValue )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_ISIZ ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_ISIZ ) ( HB_SIZE ) pItem->item.asDouble.value;
#else
return ( HB_ISIZ ) pItem->item.asDouble.value;
@@ -658,7 +658,7 @@ HB_LONGLONG hb_parnll( int iParam )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_LONGLONG ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_LONGLONG ) ( HB_ULONGLONG ) pItem->item.asDouble.value;
#else
return ( HB_LONGLONG ) pItem->item.asDouble.value;
@@ -687,7 +687,7 @@ HB_MAXINT hb_parnint( int iParam )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_MAXINT ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_MAXINT ) ( HB_MAXUINT ) pItem->item.asDouble.value;
#else
return ( HB_MAXINT ) pItem->item.asDouble.value;
@@ -715,7 +715,7 @@ HB_MAXINT hb_parnintdef( int iParam, HB_MAXINT nDefValue )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_MAXINT ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_MAXINT ) ( HB_MAXUINT ) pItem->item.asDouble.value;
#else
return ( HB_MAXINT ) pItem->item.asDouble.value;
@@ -1161,7 +1161,7 @@ int hb_parvni( int iParam, ... )
else if( HB_IS_LONG( pItem ) )
return ( int ) pItem->item.asLong.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( int ) ( unsigned int ) pItem->item.asDouble.value;
#else
return ( int ) pItem->item.asDouble.value;
@@ -1200,7 +1200,7 @@ long hb_parvnl( int iParam, ... )
else if( HB_IS_INTEGER( pItem ) )
return ( long ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( long ) ( unsigned long ) pItem->item.asDouble.value;
#else
return ( long ) pItem->item.asDouble.value;
@@ -1278,7 +1278,7 @@ HB_LONGLONG hb_parvnll( int iParam, ... )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_LONGLONG ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_LONGLONG ) ( HB_ULONGLONG ) pItem->item.asDouble.value;
#else
return ( HB_LONGLONG ) pItem->item.asDouble.value;
@@ -1318,7 +1318,7 @@ HB_MAXINT hb_parvnint( int iParam, ... )
else if( HB_IS_INTEGER( pItem ) )
return ( HB_MAXINT ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_MAXINT ) ( HB_MAXUINT ) pItem->item.asDouble.value;
#else
return ( HB_MAXINT ) pItem->item.asDouble.value;
diff --git a/src/vm/itemapi.c b/src/vm/itemapi.c
index 18149fe..46e058f 100644
--- a/src/vm/itemapi.c
+++ b/src/vm/itemapi.c
@@ -627,7 +627,7 @@ int hb_itemGetNI( PHB_ITEM pItem )
return ( int ) pItem->item.asLong.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( int ) ( unsigned int ) pItem->item.asDouble.value;
#else
return ( int ) pItem->item.asDouble.value;
@@ -650,7 +650,7 @@ long hb_itemGetNL( PHB_ITEM pItem )
return ( long ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( long ) ( HB_ULONG ) pItem->item.asDouble.value;
#else
return ( long ) pItem->item.asDouble.value;
@@ -673,7 +673,7 @@ HB_ISIZ hb_itemGetNS( PHB_ITEM pItem )
return ( HB_ISIZ ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_ISIZ ) ( HB_SIZE ) pItem->item.asDouble.value;
#else
return ( HB_ISIZ ) pItem->item.asDouble.value;
@@ -696,7 +696,7 @@ HB_MAXINT hb_itemGetNInt( PHB_ITEM pItem )
return ( HB_MAXINT ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_MAXINT ) ( HB_MAXUINT ) pItem->item.asDouble.value;
#else
return ( HB_MAXINT ) pItem->item.asDouble.value;
@@ -720,7 +720,7 @@ HB_LONGLONG hb_itemGetNLL( PHB_ITEM pItem )
return ( HB_LONGLONG ) pItem->item.asInteger.value;
else if( HB_IS_DOUBLE( pItem ) )
-#if defined( __GNUC__ )
+#if defined( __GNUC__ ) && !( defined( HB_CPU_ARM ) || defined( HB_CPU_ARM_64 ) )
return ( HB_LONGLONG ) ( HB_ULONGLONG ) pItem->item.asDouble.value;
#else
return ( HB_LONGLONG ) pItem->item.asDouble.value;
@@ -1316,7 +1316,7 @@ PHB_ITEM hb_itemPutNumType( PHB_ITEM pItem, double dNumber, int iDec, int iType1
else if( HB_DBL_LIM_LONG( dNumber ) )
{
#ifdef HB_LONG_LONG_OFF
- return hb_itemPutNL( pItem, ( long ) ( unsigned long ) dNumber );
+ return hb_itemPutNL( pItem, ( long ) ( unsigned long ) ( long ) dNumber );
#else
return hb_itemPutNLL( pItem, ( HB_LONGLONG ) dNumber );
#endifWhat do you think would be the best fix for this?
Harbour test code:
local d := -10000000.0
local i := -10000000
hb_cdpSelect( "cp437" )
? Asc( Chr( -10000000 ) ) // OK
? Asc( Chr( -10000000.0 ) ) // WRONG ON ARM32/GCC
? Asc( Chr( i ) ) // OK
? Asc( Chr( d ) ) // WRONG ON ARM32/GCC
? Asc( hb_BChar( -10000000 ) ) // OK
? Asc( hb_BChar( -10000000.0 ) ) // WRONG ON ARM32/GCC
? Asc( hb_BChar( i ) ) // OK
? Asc( hb_BChar( d ) ) // WRONG ON ARM32/GCC
? 0d20170310 - -10000000 // OK
? 0d20170310 - -10000000.0 // WRONG ON ARM32/GCC
? 0d20170310 + -10000000 // OK
? 0d20170310 + -10000000.0 // WRONG ON ARM32/GCC
? -10000000 + 0d20170310 // OK
? -10000000.0 + 0d20170310 // WRONG ON ARM32/GCC
? 0d20170310 - i // OK
? 0d20170310 - d // WRONG ON ARM32/GCC
? 0d20170310 + i // OK
? 0d20170310 + d // WRONG ON ARM32/GCC
? i + 0d20170310 // OK
? d + 0d20170310 // WRONG ON ARM32/GCCC test code:
#include <stdio.h>
#include <string.h>
static const char * i2bi( int x )
{
static char b[ 64 ];
unsigned int z;
b[ 0 ] = '\0';
for( z = 0x80000000; z > 0; z >>= 1 )
strcat( b, ( ( x & z ) == z ) ? "1" : "0" );
return b;
}
static const char * i2bl( long x )
{
static char b[ 64 ];
unsigned long z;
b[ 0 ] = '\0';
for( z = 0x80000000; z > 0; z >>= 1 )
strcat( b, ( ( x & z ) == z ) ? "1" : "0" );
return b;
}
int main( void )
{
double d = -10000000.0;
printf("%s\n", i2bi( ( int ) d ) ); /* OK */
printf("%s\n", i2bi( ( int ) ( unsigned int ) d ) ); /* ZERO on ARM32/GCC */
printf("%s\n", i2bi( ( unsigned int ) d ) ); /* ZERO on ARM32/GCC */
printf("%s\n", i2bi( ( int ) ( unsigned int ) ( int ) d ) ); /* OK */
printf("%s\n", i2bl( ( long ) d ) ); /* OK */
printf("%s\n", i2bl( ( long ) ( unsigned long ) d ) ); /* ZERO on ARM32/GCC */
printf("%s\n", i2bl( ( unsigned long ) d ) ); /* ZERO on ARM32/GCC */
printf("%s\n", i2bl( ( long ) ( unsigned long ) ( long ) d ) ); /* OK */
return 0;
}