[Vm-dev] intAt from sqMemoryAccess.h does not extend sign

[David T. Lewis]

On Tue, Nov 08, 2016 at 03:19:50PM +0100, Nicolas Cellier wrote:
>  
> Hi David,
> Sorry, I've no linux/mac vm available currently to exhibit the problem.

Hi Nicolas,

I was using this pre-built Spur VM:

Virtual Machine
---------------
/usr/local/lib/squeak/5.0-201607171247_64/squeak
Croquet Closure Cog[Spur] VM [CoInterpreterPrimitives VMMaker.oscog-eem.1902]
Unix built on Jul 17 2016 14:02:20 Compiler: 4.6.3
platform sources revision VM: r201607171247 https://github.com/OpenSmalltalk/opensmalltalk-vm.git $ Date: Sun Jul 17 14:47:17 2016 +0200 $ Plugins: r201607171247 https://github.com/OpenSmalltalk/opensmalltalk-vm.git $
CoInterpreter VMMaker.oscog-eem.1902 uuid: d5860bb5-b41c-4337-90d4-c2abc979248d Jul 17 2016
StackToRegisterMappingCogit VMMaker.oscog-eem.1902 uuid: d5860bb5-b41c-4337-90d4-c2abc979248d Jul 17 2016


Unfortunately I cannot compile Cog/Spur on my Ubuntu PC (build system issues),
so I cannot be of much help beyond that.

But just for my own understanding, is it the case that IntegerArray access
does not work for you on a 64 bit image on Windows?

Note, the #ifdef USE_INLINE_MEMORY_ACCESSORS is very old. I think that Ian
wrote it (Tim might know better). The only memory access code that I wrote
is the Smalltalk (slang) version in the VMMaker repository, but that is
not relevant here.

Dave



> But if you look at github head revision
> 
> https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/spur64src/vm/gcc3x-cointerp.c
> 
> then you'll find this code:
> 
> primitiveIntegerAt(void)
> {   DECL_MAYBE_SQ_GLOBAL_STRUCT
>     sqInt addr;
> ... snip declarations of no interest ...
>     sqInt value;
> ... snip long preamble to check parameter types ...
> 
> 	/* for zero indexing */
> 	addr = (rcvr + BaseHeaderSize) + ((index - 1) * 4);
> 	value = intAt(addr);
> 	/* begin pop: */
> 	GIV(stackPointer) += 2 * BytesPerWord;
> 	if ((((((usqInt) value) >> 60) + 1) & 15) <= 1) {
> 		/* begin pushInteger: */
> 		longAtput((sp = GIV(stackPointer) - BytesPerWord), ((value << 3) | 1));
> 		GIV(stackPointer) = sp;
> 	}
> 	else {
> ... snip this branch is never reached ...
> 	}
> }
> 
> Above code clearly can't work if the 32bit contents of addr is not
> sign-extended into the 64bits value...
> So I don't know which version you used exactly,
> ...UNLESS...
> there might be another reason for the mismatch in:
> 
> https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/Cross/vm/sqMemoryAccess.h
> 
> You can see that the two possible compiler branches don't agree and won't
> behave the same sign-wise:
> 
> #ifdef USE_INLINE_MEMORY_ACCESSORS
>   /* Use static inline functions when the compiler produces efficient
> code for small accessors.
>      These are preferred because static type checking will prevent
> inadvertent confusion of pointers and oops. */
> ... snip ...
>   static inline sqInt intAtPointer(char *ptr)			{ return
> (sqInt)(*((unsigned int *)ptr)); }
>   static inline sqInt intAtPointerput(char *ptr, int val)	{ return
> (sqInt)(*((unsigned int *)ptr)= val); }
> ... snip ...
> 
> #else /* USE_INLINE_MEMORY_ACCESSORS */
>   /* Use macros when static inline functions aren't efficient. */
> ... snip ...
> # define intAtPointer(ptr)			((sqInt)(*((int *)(ptr))))
> # define intAtPointerput(ptr,val)	((sqInt)(*((int *)(ptr))= (int)(val)))
> ... snip ...
> #endif /* USE_INLINE_MEMORY_ACCESSORS */
> 
> I presume you (or someone else) arranged to not  USE_INLINE_MEMORY_ACCESSORS
> Though it seems to be the default to use them on macosx (I'm just invoking
> ./mvm).
> 
> The two branches should better adopt the same (sane) behavior, shouldn't
> they?
> 
> 2016-11-08 14:23 GMT+01:00 David T. Lewis <lewis at mail.msen.com>:
> 
> >
> > On Tue, Nov 08, 2016 at 03:29:05AM +0100, Nicolas Cellier wrote:
> > >
> > > In this code:
> > >
> > >   static inline sqInt intAtPointer(char *ptr)   { return
> > > (sqInt)(*((unsigned int *)ptr)); }
> > >   static inline sqInt intAtPointerput(char *ptr, int val)  { return
> > > (sqInt)(*((unsigned int *)ptr)= val); }
> > >
> > > the usage of unsigned int is questionable...
> > > It means that intAt and long32At won't perform sign extension in 64bits
> > > Spur VM.
> > > Since it returns a signed result (sqInt), that's troubling.
> > >
> > > The sole real sender of intAt seems to be primitiveIntegerAt: which fails
> > > for this reason on 64bits spur when fed with negative integers.
> >
> >
> > Can you give an example of the failure? I tried on 64-bit Spur and also
> > on image format 68002 (64-bit V3) on Linux, and the accesses to elements
> > of an IntegerArray work correctly in both cases.
> >
> >
> > For example:
> >
> > { -1 . -2 . -3 . -4 . 7 . -9 } asIntegerArray ==> an IntegerArray(-1 -2 -3
> > -4 7 -9)
> >
> > Accessing the elements of this IntegerArray works as expected, and I can
> > see
> > in an inspector that the first element is stored internally as 16rFFFFFFFF.
> >
> > Is there something else that is failing?
> >
> > Dave
> >
> >
> > >
> > > For long32At, it's difficult to analyze: many senders!
> > >
> > > IMO, if we don't want a signed quantity, we should better write
> > > unsignedLong32At (or use lowcode uint32AtPointer which answers a 32 bits
> > > result)
> >
> >