<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2016-03-31 2:41 GMT+02:00 David T. Lewis <span dir="ltr"><<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.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>
Changing the subject line to reflect current discussion topic.<br>
<br>
On Wed, Mar 30, 2016 at 10:46:21PM +0200, Nicolas Cellier wrote:<br>
><br>
> 2016-03-30 13:46 GMT+02:00 Ben Coman <<a href="mailto:btc@openinworld.com">btc@openinworld.com</a>>:<br>
><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>
> > ><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>
> > >> 2016-03-27 15:39 GMT+02:00 Nicolas Cellier <<br>
> > <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<br>
> > fail with a squeak.stack.v3 VM compiled with head revision of COG VM<br>
> > branch, whatever OS.<br>
> > >>><br>
> > >>> Then knowing from which SVN version of COG VM branch the KernelNumber<br>
> > 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><br>
> > 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<br>
> > 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<br>
> > << 3)]<br>
> > >> ifFalse:<br>
> > >> [self cppIf: VMBIGENDIAN<br>
> > >> ifTrue: [((self long32At: oop + self baseHeaderSize +<br>
> > (longIndex << 3)) asUnsignedLongLong << 32)<br>
> > >> + (self long32At: oop + self baseHeaderSize +<br>
> > (longIndex << 3 + 4))]<br>
> > >> ifFalse: [(self long32At: oop + self baseHeaderSize +<br>
> > (longIndex << 3))<br>
> > >> + ((self long32At: oop + self baseHeaderSize +<br>
> > (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<br>
> > signedness...<br>
> > >> I would much much much prefer to call unsignedLong32At because it's<br>
> > what I mean 9 times out of 10: get the magnitude...<br>
> > ><br>
> > ><br>
> > > Let's add them. It would be great to have them all consistent too. But<br>
> > at least let's add unsignedLong32At:[put:]. Wait. In the simulator<br>
> > 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<br>
> > signedLongAt:[put:] et al.<br>
> > > c) make the simulator's longAt:[put:] long32At:put: signed and then add<br>
> > unsignedLongAt:[put:] et al<br>
> > ><br>
> > > Any others?<br>
> > ><br>
> > > I think a) is unwise. The simulator and C should agree. Nicolas, I can<br>
> > join your experience in finding that debugging these issues is extremely<br>
> > time consuming.<br>
> > ><br>
> > > My preference is for b); some uses where the type should be signed will<br>
> > by located by C compiler warnings; we hope that VM tests will catch the<br>
> > 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>
> ><br>
> ><br>
> My first thought was as Eliot, use b) because<br>
> - the simulator is already b)<br>
> - 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<br>
> signedIntFromLong,<br>
> * long unconditionnally meaning 64 bits like sqLong<br>
> * long meaning either 32 or 64 bits depending on architecture in generated<br>
> C (like in StackInterperter>>putLong:toFile:)<br>
> * long meaning either 32 or 64 depending on the context<br>
> (#longAt: is allways 32 bits when sent to ByteArray Bitmap or CAccessor,<br>
> but might be 64 bits when sent to simulator argh...)<br>
><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<br>
> on target architecture.<br>
> Even now, I'm not sure of which word we are talking of...<br>
<br>
Me too. This is endlessly confusing no matter how long I look at it.<br>
We need to clarify the terminology and the type declarations to distinguish<br>
between references to slots in the object memory, object pointers that<br>
coincidentally happen to fit into slots in the object memory, and references<br>
to machine data types (C short/int/long/long long).<br>
<br>
><br>
> Also b) goes with a blitz strategy:<br>
> - do a massive semantic change long32 -> unsigned<br>
> - review the send sites and change a few variant to signed<br>
> - repair broken C code once image start crashing and tests start failing<br>
> - repair the simulator which might show different symptoms (see<br>
> implementors of #asUnsignedLong)<br>
> either it works soon, or never...<br>
><br>
> I've noted only 47 senders of long32At: .<br>
> Plus the 33 senders of fetchLong32:ofObject:<br>
> And cannot analyze easily the possible side effects thru Slang type<br>
> inference...<br>
><br>
> As my recent bug of LargeInteger division showed: abuse of unsigned lead to<br>
> expensive bug tracking too.<br>
> That does a bit restrain my enthusiasm for solution b) - chat ??chaud??<br>
> craint l'eau froide ;)<br>
><br>
> Last, I did not apply b) policy in my VM branch for 32 bits large int.<br>
> Instead I introduced the unsigned variant gradually... (coward !)<br>
><br>
> Before we attempt anything, I'd like to remind the short term goals:<br>
> - avoid uncontrolled sign extension when promoting to larger int in C<br>
> - avoid undefined behavior, most of which being related to signed arithmetic<br>
> - make the simulator better match C (or vice versa in the case)<br>
><br>
> The last goal is far away, there are subtle "differences"...<br>
> Like in the simulator, the difference of two unsigned might be < 0.<br>
> In C not, unless we force it (cast, type punning, whatever...).<br>
><br>
> Last point, gradual introduction and non-regression testing pre-suppose a<br>
> stable VM.<br>
> I still experiment random crashes I'd like to track first.<br>
><br>
<br>
I am still undecided between b) and c). Ben's approach makes the most<br>
sense to me on the face of it, but I just did a quick test of the b)<br>
approach and the resulting VM works fine, with BitBltTest still green.<br>
I did this by modifying the MemoryAccess methods so that #long32At:<br>
generates this (which will be inlined in the actual VM code):<br>
<br>
/* #define long32At intAt */<br>
<br>
static unsigned int long32At(sqInt ptr) {<br>
return (((unsigned int *) ((sqMemoryBase) + ptr)))[0];<br>
}<br>
<br>
<br>
I am probably oversimplifying, and different compilers may treat these<br>
things differently, but I see two conclusions that may be drawn from this:<br>
<br>
1) Declaring #long32At: as either signed or unsigned is not really<br>
important. What is important is the type declaration of the thing that<br>
the result gets stored into. We are assuming twos compliment representations,<br>
and a function that stores a "signed" value into some context that treats<br>
it as unsigned will be an unsigned value, and vice versa.<br>
<br>
2) We currently treat the result of #byteAt:, #shortAt:, #intAt:, and<br>
#longAt: as signed values. There is no reason to change that convention.<br>
But #long32At: (or #long128At: or whatever) could easily be changed at<br>
this point to explicitly answer unsigned values. It does not look to<br>
me like this would break anything, and it makes sense intuitively because<br>
saying "long32At" implies that I want to retrieve a binary bit field<br>
of size 32, with no presumption that it should be interpreted either as<br>
twos compliment signed or as unsigned.<br>
<br>
Dave<br>
<br></blockquote><div><br></div><div>It's bits as long as you don't use it in an expression<br></div><div>We have to decide about sign as soon as we write:<br></div><div> low | ((unsigned long long) high << 32)<br></div><div>Note that I used | instead of + to better show my intention to operate on bits...<br></div><div>But when the compiler has to extend 32bits low to 64bits, it has to know how to handle sign bit.<br></div><div>In this example we need to explicitely declare things unsigned...<br><br></div><div>But there is even more trickyness in C:<br>declaring all variable as unsigned still does not protect us from sign extension<br></div><div>Here is a small snippet to illustrate it<br><br>$ cat >test.c <<END<br>#include <stdio.h><br>int main() {<br> unsigned short x=0xFF00;<br> unsigned long long y = x << 16;<br> printf("y=%llx\n",y);<br> return 0;<br>}<br>END<br><br></div><div>$ gcc tests.c<br></div><div>$ ./a.exe<br>y=ffffffffff000000<br><br></div><div>Surprising?<br></div><div>No, the unsigned short gets promoted to int (signed int) in the expression x << 16<br></div><div>So we overflow the sign bit (UB) and then promote 32bit signed to 64bits...<br></div><div> <br></div></div><br></div></div>