abs() generation problem

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

abs() generation problem

Nicolas Cellier
 
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 (17K) Download Attachment
patch_abs_generation.st (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: abs() generation problem

Eliot Miranda-2

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 <[hidden email]> 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>
Reply | Threaded
Open this post in threaded view
|

Re: abs() generation problem

johnmci
 
Fine by me

On Thu, Mar 24, 2016 at 10:10 PM, Eliot Miranda <[hidden email]> 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 <[hidden email]> 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
===========================================================================
Reply | Threaded
Open this post in threaded view
|

Re: abs() generation problem

David T. Lewis
 
+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 <[hidden email]>
> 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 <
> > [hidden email]> 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
> ===========================================================================

Reply | Threaded
Open this post in threaded view
|

Re: abs() generation problem

EstebanLM

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 <[hidden email]> 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 <[hidden email]>
>> 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 <
>>> [hidden email]> 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
>> ===========================================================================
>