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