<div dir="ltr"><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div>Hi,<br></div>I already reported a few days ago a problem about type inference leading to highly suspect code in generated BitBltPlugin.<br><br></div>I can still show the same examples in latest commit, and not only for BitBlt, also in ZipPlugin (harmless according to my analysis) and cointerp...<br><br>Here is a small extract of the LOGD report for squeak.cog.spur<br><br>/Users/nicolas/Smalltalk/Squeak/vm_cog/build.macos32x86/squeak.cog.spur/../../spursrc/vm/gcc3x-cointerp.c:18984:31:{18984:8-18984:30}{18984:34-18984:35}: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare,3]<br> && ((value ^ (value << 1)) >= 0)) {<br><br></div>We recognize the #isIntegerValue: pattern.<br></div>But what's wrong with it?<br></div>Well, type of value has been inferred as #'unsigned long'<br><br></div>It's because #positiveMachineIntegerFor: has its parameter declared as #'unsigned long' which is a good thing. Indeed, I'm pretty sure that we would not try to re-interpret a negative
as positive, and that >= 0 is in pre-condition of all senders (maybe
I just changed one for the 32Bit variant).<br><br>All this was from sendInvokeCallbackContext and the
parameters are something like the stack pointer. I don't know if
possible, but if for some reason the stackp is in high memory address, we gonna have some surprise.<br><br></div><div>Though, if we know that the parameter is positive, we should not enter into the #isIntegerValue: bit trick. Instead we would just write a simple test on high bound.<br><br>positive32BitIntegerFor: integerValue<br> "N.B. will *not* cause a GC."<br> <br> | newLargeInteger |<br> <var: 'integerValue' type: #'unsigned int'><br> integerValue <= self maxSmallInteger<br> ifTrue: [^ objectMemory integerObjectOf: integerValue].<br></div><div>snip...<br></div><br><div>I've done that in my 32 bit LargeInteger digits
branch for a very long time, but now, maybe it's time to backport that
in the cog branch.<br><br></div></div><div><div>______________________<br></div><br></div><div>While at it, note that the #isIntegerValue: bit trick is technically undefined behavior in case intValue is a signed int.<br></div><div>If intValue is negative, then shifting a negative left is UB.<br></div><div>If intValue is positive, then shifting more than can be represented in signed int is UB (that is in case of overflow).<br></div><br></div><div>So a very good compiler assuming we don't rely on UB could discard the case of negative intValue, then discard the case of overflow for positive.<br></div><div>Thus both intValue and (intValue << 1) can be assumed positive.<br></div><div>And a bitXor of two positive is positive, so the test >=0 can be eliminated alltogether also in case of signed int because allways true...<br></div><div>Currently, we are lucky because no one in llvm nor gcc folks noticed the bitXor property probably, I just hope they don't read this list ;)<br></div><div>But we are not going to be lucky forever.<br></div><div>In this particular case, we should better write:<br></div><div><br>(intValue asUnsignedInteger bitXor: (intValue asUnsignedInteger << 1)) asInteger >= 0<br><br></div>By doing so, we eliminate the undefined behavior and this code works for both intValue < minSmallInteger and intValue > maxSmallInteger, and is most probably as fast as the current one. Just safer.<br><br></div><div>Technically, Behavior is still implementation defined, because we use a conversion from unsigned -> signed which does not fit into a positive signed. But it will work on machines that use 2-complements which give us enough portability ;)<br><br></div>The method would also not work if we would supply a longer int (like a long long) and the inliner will be incorrect in both cases:<br></div>- if it let the bit trick be written as is with the long long<br></div>- if it forces an assignment to a shorter int.<br><br></div><div>In case of a longer type or a signedness mismatch, the inliner should not try to inline anything.<br></div><div>It should fallback to the slower but correct test, intValue >= minSmallInteger and intValue <= maxSmallInteger (just the later for unsigned).<br></div><br></div>Right now, it's caveat emptor, but it's way too dangerous!!!<br><br></div><div>Nicolas<br></div><div><div><div><div><div><div><div><div><br></div></div></div></div></div></div></div></div></div>