[Vm-dev] abs() generation problem

Esteban Lorenzano estebanlm at gmail.com
Fri Mar 25 16:24:52 UTC 2016


which would not be an issue if we were using git… :)

(sorry, I couldn’t resist it)

Esteban

> On 25 Mar 2016, at 16:58, David T. Lewis <lewis at mail.msen.com> wrote:
> 
> 
> +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