EPICS Controls Argonne National Laboratory

Experimental Physics and
Industrial Control System

2002  2003  2004  2005  2006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  <20202021  2022  2023  2024  Index 2002  2003  2004  2005  2006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  <20202021  2022  2023  2024 
<== Date ==> <== Thread ==>

Subject: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15
From: Freddie Akeroyd via Core-talk <core-talk at aps.anl.gov>
To: mp+379191 at code.launchpad.net
Date: Fri, 14 Feb 2020 01:31:22 -0000
Freddie Akeroyd has proposed merging ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15.

Commit message:

Fix bit operations failures on VS2019 32bit

Working with Dirk Zimoch @dirk.zimoch, fixed various issues
with bit operations on VS2019 32bit. These seem to relate to
handling bit 31 of a 32 bit number.

As EPICS << is an arithmetic bit shift, following Java we
have added <<< and >>> operators for logical shifts

Though it is on a different architecture, this looks like
a similar issue to LP: #1838792



Requested reviews:
  EPICS Core Developers (epics-core)
Related bugs:
  Bug #1838792 in EPICS Base: "epicsCalc bit-wise operators on aarch64"
  https://bugs.launchpad.net/epics-base/+bug/1838792

For more details, see:
https://code.launchpad.net/~freddie-akeroyd/epics-base/+git/epics-base/+merge/379191
-- 
Your team EPICS Core Developers is requested to review the proposed merge of ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15.
diff --git a/src/libCom/calc/calcPerform.c b/src/libCom/calc/calcPerform.c
index c0f4aeb..1d675cf 100644
--- a/src/libCom/calc/calcPerform.c
+++ b/src/libCom/calc/calcPerform.c
@@ -33,7 +33,7 @@ static int cond_search(const char **ppinst, int match);
 #endif
 
 /* Turn off global optimization for 64-bit MSVC builds */
-#if defined(_WIN32) && defined(_M_X64) && !defined(_MINGW)
+#if 0 && defined(_WIN32) && defined(_M_X64) && !defined(_MINGW)
 #  pragma optimize("g", off)
 #endif
 
@@ -48,7 +48,6 @@ epicsShareFunc long
     double *ptop;			/* stack pointer */
     double top; 			/* value from top of stack */
     epicsInt32 itop;			/* integer from top of stack */
-    epicsUInt32 utop;			/* unsigned integer from top of stack */
     int op;
     int nargs;
 
@@ -287,30 +286,37 @@ epicsShareFunc long
 	    *ptop = ! *ptop;
 	    break;
 
-        /* For bitwise operations on values with bit 31 set, double values
-         * must first be cast to unsigned to correctly set that bit; the
-         * double value must be negative in that case. The result must be
-         * cast to a signed integer before converting to the double result.
+        /* Be VERY careful converting double to int in case bit 31 is set!
+         * Out-of-range errors give very different results on different sytems.
+         * Convert negative doubles to signed and positive doubles to unsigned
+         * first to avoid overflows if bit 32 is set.
+         * The result is always signed, values with bit 31 set are negative
+         * to avoid problems when writing the value to signed integer fields
+         * like longout.VAL or ao.RVAL. However unsigned fields may give
+         * problems on some architectures. (Fewer than giving problems with
+         * signed integer. Maybe the conversion functions should handle
+         * overflows better.)
          */
+        #define d2i(x) ((x)<0?(epicsInt32)(x):(epicsInt32)(epicsUInt32)(x))
+        #define d2ui(x) ((x)<0?(epicsUInt32)(epicsInt32)(x):(epicsUInt32)(x))
 
 	case BIT_OR:
-	    utop = *ptop--;
-	    *ptop = (epicsInt32) ((epicsUInt32) *ptop | utop);
+	    top = *ptop--;
+	    *ptop = (double)(d2i(*ptop) | d2i(top));
 	    break;
 
 	case BIT_AND:
-	    utop = *ptop--;
-	    *ptop = (epicsInt32) ((epicsUInt32) *ptop & utop);
+	    top = *ptop--;
+	    *ptop = (double)(d2i(*ptop) & d2i(top));
 	    break;
 
 	case BIT_EXCL_OR:
-	    utop = *ptop--;
-	    *ptop = (epicsInt32) ((epicsUInt32) *ptop ^ utop);
+	    top = *ptop--;
+	    *ptop = (double)(d2i(*ptop) ^ d2i(top));
 	    break;
 
 	case BIT_NOT:
-	    utop = *ptop;
-	    *ptop = (epicsInt32) ~utop;
+	    *ptop = (double)~d2i(*ptop);
 	    break;
 
         /* The shift operators use signed integers, so a right-shift will
@@ -318,14 +324,24 @@ epicsShareFunc long
          * double-casting through unsigned here is important, see above.
          */
 
