Merge lp:~epics-core/epics-base/fix-calc-bit-manipulation into lp:~epics-core/epics-base/3.14
- fix-calc-bit-manipulation
- Merge into 3.14
Status: | Merged |
---|---|
Approved by: | Andrew Johnson |
Approved revision: | 12615 |
Merged at revision: | 12611 |
Proposed branch: | lp:~epics-core/epics-base/fix-calc-bit-manipulation |
Merge into: | lp:~epics-core/epics-base/3.14 |
Diff against target: |
371 lines (+141/-53) 3 files modified
src/libCom/calc/calcPerform.c (+35/-22) src/libCom/calc/postfix.c (+15/-12) src/libCom/test/epicsCalcTest.cpp (+91/-19) |
To merge this branch: | bzr merge lp:~epics-core/epics-base/fix-calc-bit-manipulation |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ralph Lange | Approve | ||
Andrew Johnson | Approve | ||
Review via email: mp+286515@code.launchpad.net |
Commit message
Description of the change
Fix for bug lp:1514520
- Pulls a backport of a 3.15 change to the calc engine, setting the internal integer format to the size-fixed epicsInt32 type.
- Adds tests that show the behavior described in the bug report.
- Applies the original patch by Dirk from that bug report.
To be able to apply this to 3.14, I had to backport the change to the sized-fixed integers. For sanity.
When applying to 3.15, that first commit has to be omitted.
One thing that I am not sure of: left shifts (zeroes filled in) are handled different than right shifts (ones filled in, assuming a signed integer operation).
The documentation (AppDevGuide) leaves it open.
- 12614. By Andrew Johnson
-
Fixed conversion overflows in tests
Minor tidying-up, added comments about casting for bitwise operations.
Ralph Lange (ralph-lange) wrote : | # |
> Added an epicsUInt32 utop variable to calcPerform() to reduce the number of
> casts, and added some code comments.
Looks good, thanks.
> Previous versions have always handled shifts as signed quantities, so the sign
> bit is extended on right-shifts.
Fine with me. I was just seeing the asymmetry.
> The shift operators also need to have their shift count limited to avoid
> Undefined Behavior; I see 2 realistic possibilities: ANDing the shift count
> with 31; or return 0 / -1 on shift-overflow [...]
> I have implemented the first option for now.
Fine with me. We don't have to fix the holes in the definition.
> The large positive decimal values in your tests are causing the same overflows
> when converted from double to integer that Dirk originally reported, they
> always convert to 0x80000000 on my RHEL systems so some tests fail.
Interesting. Is that a compiler or a size dependency? My Debian 64bit gcc5 machine was testing them fine.
Thanks a lot!
Andrew Johnson (anj) wrote : | # |
> Is that a compiler or a size dependency? My Debian 64bit gcc5 machine was testing them fine.
Don't know, I was testing on RHEL 6.7 with both 64 and 32-bit builds. Let me boot my Ubuntu VM to see what that does...
No difference; I'm doing manual checks on an IOC with a single calc record loaded.
This is with an unfixed 3.14 IOC, 64-bit:
tux% caget -lx anj:calc.CALC anj:calc
anj:calc.CALC a:=2863311530.0; a >> 8
anj:calc 0xAAAAAA
This is a 3.15 IOC, 64-bit:
tux% caget -lx anj:calc.CALC anj:calc
anj:calc.CALC a:=2863311530.0; a >> 8
anj:calc 0xFFFFFFFFFF800000
Then with this branch, 64-bit:
tux% caget -lx anj:calc.CALC anj:calc
anj:calc.CALC a:=2863311530.0; a >> 8
anj:calc 0xFFFFFFFFFFAAAAAA
The difference between 3.14 and 3.15 is because the bit-wise expressions were changed from long to epicsInt32, which fixed the shift to make them sign-extend properly, but show the broken value conversions. The final result above is correct (the sign extension on the results above are being done by caget -lx on 64-bit). I can get the right result from 3.15 using the negative value instead:
tux% caget -lx anj:calc.CALC anj:calc
anj:calc.CALC a:=-1431655766.1; a >> 8
anj:calc 0xFFFFFFFFFFAAAAAA
I think I got a bit confused when I was working on this last week; the negative value is evidently not (and maybe shouldn't be) necessary with the proper fix, so maybe I didn't need to change those tests. However I have just rewritten the double tests to check that we do convert the arguments properly for every bit-wise operator instead of duplicating the earlier integer tests:
// converting double values (add 0.1 to force as double)
// 0xaaaaaaaa = -1431655766 or 2863311530u
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
testUInt32C
Hmm, I just copied the resulting epicsCalcTest.cpp file into an unmodified 3.14 branch, where it happily...
Ralph Lange (ralph-lange) wrote : | # |
That's a nice one, eh?! When working on it last week, I got knots in my brain.
First of all, I set a narrow scope: I decided that I wanted to fix the calc engine, not the calc(out) record. To achieve that, I based the main tests on setting fields to double values before doing the operation, as this gets as close as possible to what the calc record does.
The other tests (using literals and setting fields to integers) were added later when I saw different failure patterns between them and the main tests.
On plain 3.14, the operations completely depend on sizeof(long). On 64bit things work fine (as setting bit31 is *not* setting the sign bit), on 32bit things fail.
To get a more reproducible situation, I first backported the change in the calc engine that changes all operations to work on fixed 32bit size types. With that change, I had the tests consistently failing in a pattern that matched Dirk's report.
After applying Dirk's patch, all tests passed. That's the state I pushed to LaunchPad.
I never tested in a real IOC using Dirk's database.
Having a test producing the same failure pattern that Dirk saw using his database, and his fix passing all my tests and (as per Dirk's report) his database testing seemed good enough to me.
Ralph Lange (ralph-lange) wrote : | # |
Note that your caget based tests add another conversion (CA server converting from the calc record's double VAL field to integer).
Too many chained conversions in one test, for my taste.
- 12615. By Andrew Johnson
-
Test each bitwise cast individually for overflow
Andrew Johnson (anj) wrote : | # |
Ok, so I wasn't testing against 3.14 on 32-bit, my bad — the tests do indeed fail there, thanks for pointing that out.
I have committed my changes to the double value tests that I posted yesterday, they are designed to check each cast in the bit-wise operator implementation individually for not overflowing with either +ve or -ve double values that have bit 31 set.
I agree that my caget technique introduced another conversion, I was trying to allow for that in my understanding of what's actually going on. Your testUInt32Calc() routine does the same conversion though:
uresult = (epicsUInt32) result;
We could avoid that completely by doing the test comparisons inside the expression being tested:
- testUInt32Calc(
+ testCalc(
However the returned result is less useful for diagnosing overflow problems, so I'm happy to keep what we have now.
If you're happy with this I will merge it.
Ralph Lange (ralph-lange) wrote : | # |
I am happy with it!
Preview Diff
1 | === modified file 'src/libCom/calc/calcPerform.c' |
2 | --- src/libCom/calc/calcPerform.c 2010-10-07 19:16:24 +0000 |
3 | +++ src/libCom/calc/calcPerform.c 2016-02-23 18:42:17 +0000 |
4 | @@ -21,6 +21,7 @@ |
5 | #include "osiUnistd.h" |
6 | #include "dbDefs.h" |
7 | #include "epicsMath.h" |
8 | +#include "epicsTypes.h" |
9 | #include "errlog.h" |
10 | #include "postfix.h" |
11 | #include "postfixPvt.h" |
12 | @@ -43,7 +44,8 @@ |
13 | double stack[CALCPERFORM_STACK+1]; /* zero'th entry not used */ |
14 | double *ptop; /* stack pointer */ |
15 | double top; /* value from top of stack */ |
16 | - int itop; /* integer from top of stack */ |
17 | + epicsInt32 itop; /* integer from top of stack */ |
18 | + epicsUInt32 utop; /* unsigned integer from top of stack */ |
19 | int op; |
20 | int nargs; |
21 | |
22 | @@ -55,14 +57,14 @@ |
23 | switch (op){ |
24 | |
25 | case LITERAL_DOUBLE: |
26 | - memcpy((void *)++ptop, pinst, sizeof(double)); |
27 | + memcpy(++ptop, pinst, sizeof(double)); |
28 | pinst += sizeof(double); |
29 | break; |
30 | |
31 | case LITERAL_INT: |
32 | - memcpy(&itop, pinst, sizeof(int)); |
33 | + memcpy(&itop, pinst, sizeof(epicsInt32)); |
34 | *++ptop = itop; |
35 | - pinst += sizeof(int); |
36 | + pinst += sizeof(epicsInt32); |
37 | break; |
38 | |
39 | case FETCH_VAL: |
40 | @@ -136,11 +138,11 @@ |
41 | break; |
42 | |
43 | case MODULO: |
44 | - itop = (long) *ptop--; |
45 | + itop = (epicsInt32) *ptop--; |
46 | if (itop) |
47 | - *ptop = (long) *ptop % itop; |
48 | + *ptop = (epicsInt32) *ptop % itop; |
49 | else |
50 | - *ptop = epicsNAN; /* NaN */ |
51 | + *ptop = epicsNAN; |
52 | break; |
53 | |
54 | case POWER: |
55 | @@ -261,7 +263,7 @@ |
56 | |
57 | case NINT: |
58 | top = *ptop; |
59 | - *ptop = (double)(long)(top >= 0 ? top + 0.5 : top - 0.5); |
60 | + *ptop = (epicsInt32) (top >= 0 ? top + 0.5 : top - 0.5); |
61 | break; |
62 | |
63 | case RANDOM: |
64 | @@ -282,34 +284,45 @@ |
65 | *ptop = ! *ptop; |
66 | break; |
67 | |
68 | + /* For bitwise operations on values with bit 31 set, double values |
69 | + * must first be cast to unsigned to correctly set that bit; the |
70 | + * double value must be negative in that case. The result must be |
71 | + * cast to a signed integer before converting to the double result. |
72 | + */ |
73 | + |
74 | case BIT_OR: |
75 | - itop = (long) *ptop--; |
76 | - *ptop = (long) *ptop | itop; |
77 | + utop = *ptop--; |
78 | + *ptop = (epicsInt32) ((epicsUInt32) *ptop | utop); |
79 | break; |
80 | |
81 | case BIT_AND: |
82 | - itop = (long) *ptop--; |
83 | - *ptop = (long) *ptop & itop; |
84 | + utop = *ptop--; |
85 | + *ptop = (epicsInt32) ((epicsUInt32) *ptop & utop); |
86 | break; |
87 | |
88 | case BIT_EXCL_OR: |
89 | - itop = (long) *ptop--; |
90 | - *ptop = (long) *ptop ^ itop; |
91 | + utop = *ptop--; |
92 | + *ptop = (epicsInt32) ((epicsUInt32) *ptop ^ utop); |
93 | break; |
94 | |
95 | case BIT_NOT: |
96 | - itop = (long) *ptop; |
97 | - *ptop = ~itop; |
98 | + utop = *ptop; |
99 | + *ptop = (epicsInt32) ~utop; |
100 | break; |
101 | |
102 | + /* The shift operators use signed integers, so a right-shift will |
103 | + * extend the sign bit into the left-hand end of the value. The |
104 | + * double-casting through unsigned here is important, see above. |
105 | + */ |
106 | + |
107 | case RIGHT_SHIFT: |
108 | - itop = (long) *ptop--; |
109 | - *ptop = (long) *ptop >> itop; |
110 | + utop = *ptop--; |
111 | + *ptop = ((epicsInt32) (epicsUInt32) *ptop) >> (utop & 31); |
112 | break; |
113 | |
114 | case LEFT_SHIFT: |
115 | - itop = (long) *ptop--; |
116 | - *ptop = (long) *ptop << itop; |
117 | + utop = *ptop--; |
118 | + *ptop = ((epicsInt32) (epicsUInt32) *ptop) << (utop & 31); |
119 | break; |
120 | |
121 | case NOT_EQ: |
122 | @@ -381,7 +394,7 @@ |
123 | pinst += sizeof(double); |
124 | break; |
125 | case LITERAL_INT: |
126 | - pinst += sizeof(int); |
127 | + pinst += sizeof(epicsInt32); |
128 | break; |
129 | case MIN: |
130 | case MAX: |
131 | @@ -468,7 +481,7 @@ |
132 | pinst += sizeof(double); |
133 | break; |
134 | case LITERAL_INT: |
135 | - pinst += sizeof(int); |
136 | + pinst += sizeof(epicsInt32); |
137 | break; |
138 | case MIN: |
139 | case MAX: |
140 | |
141 | === modified file 'src/libCom/calc/postfix.c' |
142 | --- src/libCom/calc/postfix.c 2013-11-19 21:26:22 +0000 |
143 | +++ src/libCom/calc/postfix.c 2016-02-23 18:42:17 +0000 |
144 | @@ -22,6 +22,7 @@ |
145 | #include "dbDefs.h" |
146 | #include "epicsStdlib.h" |
147 | #include "epicsString.h" |
148 | +#include "epicsTypes.h" |
149 | #include "postfix.h" |
150 | #include "postfixPvt.h" |
151 | #include "shareLib.h" |
152 | @@ -216,7 +217,7 @@ |
153 | char * const pdest = pout; |
154 | char *pnext; |
155 | double lit_d; |
156 | - int lit_i; |
157 | + epicsInt32 lit_i; |
158 | |
159 | if (psrc == NULL || *psrc == '\0' || |
160 | pout == NULL || perror == NULL) { |
161 | @@ -249,27 +250,29 @@ |
162 | goto bad; |
163 | } |
164 | psrc = pnext; |
165 | - lit_i = (int) lit_d; |
166 | + lit_i = (epicsInt32) lit_d; |
167 | if (lit_d != (double) lit_i) { |
168 | *pout++ = pel->code; |
169 | - memcpy(pout, (void *)&lit_d, sizeof(double)); |
170 | + memcpy(pout, &lit_d, sizeof(double)); |
171 | pout += sizeof(double); |
172 | } else { |
173 | *pout++ = LITERAL_INT; |
174 | - memcpy(pout, (void *)&lit_i, sizeof(int)); |
175 | - pout += sizeof(int); |
176 | + memcpy(pout, &lit_i, sizeof(epicsInt32)); |
177 | + pout += sizeof(epicsInt32); |
178 | } |
179 | } |
180 | else { |
181 | - lit_i = strtoul(psrc, &pnext, 0); |
182 | + epicsUInt32 lit_ui; |
183 | + |
184 | + lit_ui = (epicsUInt32) strtoul(psrc, &pnext, 0); |
185 | if (pnext == psrc) { |
186 | *perror = CALC_ERR_BAD_LITERAL; |
187 | goto bad; |
188 | } |
189 | psrc = pnext; |
190 | *pout++ = LITERAL_INT; |
191 | - memcpy(pout, (void *)&lit_i, sizeof(int)); |
192 | - pout += sizeof(int); |
193 | + memcpy(pout, &lit_ui, sizeof(epicsUInt32)); |
194 | + pout += sizeof(epicsUInt32); |
195 | } |
196 | |
197 | operand_needed = FALSE; |
198 | @@ -594,18 +597,18 @@ |
199 | }; |
200 | char op; |
201 | double lit_d; |
202 | - int lit_i; |
203 | + epicsInt32 lit_i; |
204 | |
205 | while ((op = *pinst) != END_EXPRESSION) { |
206 | switch (op) { |
207 | case LITERAL_DOUBLE: |
208 | - memcpy((void *)&lit_d, ++pinst, sizeof(double)); |
209 | + memcpy(&lit_d, ++pinst, sizeof(double)); |
210 | printf("\tDouble %g\n", lit_d); |
211 | pinst += sizeof(double); |
212 | break; |
213 | case LITERAL_INT: |
214 | - memcpy((void *)&lit_i, ++pinst, sizeof(int)); |
215 | - printf("\tInteger %d\n", lit_i); |
216 | + memcpy(&lit_i, ++pinst, sizeof(epicsInt32)); |
217 | + printf("\tInteger %d (0x%x)\n", lit_i, lit_i); |
218 | pinst += sizeof(int); |
219 | break; |
220 | case MIN: |
221 | |
222 | === modified file 'src/libCom/test/epicsCalcTest.cpp' |
223 | --- src/libCom/test/epicsCalcTest.cpp 2015-02-17 22:21:13 +0000 |
224 | +++ src/libCom/test/epicsCalcTest.cpp 2016-02-23 18:42:17 +0000 |
225 | @@ -8,6 +8,7 @@ |
226 | // Author: Andrew Johnson |
227 | |
228 | #include "epicsUnitTest.h" |
229 | +#include "epicsTypes.h" |
230 | #include "epicsMath.h" |
231 | #include "epicsAlgorithm.h" |
232 | #include "postfix.h" |
233 | @@ -38,32 +39,59 @@ |
234 | /* Evaluate expression, test against expected result */ |
235 | bool pass = false; |
236 | double args[CALCPERFORM_NARGS] = { |
237 | - 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0 |
238 | + 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0 |
239 | }; |
240 | char rpn[MAX_POSTFIX_SIZE]; |
241 | short err; |
242 | double result = 0.0; |
243 | result /= result; /* Start as NaN */ |
244 | - |
245 | + |
246 | if (postfix(expr, rpn, &err)) { |
247 | - testDiag("postfix: %s in expression '%s'", calcErrorStr(err), expr); |
248 | + testDiag("postfix: %s in expression '%s'", calcErrorStr(err), expr); |
249 | } else |
250 | - if (calcPerform(args, &result, rpn) && finite(result)) { |
251 | - testDiag("calcPerform: error evaluating '%s'", expr); |
252 | - } |
253 | - |
254 | + if (calcPerform(args, &result, rpn) && finite(result)) { |
255 | + testDiag("calcPerform: error evaluating '%s'", expr); |
256 | + } |
257 | + |
258 | if (finite(expected) && finite(result)) { |
259 | - pass = fabs(expected - result) < 1e-8; |
260 | + pass = fabs(expected - result) < 1e-8; |
261 | } else if (isnan(expected)) { |
262 | - pass = (bool) isnan(result); |
263 | + pass = (bool) isnan(result); |
264 | } else { |
265 | - pass = (result == expected); |
266 | - } |
267 | - if (!testOk(pass, "%s", expr)) { |
268 | - testDiag("Expected result is %g, actually got %g", expected, result); |
269 | - calcExprDump(rpn); |
270 | - } |
271 | - return; |
272 | + pass = (result == expected); |
273 | + } |
274 | + if (!testOk(pass, "%s", expr)) { |
275 | + testDiag("Expected result is %g, actually got %g", expected, result); |
276 | + calcExprDump(rpn); |
277 | + } |
278 | +} |
279 | + |
280 | +void testUInt32Calc(const char *expr, epicsUInt32 expected) { |
281 | + /* Evaluate expression, test against expected result */ |
282 | + bool pass = false; |
283 | + double args[CALCPERFORM_NARGS] = { |
284 | + 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0 |
285 | + }; |
286 | + char rpn[MAX_POSTFIX_SIZE]; |
287 | + short err; |
288 | + epicsUInt32 uresult; |
289 | + double result = 0.0; |
290 | + result /= result; /* Start as NaN */ |
291 | + |
292 | + if (postfix(expr, rpn, &err)) { |
293 | + testDiag("postfix: %s in expression '%s'", calcErrorStr(err), expr); |
294 | + } else |
295 | + if (calcPerform(args, &result, rpn) && finite(result)) { |
296 | + testDiag("calcPerform: error evaluating '%s'", expr); |
297 | + } |
298 | + |
299 | + uresult = (epicsUInt32) result; |
300 | + pass = (uresult == expected); |
301 | + if (!testOk(pass, "%s", expr)) { |
302 | + testDiag("Expected result is 0x%x (%u), actually got 0x%x (%u)", |
303 | + expected, expected, uresult, uresult); |
304 | + calcExprDump(rpn); |
305 | + } |
306 | } |
307 | |
308 | void testArgs(const char *expr, unsigned long einp, unsigned long eout) { |
309 | @@ -238,8 +266,8 @@ |
310 | const double a=1.0, b=2.0, c=3.0, d=4.0, e=5.0, f=6.0, |
311 | g=7.0, h=8.0, i=9.0, j=10.0, k=11.0, l=12.0; |
312 | |
313 | - testPlan(577); |
314 | - |
315 | + testPlan(613); |
316 | + |
317 | /* LITERAL_OPERAND elements */ |
318 | testExpr(0); |
319 | testExpr(1); |
320 | @@ -883,7 +911,51 @@ |
321 | testBadExpr("1?", CALC_ERR_CONDITIONAL); |
322 | testBadExpr("1?1", CALC_ERR_CONDITIONAL); |
323 | testBadExpr(":1", CALC_ERR_SYNTAX); |
324 | - |
325 | + |
326 | + // Bit manipulations wrt bit 31 (bug lp:1514520) |
327 | + // using integer literals |
328 | + testUInt32Calc("0xaaaaaaaa AND 0xffff0000", 0xaaaa0000u); |
329 | + testUInt32Calc("0xaaaaaaaa OR 0xffff0000", 0xffffaaaau); |
330 | + testUInt32Calc("0xaaaaaaaa XOR 0xffff0000", 0x5555aaaau); |
331 | + testUInt32Calc("~0xaaaaaaaa", 0x55555555u); |
332 | + testUInt32Calc("~~0xaaaaaaaa", 0xaaaaaaaau); |
333 | + testUInt32Calc("0xaaaaaaaa >> 8", 0xffaaaaaau); |
334 | + testUInt32Calc("0xaaaaaaaa << 8", 0xaaaaaa00u); |
335 | + // using integer literals assigned to variables |
336 | + testUInt32Calc("a:=0xaaaaaaaa; b:=0xffff0000; a AND b", 0xaaaa0000u); |
337 | + testUInt32Calc("a:=0xaaaaaaaa; b:=0xffff0000; a OR b", 0xffffaaaau); |
338 | + testUInt32Calc("a:=0xaaaaaaaa; b:=0xffff0000; a XOR b", 0x5555aaaau); |
339 | + testUInt32Calc("a:=0xaaaaaaaa; ~a", 0x55555555u); |
340 | + testUInt32Calc("a:=0xaaaaaaaa; ~~a", 0xaaaaaaaau); |
341 | + testUInt32Calc("a:=0xaaaaaaaa; a >> 8", 0xffaaaaaau); |
342 | + testUInt32Calc("a:=0xaaaaaaaa; a << 8", 0xaaaaaa00u); |
343 | + |
344 | + // Test proper conversion of double values (+ 0.1 enforces double literal) |
345 | + // when used as inputs to the bitwise operations. |
346 | + // 0xaaaaaaaa = -1431655766 or 2863311530u |
347 | + testUInt32Calc("-1431655766.1 OR 0", 0xaaaaaaaau); |
348 | + testUInt32Calc("2863311530.1 OR 0", 0xaaaaaaaau); |
349 | + testUInt32Calc("0 OR -1431655766.1", 0xaaaaaaaau); |
350 | + testUInt32Calc("0 OR 2863311530.1", 0xaaaaaaaau); |
351 | + testUInt32Calc("-1431655766.1 XOR 0", 0xaaaaaaaau); |
352 | + testUInt32Calc("2863311530.1 XOR 0", 0xaaaaaaaau); |
353 | + testUInt32Calc("0 XOR -1431655766.1", 0xaaaaaaaau); |
354 | + testUInt32Calc("0 XOR 2863311530.1", 0xaaaaaaaau); |
355 | + testUInt32Calc("-1431655766.1 AND 0xffffffff", 0xaaaaaaaau); |
356 | + testUInt32Calc("2863311530.1 AND 0xffffffff", 0xaaaaaaaau); |
357 | + testUInt32Calc("0xffffffff AND -1431655766.1", 0xaaaaaaaau); |
358 | + testUInt32Calc("0xffffffff AND 2863311530.1", 0xaaaaaaaau); |
359 | + testUInt32Calc("~ -1431655766.1", 0x55555555u); |
360 | + testUInt32Calc("~ 2863311530.1", 0x55555555u); |
361 | + testUInt32Calc("-1431655766.1 >> 0", 0xaaaaaaaau); |
362 | + testUInt32Calc("2863311530.1 >> 0", 0xaaaaaaaau); |
363 | + testUInt32Calc("-1431655766.1 >> 0.1", 0xaaaaaaaau); |
364 | + testUInt32Calc("2863311530.1 >> 0.1", 0xaaaaaaaau); |
365 | + testUInt32Calc("-1431655766.1 << 0", 0xaaaaaaaau); |
366 | + testUInt32Calc("2863311530.1 << 0", 0xaaaaaaaau); |
367 | + testUInt32Calc("-1431655766.1 << 0.1", 0xaaaaaaaau); |
368 | + testUInt32Calc("2863311530.1 << 0.1", 0xaaaaaaaau); |
369 | + |
370 | return testDone(); |
371 | } |
372 |
Added an epicsUInt32 utop variable to calcPerform() to reduce the number of casts, and added some code comments.
Previous versions have always handled shifts as signed quantities, so the sign bit is extended on right-shifts. I changed the left-shift code to match, although it doesn't actually make any difference.
The shift operators also need to have their shift count limited to avoid Undefined Behavior; I see 2 realistic possibilities: ANDing the shift count with 31; or return 0 on shift-overflow but when right-shifting a negative quantity return -1 on shift-overflow. The first is the fastest and simplest to code, but the second one might be regarded as be more logical. Any preferences or alternative suggestions? I have implemented the first option for now.
The large positive decimal values in your tests are causing the same overflows when converted from double to integer that Dirk originally reported, they always convert to 0x80000000 on my RHEL systems so some tests fail. The paragraph before Dirk's "The fix is:" line explains that you have to use negative values for bit 31 to be set, which my tests agree with. The negative versions that work are 0xaaaaaaaa = -1431655766, 0xffff0000 = -65536. Note that postfix() compiles double literals into a LITERAL_INT op-code if the value can be safely held in an epicsInt32, so adding ".0" has no effect; but using ".1" instead does work, the fraction gets truncated.
Also fixed some minor white-space issues.