[Vm-dev] Still some pathological type inference cases visible in latest commit

[Nicolas Cellier]

Hi,
I already reported a few days ago a problem about type inference leading to
highly suspect code in generated BitBltPlugin.

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...

Here is a small extract of the LOGD report for squeak.cog.spur

/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]
                  && ((value ^ (value << 1)) >= 0)) {

We recognize the #isIntegerValue: pattern.
But what's wrong with it?
Well, type of value has been inferred as #'unsigned long'

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).

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.

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.

positive32BitIntegerFor: integerValue
    "N.B. will *not* cause a GC."

    | newLargeInteger |
    <var: 'integerValue' type: #'unsigned int'>
    integerValue <= self maxSmallInteger
        ifTrue: [^ objectMemory integerObjectOf: integerValue].
snip...

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.

______________________

While at it, note that the #isIntegerValue: bit trick is technically
undefined behavior in case intValue is a signed int.
If intValue is negative, then shifting a negative left is UB.
If intValue is positive, then shifting more than can be represented in
signed int is UB (that is in case of overflow).

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.
Thus both intValue and (intValue << 1) can be assumed positive.
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...
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 ;)
But we are not going to be lucky forever.
In this particular case, we should better write:

(intValue asUnsignedInteger bitXor: (intValue asUnsignedInteger << 1))
asInteger >= 0

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.

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 ;)

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:
- if it let the bit trick be written as is with the long long
- if it forces an assignment to a shorter int.

In case of a longer type or a signedness mismatch, the inliner should not
try to inline anything.
It should fallback to the slower but correct test, intValue >=
minSmallInteger and intValue <= maxSmallInteger (just the later for
unsigned).

Right now, it's caveat emptor, but it's way too dangerous!!!

Nicolas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20150326/c383bd2f/attachment.htm