-	case RIGHT_SHIFT:
-	    utop = *ptop--;
-	    *ptop = ((epicsInt32) (epicsUInt32) *ptop) >> (utop & 31);
+	case RIGHT_SHIFT_ARITH:
+	    top = *ptop--;
+	    *ptop = (double)(d2i(*ptop) >> (d2i(top) & 31));
 	    break;
 
-	case LEFT_SHIFT:
-	    utop = *ptop--;
-	    *ptop = ((epicsInt32) (epicsUInt32) *ptop) << (utop & 31);
+	case LEFT_SHIFT_ARITH:
+	    top = *ptop--;
+	    *ptop = (double)(d2i(*ptop) << (d2i(top) & 31));
+	    break;
+
+	case RIGHT_SHIFT_LOGIC:
+	    top = *ptop--;
+	    *ptop = (double)(d2ui(*ptop) >> (d2ui(top) & 31u));
+	    break;
+
+	case LEFT_SHIFT_LOGIC:
+	    top = *ptop--;
+	    *ptop = (double)(d2ui(*ptop) << (d2ui(top) & 31u));
 	    break;
 
 	case NOT_EQ:
@@ -382,11 +398,12 @@ epicsShareFunc long
     *presult = *ptop;
     return 0;
 }
-#if defined(_WIN32) && defined(_M_X64) && !defined(_MINGW)
+#if 0 && defined(_WIN32) && defined(_M_X64) && !defined(_MINGW)
 #  pragma optimize("", on)
 #endif
 
