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

[Nicolas Cellier]

2016-11-08 16:08 GMT+01:00 David T. Lewis <lewis at mail.msen.com>:

>
> 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
>
>
So this path is forced on macosx 32 & 64 bits via
https://raw.githubusercontent.com/OpenSmalltalk/opensmalltalk-vm/Cog/build.macos64x64/common/Makefile.vm

DEFS:=    $(COGDEFS) -DUSE_GLOBAL_STRUCT=0 -DNO_ISNAN=1 \
        -DUSE_INLINE_MEMORY_ACCESSORS -D'TZ="$(TZ)"' \
        $(INTERPFLAGS)

Linux does not have it set... That explains the difference, I tested 64bits
spur on mac...


>
>
> > 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)
> > >
> > >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20161108/8d54bed1/attachment.html>