[Vm-dev] Issues with the clock on V3
Nicolas Cellier
nicolas.cellier.aka.nice at gmail.com
Sat Sep 3 14:54:32 UTC 2016
I've corrected the C code generation in VMMaker.oscog-nice.1933.
There's still one thing I do not understand: why the hell was the debug
version working???
squeak.stack.v3 nicolas$ lldb CocoaDebug.app/Contents/MacOS/Squeak
(lldb) disassemble -n fetchLong64ofObject
Squeak`fetchLong64ofObject:
Squeak[0x21ce0] <+0>: pushl %ebp
Squeak[0x21ce1] <+1>: movl %esp, %ebp
Squeak[0x21ce3] <+3>: subl $0x18, %esp
Squeak[0x21ce6] <+6>: movl 0xc(%ebp), %eax
Squeak[0x21ce9] <+9>: movl 0x8(%ebp), %ecx
Squeak[0x21cec] <+12>: movl %ecx, -0x4(%ebp)
Squeak[0x21cef] <+15>: movl %eax, -0x8(%ebp)
Squeak[0x21cf2] <+18>: movl -0x4(%ebp), %ecx
Squeak[0x21cf5] <+21>: leal 0x4(%eax,%ecx,8), %eax
Squeak[0x21cf9] <+25>: movl %esp, %ecx
Squeak[0x21cfb] <+27>: movl %eax, (%ecx)
Squeak[0x21cfd] <+29>: calll 0x202f0 ; intAt at
sqMemoryAccess.h:154
Squeak[0x21d02] <+34>: movl -0x8(%ebp), %ecx
Squeak[0x21d05] <+37>: movl -0x4(%ebp), %edx
Squeak[0x21d08] <+40>: leal 0x8(%ecx,%edx,8), %ecx
Squeak[0x21d0c] <+44>: movl %esp, %edx
Squeak[0x21d0e] <+46>: movl %ecx, (%edx)
Squeak[0x21d10] <+48>: movl %eax, -0xc(%ebp)
Squeak[0x21d13] <+51>: calll 0x202f0 ; intAt at
sqMemoryAccess.h:154
Squeak[0x21d18] <+56>: xorl %edx, %edx
Squeak[0x21d1a] <+58>: movl %eax, -0x10(%ebp)
Squeak[0x21d1d] <+61>: movl %ecx, %eax
Squeak[0x21d1f] <+63>: addl $0x18, %esp
Squeak[0x21d22] <+66>: popl %ebp
Squeak[0x21d23] <+67>: retl
My understanding is that the two 32-bits words of return value that are
returned in eax after call to intAt are then pushed on the stack:
But the 64bit return value should be in registers $edx and $eax
And $edx is set to zero before returning:
Squeak[0x21d18] <+56>: xorl %edx, %edx
Ahh, but now that I inspect the results more closely, I see that the debug
version does not work, it just returns a different value from fast version.
2016-09-03 11:02 GMT+02:00 Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com>:
> I'm going to show how I try to solve such problem.
> It's going to be a bit long, sorry, but there might be something didactic
> in it since you ask for a clue...
>
> Starting with Integer Sunit test, I detect that one example of strange
> behavior is:
> (1 << 63) negated + 1.
> It answers a smallInteger (1).
> It uses primitive 21, that is primitiveAddLargeIntegers
>
> This one does not print correctly, but produces a correct result:
> 1 + (1 << 63) negated.
> It uses primitiveDigitAdd from LargeIntegers, so at least, we have one
> path working...
>
> primitiveAddLargeIntegers tries using native 64bits arithmetic before
> engaging LargeIntegers module digit-based arithmetic.
> It has already been modified long time ago to avoid undefined behavior
> related to checking overflow in post condition, so the slang code looks
> correct at upper level. Anyway, since both integers in our failing case
> have different sign we should not pass thru the branch checking for
> overflow.
> It might be a problem of UB with used methods or with inlining and type
> inference.
>
> So one has to focus on generated C code of primitiveAddLargeIntegers.
> it's not a problem with inlining, because no method are inlined here, and
> the C code looks equally good to slang.
>
> So it remains to check isNegativeIntegerValueOf magnitude64BitValueOf and
> magnitude64BitIntegerForneg.
> There is some dead code generated for checking class of oop, for example:
> ok = ClassLargeNegativeIntegerCompactIndex == ccIndex;
> goto l1;
>
> ok = classOop == (fetchPointerofObject(ccIndex - 1,
> longAt((GIV(specialObjectsOop) + BaseHeaderSize) + (((int)((usqInt)(CompactClasses)
> << (shiftForWord())))))));
> l1: /* end isClassOfNonImm:equalTo:compactClassIndex: */;
> The line after goto l1 is unreacheable, but that should not be the
> problem, it' juts because we exploited the compact class index.
>
> One line is worth checking in magnitude64BitValueOf:
> value = SQ_SWAP_8_BYTES_IF_BIGENDIAN((fetchLong64ofObject(0, oop)));
>
> Then inspecting code of fetchLong64ofObject we see an awfull thing:
> return
> (((unsigned int) (long32At((oop + BaseHeaderSize) +
> (((sqInt)((usqInt)(longIndex) << 3))))))) + (((sqInt)((usqInt)((((unsigned
> long long)(long32At((oop + BaseHeaderSize) + ((((sqInt)((usqInt)(longIndex)
> << 3))) + 4)))))) << 32)))
>
> That's the problem. We are converting to unsigned long long (64) before
> shifting by 32 bit, that's correct, but just before the shift, we convert
> back to usqInt (32 bits long) and thus get completely undefined behaviour!
>
> Let's see the warnings in generated LOGF compilation report:
> ../../stacksrc/vm/gcc3x-interp.c:27429:226: warning: shift count >= width
> of type [-Wshift-count-overflow] (((unsigned int)
> (long32At((oop + BaseHeaderSize) + (((sqInt)((usqInt)(longIndex) <<
> 3))))))) + (((sqInt)((usqInt)((((unsigned long long)(long32At((oop +
> BaseHeaderSize) + ((((sqInt)((usqInt)(longIndex) << 3))) + 4)))))) << 32)))
>
> Problem confirmed (Phew, I've deciphered correctly the parentheses and
> type cast precedence).
> Let's see the slang code of fetchLong64: longIndex ofObject: oop
> <returnTypeC: #sqLong>
> ^self cppIf: BytesPerWord = 8
> ifTrue: [self long64At: oop + self baseHeaderSize + (longIndex <<
> 3)]
> ifFalse:
> ["BEWARE OF SIGN EXTENSION ON LEAST SIGNIFICAND LIMB
> 'unsigned int) CAST IS THERE TO PREVENT SUCH ERROR"
> self cppIf: VMBIGENDIAN
> ifTrue: [((self long32At: oop + self baseHeaderSize +
> (longIndex << 3)) asUnsignedLongLong << 32)
> + (self cCoerceSimple: (self long32At: oop + self
> baseHeaderSize + (longIndex << 3 + 4)) to: #'unsigned int')]
> ifFalse: [(self cCoerceSimple: (self long32At: oop + self
> baseHeaderSize + (longIndex << 3)) to: #'unsigned int')
> + ((self long32At: oop + self baseHeaderSize +
> (longIndex << 3 + 4)) asUnsignedLongLong << 32)]]
>
> Till there, that sounds correct. So there's a problem in generating the
> shift << 32 with undue usqInt conversion.
> This generation takes place in CCodeGenerator generateShiftLeft: msgNode
> on: aStream indent: level.
> I've added a protection to avoid undefined behavior: I first convert to
> unsigned before shifting.
> But the type is elready unsigned long long, so that would mean that it's
> the type inference of asUnsignedLongLong that failed.
>
> asUnsignedLongLong is generated directly via CCodeGenerator>>
> generateAsUnsignedLongLong:on:indent: (see initializeCTranslationDictiona
> ry)
> This mechanism does bypass type inference rules, so there should be a
> corresponding type inference rule in CCodeGenerator>>
> returnTypeForSend:in:ifNil:
> ALAS, there is none!!!
>
> So my change of << generation for avoiding one problem with UB (a real
> problem: that prevents compiling win64 version correctly) did break another
> part.
>
> This shows how brittle the C Code generation is.
> Now that we use type inference rules, EVERY selector generated thru
> initializeCTranslationDictionary should have a type rule counterpart in
> returnTypeForSend:in:ifNil:
>
> The problem does not appear in SPUR, because memory is eight byte aligned,
> so we do not have to decompose the fetch into 2 32-bits words.
> I carried this analysis in LLP64_v2 branch, but I think it's the same in
> cog.
> I'll commit a patch.
>
> Thanks for reporting!
>
>
>
> 2016-09-03 9:57 GMT+02:00 Nicolas Cellier <nicolas.cellier.aka.nice@
> gmail.com>:
>
>> Interestingly, when I recompile on MacOSX, the bug happens on fast
>> version of squeak.stack.v3 but not on debug version!!!
>> That means that it's vain to track the problem with VM simulator.
>> It's also vain to try debugging at C level...
>>
>> What remains is:
>> Use lldb the unstripped fast version, put a break point in some
>> arithmetic primitive then stepi into assembly.
>> Or carefully inspect the compiler warnings (with ./mvm -- WARNINGS='-Wall
>> -Wextra') and carefully read generated C code to track for a possible
>> undefined behavior striking.
>> Or compare generated C code with generated C assembly to understand
>> what's going on.
>> Or instrument the generated code with undefined behavior guards (I don't
>> remember the clang incantation for that, but it's possible).
>>
>> 2016-09-02 22:09 GMT+02:00 Nicolas Cellier <nicolas.cellier.aka.nice at gmai
>> l.com>:
>>
>>> Hi Laura,
>>> it sounds like some LargeIntegers arithmetic is broken in v3...
>>> The clock symptoms is just a side effect.
>>>
>>> 2016-09-02 21:23 GMT+02:00 Laura Perez Cerrato <
>>> lauraperezcerrato at gmail.com>:
>>>
>>>>
>>>> Hello everyone,
>>>>
>>>> Lately, while working on the VM, I noticed that a few primitives
>>>> related with the clock on V3 were returning incorrect values. Cloning the
>>>> repository and building a Squeak Cog V3 VM produces a VM in which
>>>> DateAndTime class>>#now answers a value closer than a day to the epoch in
>>>> both Squeak 4.5 and the lastest version of Cuis. In the lastest version of
>>>> Cuis, Time class>>#primLocalMicrosecondClock, Time
>>>> class>>#primLocalSecondClock and Time class>>#primHighResClock return odd
>>>> values. This problem is seen exclusively on V3 VMs as far as I can tell,
>>>> independently of the interpreter.
>>>>
>>>> I wasn't capable of pinning down the reason for this strange behaviour.
>>>> If anyone could give me a hand with that, I'd appreaciate it.
>>>>
>>>> Cheers!
>>>>
>>>> -Laura Perez Cerrato
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20160903/dab878c5/attachment-0001.htm
More information about the Vm-dev
mailing list