-
+
+
 epicsShareFunc long
 calcArgUsage(const char *pinst, unsigned long *pinputs, unsigned long *pstores)
 {
diff --git a/src/libCom/calc/postfix.c b/src/libCom/calc/postfix.c
index 463ceea..cf54eb8 100644
--- a/src/libCom/calc/postfix.c
+++ b/src/libCom/calc/postfix.c
@@ -148,13 +148,15 @@ static const ELEMENT operators[] = {
 {":=",		0, 0,	-1,	STORE_OPERATOR, STORE_A},
 {";",		0, 0,	0,	EXPR_TERMINATOR,NOT_GENERATED},
 {"<",		3, 3,	-1,	BINARY_OPERATOR,LESS_THAN},
-{"<<",		2, 2,	-1,	BINARY_OPERATOR,LEFT_SHIFT},
+{"<<",		2, 2,	-1,	BINARY_OPERATOR,LEFT_SHIFT_ARITH},
+{"<<<",		2, 2,	-1,	BINARY_OPERATOR,LEFT_SHIFT_LOGIC},
 {"<=",		3, 3,	-1,	BINARY_OPERATOR,LESS_OR_EQ},
 {"=",		3, 3,	-1,	BINARY_OPERATOR,EQUAL},
 {"==",		3, 3,	-1,	BINARY_OPERATOR,EQUAL},
 {">",		3, 3,	-1,	BINARY_OPERATOR,GR_THAN},
 {">=",		3, 3,	-1,	BINARY_OPERATOR,GR_OR_EQ},
-{">>",		2, 2,	-1,	BINARY_OPERATOR,RIGHT_SHIFT},
+{">>",		2, 2,	-1,	BINARY_OPERATOR,RIGHT_SHIFT_ARITH},
+{">>>",		2, 2,	-1,	BINARY_OPERATOR,RIGHT_SHIFT_LOGIC},
 {"?",		0, 0,	-1,	CONDITIONAL,	COND_IF},
 {"AND",		2, 2,	-1,	BINARY_OPERATOR,BIT_AND},
 {"OR",		1, 1,	-1,	BINARY_OPERATOR,BIT_OR},
@@ -579,8 +581,10 @@ epicsShareFunc void
 	"BIT_AND",
 	"BIT_EXCL_OR",
 	"BIT_NOT",
-	"RIGHT_SHIFT",
-	"LEFT_SHIFT",
+	"RIGHT_SHIFT_ARITH",
+	"RIGHT_SHIFT_LOGIC",
+	"LEFT_SHIFT_ARITH",
+	"LEFT_SHIFT_LOGIC",
     /* Relationals */
 	"NOT_EQ",
 	"LESS_THAN",
diff --git a/src/libCom/calc/postfixPvt.h b/src/libCom/calc/postfixPvt.h
index 53efb32..5b2ba0a 100644
--- a/src/libCom/calc/postfixPvt.h
+++ b/src/libCom/calc/postfixPvt.h
@@ -84,8 +84,10 @@ typedef enum {
 	BIT_AND,
 	BIT_EXCL_OR,
 	BIT_NOT,
-	RIGHT_SHIFT,
-	LEFT_SHIFT,
+	RIGHT_SHIFT_ARITH,
+	RIGHT_SHIFT_LOGIC,
+	LEFT_SHIFT_ARITH,
+	LEFT_SHIFT_LOGIC,
     /* Relationals */
 	NOT_EQ,
 	LESS_THAN,
diff --git a/src/libCom/test/epicsCalcTest.cpp b/src/libCom/test/epicsCalcTest.cpp
index 2492c95..a0131cd 100644
--- a/src/libCom/test/epicsCalcTest.cpp
+++ b/src/libCom/test/epicsCalcTest.cpp
@@ -104,7 +104,7 @@ void testUInt32Calc(const char *expr, epicsUInt32 expected) {
             testDiag("calcPerform: error evaluating '%s'", expr);
         }
 
-    uresult = (epicsUInt32) result;
+    uresult = (result < 0.0 ? (epicsUInt32)(epicsInt32)result : (epicsUInt32)result);
     pass = (uresult == expected);
     if (!testOk(pass, "%s", expr)) {
         testDiag("Expected result is 0x%x (%u), actually got 0x%x (%u)",
@@ -297,7 +297,7 @@ MAIN(epicsCalcTest)
     const double a=1.0, b=2.0, c=3.0, d=4.0, e=5.0, f=6.0,
 		 g=7.0, h=8.0, i=9.0, j=10.0, k=11.0, l=12.0;
     
-    testPlan(613);
+    testPlan(643);
 
     /* LITERAL_OPERAND elements */
     testExpr(0);
@@ -688,7 +688,9 @@ MAIN(epicsCalcTest)
     testExpr(NaN < NaN);
     
     testExpr(1 << 2);
-    testExpr(1 << 3 << 2)
+    testCalc("1 <<< 2", 1u << 2u);
+    testExpr(1 << 3 << 2);
+    testCalc("1 <<< 3 <<< 2", 1u << 3u << 2u);
     
     testExpr(0 <= 1);
     testExpr(0 <= 0);
@@ -776,7 +778,9 @@ MAIN(epicsCalcTest)
     testExpr(NaN >= NaN);
     
     testExpr(8 >> 1);
+    testCalc("8 >>> 1", 8u >> 1u);
     testExpr(64 >> 2 >> 1);
+    testCalc("64 >>> 2 >>> 1", 64u >> 2u >> 1u);
     
     testExpr(7 AND 4);
     
@@ -873,13 +877,19 @@ MAIN(epicsCalcTest)
     testExpr(2 | 4 / 2);			// 1 5
     testCalc("1 | 2 ** 3", 1 | (int) pow(2., 3.));// 1 6
     testExpr(3 << 2 & 10);			// 2 2
+    testCalc("3 <<< 2 & 10", 3u << 2u & 10u);			// 2 2
     testCalc("18 & 6 << 2", (18 & 6) << 2);	// 2 2
+    testCalc("18 & 6 <<< 2", (18u & 6u) << 2u);	// 2 2
     testExpr(36 >> 2 & 10);			// 2 2
+    testCalc("36 >>> 2 & 10", 36u >> 2u & 10u);			// 2 2
     testCalc("18 & 20 >> 2", (18 & 20) >> 2);	// 2 2
+    testCalc("18 & 20 >>> 2", (18u & 20u) >> 2u);	// 2 2
     testExpr(3 & 4 == 4);			// 2 3
     testExpr(3 AND 4 == 4);			// 2 3
     testCalc("1 << 2 != 4", 1 << (2 != 4));	// 2 3
+    testCalc("1 <<< 2 != 4", 1u << (2u != 4u));	// 2 3
     testCalc("16 >> 2 != 4", 16 >> (2 != 4));	// 2 3
+    testCalc("16 >>> 2 != 4", 16u >> (2u != 4u));	// 2 3
     testExpr(3 AND -2); 			// 2 8
     testExpr(0 < 1 ? 2 : 3);			// 3 0
     testExpr(1 <= 0 ? 2 : 3);			// 3 0
@@ -951,7 +961,13 @@ MAIN(epicsCalcTest)
     testUInt32Calc("~0xaaaaaaaa", 0x55555555u);
     testUInt32Calc("~~0xaaaaaaaa", 0xaaaaaaaau);
     testUInt32Calc("0xaaaaaaaa >> 8", 0xffaaaaaau);
+    testUInt32Calc("0x55555555 >> 8", 0x00555555u);
+    testUInt32Calc("0xaaaaaaaa >>> 8", 0x00aaaaaau);
+    testUInt32Calc("0x55555555 >>> 8", 0x00555555u);
     testUInt32Calc("0xaaaaaaaa << 8", 0xaaaaaa00u);
+    testUInt32Calc("0x55555555 << 8", 0x55555500u);
+    testUInt32Calc("0xaaaaaaaa <<< 8", 0xaaaaaa00u);
+    testUInt32Calc("0x55555555 <<< 8", 0x55555500u);
     //   using integer literals assigned to variables
     testUInt32Calc("a:=0xaaaaaaaa; b:=0xffff0000; a AND b", 0xaaaa0000u);
     testUInt32Calc("a:=0xaaaaaaaa; b:=0xffff0000; a OR b", 0xffffaaaau);
@@ -959,7 +975,13 @@ MAIN(epicsCalcTest)
     testUInt32Calc("a:=0xaaaaaaaa; ~a", 0x55555555u);
     testUInt32Calc("a:=0xaaaaaaaa; ~~a", 0xaaaaaaaau);
     testUInt32Calc("a:=0xaaaaaaaa; a >> 8", 0xffaaaaaau);
+    testUInt32Calc("a:=0xaaaaaaaa; a >>> 8", 0x00aaaaaau);
     testUInt32Calc("a:=0xaaaaaaaa; a << 8", 0xaaaaaa00u);
+    testUInt32Calc("a:=0xaaaaaaaa; a <<< 8", 0xaaaaaa00u);
+    testUInt32Calc("a:=0x55555555; a >> 8", 0x00555555u);
+    testUInt32Calc("a:=0x55555555; a >>> 8", 0x00555555u);
+    testUInt32Calc("a:=0x55555555; a << 8", 0x55555500u);
+    testUInt32Calc("a:=0x55555555; a <<< 8", 0x55555500u);
 
     // Test proper conversion of double values (+ 0.1 enforces double literal)
     // when used as inputs to the bitwise operations.
@@ -979,14 +1001,21 @@ MAIN(epicsCalcTest)
     testUInt32Calc("~ -1431655766.1", 0x55555555u);
     testUInt32Calc("~ 2863311530.1", 0x55555555u);
     testUInt32Calc("-1431655766.1 >> 0", 0xaaaaaaaau);
+    testUInt32Calc("-1431655766.1 >>> 0", 0xaaaaaaaau);
     testUInt32Calc("2863311530.1 >> 0", 0xaaaaaaaau);
+    testUInt32Calc("2863311530.1 >>> 0", 0xaaaaaaaau);
     testUInt32Calc("-1431655766.1 >> 0.1", 0xaaaaaaaau);
+    testUInt32Calc("-1431655766.1 >>> 0.1", 0xaaaaaaaau);
     testUInt32Calc("2863311530.1 >> 0.1", 0xaaaaaaaau);
+    testUInt32Calc("2863311530.1 >>> 0.1", 0xaaaaaaaau);
     testUInt32Calc("-1431655766.1 << 0", 0xaaaaaaaau);
+    testUInt32Calc("-1431655766.1 <<< 0", 0xaaaaaaaau);
     testUInt32Calc("2863311530.1 << 0", 0xaaaaaaaau);
+    testUInt32Calc("2863311530.1 <<< 0", 0xaaaaaaaau);
     testUInt32Calc("-1431655766.1 << 0.1", 0xaaaaaaaau);
+    testUInt32Calc("-1431655766.1 <<< 0.1", 0xaaaaaaaau);
     testUInt32Calc("2863311530.1 << 0.1", 0xaaaaaaaau);
+    testUInt32Calc("2863311530.1 <<< 0.1", 0xaaaaaaaau);
 
     return testDone();
 }
-

Replies:
Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 mdavidsaver via Core-talk
Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 mdavidsaver via Core-talk
Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 Martin Konrad via Core-talk
Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 Freddie Akeroyd via Core-talk
Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 Freddie Akeroyd via Core-talk
Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 Freddie Akeroyd via Core-talk
[Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 noreply--- via Core-talk

Navigate by Date:
Prev: [Bug 1838792] Re: epicsCalc bit-wise operators on aarch64 Launchpad Bug Tracker via Core-talk
Next: Build failed: EPICS Base base-7.0-536 AppVeyor via Core-talk
Index: 2002  2003  2004  2005  2006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  <20202021  2022  2023  2024 
Navigate by Thread:
Prev: [Bug 1838792] Re: epicsCalc bit-wise operators on aarch64 Andrew Johnson via Core-talk
Next: Re: [Merge] ~freddie-akeroyd/epics-base:bit_operations into epics-base:3.15 mdavidsaver via Core-talk
Index: 2002  2003  2004  2005  2006  2007  2008  2009  2010  2011  2012  2013  2014  2015  2016  2017  2018  2019  <20202021  2022  2023  2024 
ANJ, 28 May 2020 Valid HTML 4.01! · Home · News · About · Base · Modules · Extensions · Distributions · Download ·
· Search · EPICS V4 · IRMIS · Talk · Bugs · Documents · Links · Licensing ·