[Vm-dev] Still a problem with inlining not assigning the return value

Nicolas Cellier nicolas.cellier.aka.nice at gmail.com
Wed Jul 27 10:16:09 UTC 2016


Hi,
I think I already reported this, but we still have a problem.

For example, in spursrc/vm/gcc-cointerp.c, see sendInvokeCallbackContext
defined around line 18464 in HEAD rev.

clang reports this with -Wall:

../../spursrc/vm/gcc3x-cointerp.c:18550:7: warning: variable 'object' is
used uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
                if ((((unsigned int) value)) <= (MaxSmallInteger)) {
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../spursrc/vm/gcc3x-cointerp.c:18592:54: note: uninitialized use occurs
here
                longAtput((sp = GIV(stackPointer) - BytesPerWord), object);
                                                                   ^~~~~~
../../platforms/Cross/vm/sqMemoryAccess.h:190:67: note: expanded from macro
'longAtput'
# define longAtput(oop,val)
longAtPointerput(atPointerArg(oop), val)

^
../../platforms/Cross/vm/sqMemoryAccess.h:171:62: note: expanded from macro
'longAtPointerput'
# define longAtPointerput(ptr,val)      (*(sqInt *)(ptr)= (sqInt)(val))
                                                                  ^
../../spursrc/vm/gcc3x-cointerp.c:18550:3: note: remove the 'if' if its
condition is always false
                if ((((unsigned int) value)) <= (MaxSmallInteger)) {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../spursrc/vm/gcc3x-cointerp.c:18489:17: note: initialize the variable
'object' to silence this warning
    sqInt object;
                ^
                 = 0

If we look from line 18547:
        /* begin positive32BitIntegerFor: */
        /* begin maybeInlinePositive32BitIntegerFor: */
        assert(!((hasSixtyFourBitImmediates())));
        if ((((unsigned int) value)) <= (MaxSmallInteger)) {
            ((value << 1) | 1);
            goto l3;
        }
then on label l3 (line 18589):
    l3:    /* end maybeInlinePositive32BitIntegerFor: */;


        longAtput((sp = GIV(stackPointer) - BytesPerWord), object);

Ouch! clang is right.
Obviously line 18551 should be
            *object=*((value << 1) | 1);
But the inliner lost it somewhere...

We can also see this with -Wunused-value
I'm not so much for stifling these warnings, some are true positives :(

Unfortunately, I cannot report this in github because those generated files
and diffs are too big...
With command line we at least know when the line was last changed:
git blame d85a19df65 spursrc/vm/gcc3x-cointerp.c

e46c1a15 (Eliot Miranda 2015-02-14 02:19:04 +0000
18551)                        ((value << 1) | 1);

But I'm not sure if it was an inliner change or just a
sendInvokeCallbackContextchange, we would have to cross check with VMMaker
history (that's where the initial Pharo strategy mixing both could shine).
Maybe it has not much value to track regression, because inliner never was
really robust.

Anyway, we can't live with such hanging sword of Damocles.
The best thing would be to unit-test code generation. A huge low rewarding
job, but we shall do it.

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


More information about the Vm-dev mailing list