[Vm-dev] abs() generation problem

David T. Lewis lewis at mail.msen.com
Fri Mar 25 15:58:50 UTC 2016


+1 to adding Nicolas

(I might not be able to do it for a couple of days, please be patient)

Dave

On Thu, Mar 24, 2016 at 10:28:04PM -0700, John McIntosh wrote:
>  
> Fine by me
> 
> On Thu, Mar 24, 2016 at 10:10 PM, Eliot Miranda <eliot.miranda at gmail.com>
> wrote:
> 
> >
> > Dave,
> >
> >     can we give Nicolas write permission to the svn repository please?  At
> > least the cog branch but I think he's to be trusted elsewhere too.  Do
> > others agree?
> >
> > _,,,^..^,,,_ (phone)
> >
> > > On Mar 24, 2016, at 4:28 PM, Nicolas Cellier <
> > nicolas.cellier.aka.nice at gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > abs(sqInt) is a problem on 64bits system.
> > > Because sqInt is a long, this should be labs(sqInt).
> > >
> > > Well, as long as we only use 32 bits out of the 64, AND restrict ourself
> > to little endian, this wrong code apparently works.
> > > But it's so fragile:
> > >
> > > #cat > test_abs.c <<END
> > > #include <stdio.h>
> > > int main() {
> > >   long long x=(1LL << 33) - 1;
> > >   long long y = abs(x);
> > >   printf("x=%lld abs(x)=%lld\n",x,y);
> > > }
> > > END
> > >
> > > #clang test_abs.c
> > > test_abs.c:4:17: warning: implicitly declaring library function 'abs'
> > with type 'int (int)'
> > >       [-Wimplicit-function-declaration]
> > >   long long y = abs(x);
> > >                 ^
> > > test_abs.c:4:17: note: include the header <stdlib.h> or explicitly
> > provide a declaration for 'abs'
> > > test_abs.c:4:17: warning: absolute value function 'abs' given an
> > argument of type 'long long' but has parameter of type
> > >       'int' which may cause truncation of value [-Wabsolute-value]
> > >   long long y = abs(x);
> > >                 ^
> > > test_abs.c:4:17: note: use function 'llabs' instead
> > >   long long y = abs(x);
> > >                 ^~~
> > >                 llabs
> > > test_abs.c:4:17: note: include the header <stdlib.h> or explicitly
> > provide a declaration for 'llabs'
> > >
> > > #./a.out
> > > x=8589934591 abs(x)=1
> > >
> > >
> > > So we can think of adding a #generateAbs:on:indent:
> > > and handle the #typeFor:in:
> > >
> > > BUT: sqInt is either defined as int or long depending on the target:
> > >
> > > #if defined(SQ_IMAGE32)
> > >   typedef int        sqInt;
> > >   typedef unsigned int    usqInt;
> > > #elif defined(SQ_HOST64)
> > >   typedef long        sqInt;
> > >   typedef unsigned long    usqInt;
> > > #elif (SIZEOF_LONG_LONG != 8)
> > > #   error long long integers are not 64-bits wide?
> > > #else
> > >   typedef long long        sqInt;
> > >   typedef unsigned long long    usqInt;
> > > #endif
> > >
> > > Here, I propose 3 solutions:
> > > 1) since sqInt=int=long in 32bits spur, and sqInt=long in 64bits spur we
> > can cheat:
> > >     sqLong 'long long' int64_t __int64 -> #llabs
> > >     sqInt long -> #lasb
> > >     int int32_t __int32 -> #abs
> > >     it's not straight and may still generate compiler warnings, which
> > should be avoided
> > > 2) testing the wordSize, and branching in the generator:
> > >     is64bitsVM := vmClass notNil and: [vmClass objectMemoryClass
> > wordSize = 8].
> > >     Ah no, we currently generate single source for src/plugins in 32 and
> > 64bits flavour,
> > >     that ain't gonna work, or we should differentiate the plugin
> > generation
> > > 3) generate a macro SQABS for sqInt and define it in sqMemoryAccess.h,
> > same of sqLong
> > >     sqInt -> SQABS
> > >     sqLong -> SQLABS
> > >
> > > The correct solution is 3), but I have no commit right to svn, so
> > someone should do it.
> > > I'm attaching my own sqMemoryAccess.h copy, but please note that it has
> > other improvments like
> > > - avoiding un-strict pointer aliasing via memcpy for fetching/storing
> > float (except swapper in BIGENDIAN branch)
> > > - providing some native unsigned memory access
> > >
> > > ----------------------------------------------
> > >
> > > Now what about unsigned types?
> > >
> > > #cat > test_abs.c <<END
> > > #include <stdio.h>
> > > int main() {
> > >   unsigned int x=-1;
> > >   printf("abs(x)=%d\n",abs(x));
> > >   return 0;
> > > }
> > > END
> > >
> > > #test_abs.c:4:24: warning: implicitly declaring library function 'abs'
> > with type 'int (int)'
> > >       [-Wimplicit-function-declaration]
> > >   printf("abs(x)=%d\n",abs(x));
> > >                        ^
> > > test_abs.c:4:24: note: include the header <stdlib.h> or explicitly
> > provide a declaration for 'abs'
> > > test_abs.c:4:24: warning: taking the absolute value of unsigned type
> > 'unsigned int' has no effect [-Wabsolute-value]
> > >   printf("abs(x)=%d\n",abs(x));
> > >                        ^
> > > test_abs.c:4:24: note: remove the call to 'abs' since unsigned values
> > cannot be negative
> > >   printf("abs(x)=%d\n",abs(x));
> > >                        ^~~
> > >
> > > #./a.out
> > > abs(x)=1
> > >
> > > clang first tells that there is no point of taking absolute value of an
> > unsigned int, an unsigned is allways positive.
> > >
> > > BUT, then it re-interprets the unsigned as signed, and the program
> > return +1.
> > > It would return -1 if abs were really a no-op.
> > >
> > > So there are several alternatives for the generator here:
> > > - 1) suppress the generation of abs for an unsigned
> > >       BEWARE, this will change behaviour vs current state of VMMaker
> > > - 2) force a signed cast to remove the warning
> > >       we just enforce current state of VMMaker but tell the compiler to
> > shut up
> > > - 3) do nothing and leave the problem for later...
> > >       the compiler will spit more errors
> > >
> > > I'm attaching solution 3), have programmed 2) in my own branch.
> > > 1) is dangerous because of 2 things :
> > > - unwanted promotion to unsigned type in C - simply put a sizeof(...) in
> > the expression and it turns unsigned...
> > > - limited confidence about how slang inlining is handling the implicit
> > type conversions...
> > >
> > > Currently there is at least 1 abs(unsigned) which is generated in
> > shrinkObjectMemory :
> > > ../../spurstack64src/vm/gcc3x-interp.c:50178:12: warning: taking the
> > absolute value of unsigned type 'unsigned long' has  no effect
> > [-Wabsolute-value]
> > >                                          && ((SQABS((((seg->segSize)) -
> > shrinkage))) < delta1)) {
> > >
> > > This is because shrinkage is declared usqInt...
> > >
> > > There would be yet another solution
> > > - 4) raise an exception and let the dear VMMaker user handle the
> > ambiguity
> > >
> > > That's my favourite, but It's only a comment in attachment, I don't want
> > to break yet another Jenkins job ;)
> > > We should force the correct cast where there is ambiguity.
> > > If we do not, the exception could be proceedable and would proceed to
> > solution 3) -
> > > (we do not shut up the compiler and give another chance to someone to
> > understand and correct slang source...)
> > >
> > > Eliot, David, Tim, others, what do you think ?
> > >
> > > <sqMemoryAccess.h>
> > > <patch_abs_generation.st>
> >
> 
> 
> 
> -- 
> ===========================================================================
> John M. McIntosh. Corporate Smalltalk Consulting Ltd
> https://www.linkedin.com/in/smalltalk
> ===========================================================================



More information about the Vm-dev mailing list