<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2016-03-30 13:46 GMT+02:00 Ben Coman <span dir="ltr"><<a href="mailto:btc@openinworld.com" target="_blank">btc@openinworld.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
On Wed, Mar 30, 2016 at 2:09 PM, Eliot Miranda <<a href="mailto:eliot.miranda@gmail.com">eliot.miranda@gmail.com</a>> wrote:<br>
><br>
> Hi Nicolas,<br>
<div><div class="h5">><br>
> On Tue, Mar 29, 2016 at 2:36 PM, Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> 2016-03-27 15:39 GMT+02:00 Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>>:<br>
>>><br>
>>> To be more specific,<br>
>>><br>
>>> The first thing to do would be to confirm that the KernelNumber tests fail with a squeak.stack.v3 VM compiled with head revision of COG VM branch, whatever OS.<br>
>>><br>
>>> Then knowing from which SVN version of COG VM branch the KernelNumber the tests start failing would be nice.<br>
>>><br>
>>> The bisect job is to:<br>
>>> - iterate on version number (whatever strategy, bsearch or something)<br>
>>> - checkout VM sources<br>
>>> - compile the build.favouriteOS/squeak.stack.v3<br>
>>> - run a v3 image with the generated VM and launch KernelNumber tests<br>
>>><br>
>>> Really a job for a jenkins bot, travis bot or whatever...<br>
>>> The next good thing would be to give a little love to <a href="http://build.squeak.org" rel="noreferrer" target="_blank">build.squeak.org</a> or anyother similar solution.<br>
>>> I only see red disks on this site...<br>
>>><br>
>><br>
>> Follow up: I did bissect manually:<br>
>> 3648 (VMMaker.oscog-eem.nice.1729) OK<br>
>> 3649 (VMMaker.oscog-eem.1740) Not OK<br>
>><br>
>> So something went wrong for V3 between these two versions.<br>
>> At the same time, it works for spur.<br>
>><br>
>> Spur objects are 8 bytes aligned, v3 objects are 4 bytes aligned...<br>
>> So fetching 64 bits from V3 MUST be decomposed into 2 32 bits fetch to avoid a segfault related to misalign fetch.<br>
>><br>
>> OK, let's look how it is performed:<br>
>><br>
>> fetchLong64: longIndex ofObject: oop<br>
>> <returnTypeC: #sqLong><br>
>> ^self cppIf: BytesPerWord = 8<br>
>> ifTrue: [self long64At: oop + self baseHeaderSize + (longIndex << 3)]<br>
>> ifFalse:<br>
>> [self cppIf: VMBIGENDIAN<br>
>> ifTrue: [((self long32At: oop + self baseHeaderSize + (longIndex << 3)) asUnsignedLongLong << 32)<br>
>> + (self long32At: oop + self baseHeaderSize + (longIndex << 3 + 4))]<br>
>> ifFalse: [(self long32At: oop + self baseHeaderSize + (longIndex << 3))<br>
>> + ((self long32At: oop + self baseHeaderSize + (longIndex << 3 + 4)) asUnsignedLongLong << 32)]]<br>
>><br>
>> AH AH! Operation is:<br>
>> low + (((unsigned) high) << 32)<br>
>><br>
>> With low declared as SIGNED (long32At is signed GRRR).<br>
>> What if bit 32 of low is 1? then the compiler will perform:<br>
>><br>
>> (unsigned long) low + ...<br>
>><br>
>> AND THIS WILL DO A SIGN EXTENSION...<br>
>><br>
>> It's not that my code was false...<br>
>> It's just that it uncovered a bug in fetchLong64:ofObject:<br>
>><br>
>> That cost me many hours, but that enforce my conviction about signedness...<br>
>> I would much much much prefer to call unsignedLong32At because it's what I mean 9 times out of 10: get the magnitude...<br>
><br>
><br>
</div></div>> Let's add them. It would be great to have them all consistent too. But at least let's add unsignedLong32At:[put:]. Wait. In the simulator longAt:[put:] et al are unsigned. So we have three choices:<br>
><br>
> a) live with it<br>
> b) make longAt:[put:] long32At:put: et al unsigned in C, and add signedLongAt:[put:] et al.<br>
> c) make the simulator's longAt:[put:] long32At:put: signed and then add unsignedLongAt:[put:] et al<br>
><br>
> Any others?<br>
><br>
> I think a) is unwise. The simulator and C should agree. Nicolas, I can join your experience in finding that debugging these issues is extremely time consuming.<br>
><br>
> My preference is for b); some uses where the type should be signed will by located by C compiler warnings; we hope that VM tests will catch the others)<br>
<br>
Wouldn't (b) risk occasional cognitive dysfunction switching between<br>
the semantic of aa C long being signed and a Slang long being<br>
unsigned? (c) seems better long term, but switching the signedness of<br>
the an existing simulation method might cause short term instability<br>
and need to be tackled in one sitting? So a naive question... Why<br>
leave one implicit? Consider d) introducing both signedLongAt:[put:]<br>
and unsignedLongAt:[put:] and gradual migration.<br>
<br>
cheers -ben<br>
<div class=""><div class="h5"><br></div></div></blockquote><div><br><div>My first thought was as Eliot, use b) because<br>- the simulator is already b)<br></div><div>- wild guess is that there are more unsigned than signed usage<br><br>But I like Ben arguments better.<br>Non uniform naming has great costs<br>- slow learning ramp for noobs<br>- seed for future bug due to misunderstanding.<br><br>For example we already have<br>* long implicitely meaning 32 bits like in signedIntToLong signedIntFromLong,<br>* long unconditionnally meaning 64 bits like sqLong<br>* long meaning either 32 or 64 bits depending on architecture in generated C (like in StackInterperter>>putLong:toFile:)<br></div><div>* long meaning either 32 or 64 depending on the context<br> (#longAt: is allways 32 bits when sent to ByteArray Bitmap or CAccessor, but might be 64 bits when sent to simulator argh...)<br></div><div><br>I remember I had similar problems with word the first time I looked at VMMaker.<br>WordArray being allways 32 bits word, but wordSize being 4 or 8 depending on target architecture.<br>Even now, I'm not sure of which word we are talking of...<br><br></div><div>Also b) goes with a blitz strategy:<br></div><div>- do a massive semantic change long32 -> unsigned<br></div><div>- review the send sites and change a few variant to signed<br></div><div>- repair broken C code once image start crashing and tests start failing<br></div><div>- repair the simulator which might show different symptoms (see implementors of #asUnsignedLong)<br></div><div>either it works soon, or never...<br><br>I've noted only 47 senders of long32At: . <br><div>Plus the 33 senders of fetchLong32:ofObject:<br>And cannot analyze easily the possible side effects thru Slang type inference...<br><br></div><div>As my recent bug of LargeInteger division showed: abuse of unsigned lead to expensive bug tracking too.<br></div>That does a bit restrain my enthusiasm for solution b) - chat échaudé craint l'eau froide ;)<br><br></div><div>Last, I did not apply b) policy in my VM branch for 32 bits large int.<br></div><div>Instead I introduced the unsigned variant gradually... (coward !)</div><div><br><div>Before we attempt anything, I'd like to remind the short term goals:<br></div>- avoid uncontrolled sign extension when promoting to larger int in C<br><div>- avoid undefined behavior, most of which being related to signed arithmetic<br></div>- make the simulator better match C (or vice versa in the case)<br><br></div></div><div><div>The last goal is far away, there are subtle "differences"...<br></div><div>Like in the simulator, the difference of two unsigned might be < 0.<br></div><div>In C not, unless we force it (cast, type punning, whatever...).<br></div><br><div>Last point, gradual introduction and non-regression testing pre-suppose a stable VM.<br>I still experiment random crashes I'd like to
track first.<br> <br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">
><br>
>><br>
>>> 2016-03-27 0:40 GMT+01:00 Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>>:<br>
>>>><br>
>>>> Hi,<br>
>>>> before I continue, i've noticed that the large integer multiply seems broken on v3 object memory (cog & stack)<br>
>>>> Note that this does not happen on Spur.<br>
>>>><br>
>>>> This is independent of my recent changes of LargeIntegers plugin as it happens BEFORE these changes and is related to primitive 29 rather than to the plugin...<br>
>>>> Here are the symptoms:<br>
>>>><br>
>>>> halfPower := 10000.<br>
>>>> s := 111111111111111.<br>
>>>> head := s quo: halfPower.<br>
>>>> tail := s - (head * halfPower).<br>
>>>> {<br>
>>>> head as: ByteArray.<br>
>>>> (1 to: halfPower digitLength) collect: [:i | halfPower digitAt: i] as: ByteArray.<br>
>>>> (head*halfPower) as: ByteArray.<br>
>>>> }.<br>
>>>><br>
>>>> the correct result is:<br>
>>>> #(#[199 25 70 150 2] #[16 39] #[112 237 78 18 14 101])<br>
>>>><br>
>>>> the wrong result I obtained with SVN revision 3651 compiled by myself is:<br>
>>>> #(#[199 25 70 150 2] #[16 39] #[112 237 78 18 254 61])<br>
>>>><br>
>>>> The most significant bits (above 32) are wrong...<br>
>>>> The pattern I obtain is (with most significant bit put back left)<br>
>>>><br>
>>>> 2r00111101 << 8 + 2r11111110 "wrong result"<br>
>>>> 2r01100101 << 8 + 2r00001110 "Correct result"<br>
>>>><br>
>>>> I completely fail to infer what's going on from this pattern...<br>
>>>><br>
>>>> This is on MacOSX clang --version<br>
>>>> Apple LLVM version 7.3.0 (clang-703.0.29)<br>
>>>> Target: x86_64-apple-darwin15.4.0<br>
>>>><br>
>>>> This goes thru primitiveMultiplyLargeIntegers (29)<br>
>>>> oopResult := self magnitude64BitIntegerFor: result neg: aIsNegative ~= bIsNegative.<br>
>>>> -> sz > 4<br>
>>>> ifTrue: [objectMemory storeLong64: 0 ofObject: newLargeInteger withValue: magnitude]<br>
>>>> (which I changed recently)<br>
>>>><br>
>>>> then:<br>
>>>> storeLong64: longIndex ofObject: oop withValue: value<br>
>>>> <var: #value type: #sqLong><br>
>>>> self flag: #endianness.<br>
>>>> self long32At: oop + self baseHeaderSize + (longIndex << 3) put: (self cCode: [value] inSmalltalk: [value bitAnd: 16rFFFFFFFF]);<br>
>>>> long32At: oop + self baseHeaderSize + (longIndex << 3) + 4 put: (value >> 32).<br>
>>>> ^value<br>
>>>><br>
>>>> I don't see anything wrong with this code...<br>
>>>> Well, using a shift on signed value is not that good, but it works for at least 3 reasons:<br>
>>>> - we throw the signBit extension away<br>
>>>> - slang inlining misses the signedness difference, and the generated C code is correct.<br>
>>>> - Anyway, in our case, the sign bit was 0...<br>
>>>><br>
>>>> Previous implementation in magnitude64BitIntegerFor:neg: was:<br>
>>>> sz > 4 ifTrue:<br>
>>>> [objectMemory storeLong32: 1 ofObject: newLargeInteger withValue: magnitude >> 32].<br>
>>>> objectMemory<br>
>>>> storeLong32: 0<br>
>>>> ofObject: newLargeInteger<br>
>>>> withValue: (self cCode: [magnitude] inSmalltalk: [magnitude bitAnd: 16rFFFFFFFF])<br>
>>>><br>
>>>> Not much different, except that the high 32 bits and low 32 bits are written in a different order...<br>
>>>><br>
>>>> If I had a server I'd like to bisect<br>
>>>> - from which version does this happens<br>
>>>> - for which OS<br>
>>>> - for which compiler<br>
>>>><br>
>>>> Without such information, I think I'll have to debug it either thru simulator or directly in gdb, but I feel like I'm really losing my time :(<br>
>>>><br>
>>>> And I've got a 2nd problem like this one...<br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>><br>
>><br>
>><br>
><br>
><br>
><br>
</div></div><span class=""><font color="#888888">> --<br>
> _,,,^..^,,,_<br>
> best, Eliot<br>
><br>
</font></span></blockquote></div><br></div></div>