[Vm-dev] abs() generation problem

Eliot Miranda eliot.miranda at gmail.com
Fri Mar 25 05:10:53 UTC 2016


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>


More information about the Vm-dev mailing list