<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Levente,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 22, 2019 at 1:58 AM Levente Uzonyi <<a href="mailto:leves@caesar.elte.hu">leves@caesar.elte.hu</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> <br>
Thanks. I spent some time in gdb land and there seems to be a miscompiled <br>
conditional jump. The question is: is it a compiler bug or a bug in the <br>
Smalltalk -> C translation. With gcc 4.8, the correct jump instruction is <br>
generated.<br>
<br>
Here is the disassembled code with my comments following the path when <br>
-100.0 basicAt: 1 is evaluated in the image:<br>
<br>
Dump of assembler code for function primitiveFloatAt:<br>
    0x0000555555589ec0 <+0>:    mov    0x3775b1(%rip),%rdx        # 0x555555901478 <stackPointer><br>
%rax will hold the index (1 or 2) as a SmallInteger (so the actual value will be 0x9 or 0x11 for 1 and 2 respectively).<br>
    0x0000555555589ec7 <+7>:    mov    (%rdx),%rax<br>
%rcx is the receiver object's address<br>
    0x0000555555589eca <+10>:   mov    0x8(%rdx),%rcx<br>
Is the index SmallInteger 1?<br>
    0x0000555555589ece <+14>:   cmp    $0x9,%rax<br>
If yes, jump to 0x555555589ef8<br>
    0x0000555555589ed2 <+18>:   je     0x555555589ef8 <primitiveFloatAt+56><br>
    0x0000555555589ed4 <+20>:   cmp    $0x11,%rax<br>
    0x0000555555589ed8 <+24>:   je     0x555555589f30 <primitiveFloatAt+112><br>
    0x0000555555589eda <+26>:   and    $0x7,%eax<br>
    0x0000555555589edd <+29>:   cmp    $0x1,%rax<br>
    0x0000555555589ee1 <+33>:   sete   %al<br>
    0x0000555555589ee4 <+36>:   movzbl %al,%eax<br>
    0x0000555555589ee7 <+39>:   add    $0x3,%rax<br>
    0x0000555555589eeb <+43>:   mov    %rax,0x37757e(%rip)        # 0x555555901470 <primFailCode><br>
    0x0000555555589ef2 <+50>:   retq<br>
    0x0000555555589ef3 <+51>:   nopl   0x0(%rax,%rax,1)<br>
Set the value of  %eax to 0 for seemingly no reason yet. The returned SmallInteger will be computed in %rax, so this is important.<br>
    0x0000555555589ef8 <+56>:   xor    %eax,%eax<br>
Check if the receiver has the SmallFloat tag set (0x4)<br>
    0x0000555555589efa <+58>:   test   $0x4,%cl<br>
If yes, jump to  0x555555589f03. This is the miscompiled conditional jump. It should go to 0x0000555555589f37, but it will just skip the next instruction leaving %eax set to 0.<br>
=> 0x0000555555589efd <+61>:    jne    0x555555589f03 <primitiveFloatAt+67> <br>
Copy the 2nd 32-bit field of the receiver into %rax with sign extension?<br>
    0x0000555555589eff <+63>:   movslq 0xc(%rcx),%rax<br>
Shift by 3 bits to make room for the tag.<br>
    0x0000555555589f03 <+67>:   shl    $0x3,%rax<br>
Adjust the stack pointer.<br>
    0x0000555555589f07 <+71>:   add    $0x8,%rdx<br>
Prepare a 32-bit mask shifted left by 3 bits for the tag.<br>
    0x0000555555589f0b <+75>:   movabs $0x7fffffff8,%rcx<br>
Mask the result.<br>
    0x0000555555589f15 <+85>:   and    %rcx,%rax<br>
Add the SmallInteger tag.<br>
    0x0000555555589f18 <+88>:   or     $0x1,%rax<br>
Store the result on the stack.<br>
    0x0000555555589f1c <+92>:   mov    %rax,(%rdx)<br>
    0x0000555555589f1f <+95>:   mov    %rdx,0x377552(%rip)        # 0x555555901478 <stackPointer><br>
    0x0000555555589f26 <+102>:  retq<br>
    0x0000555555589f27 <+103>:  nopw   0x0(%rax,%rax,1)<br>
    0x0000555555589f30 <+112>:  xor    %eax,%eax<br>
    0x0000555555589f32 <+114>:  test   $0x4,%cl<br>
    0x0000555555589f35 <+117>:  jne    0x555555589f03 <primitiveFloatAt+67><br>
Copy the 1st 32-bit field of the receiver into %rax with sign extension?<br>
    0x0000555555589f37 <+119>:  movslq 0x8(%rcx),%rax<br>
    0x0000555555589f3b <+123>:  jmp    0x555555589f03 <primitiveFloatAt+67><br>
End of assembler dump.<br></blockquote><div><br></div><div>I'm pretty sure the C code is correct, but Nicolas is our C lawyer.  However we need to look at both primitiveFloatAt and fetchLong32:ofFloatObject:. Here's the source:</div><div><br></div><div><div>    /* Spur64BitMemoryManager>>#fetchLong32:ofFloatObject: */</div><div>static sqInt NoDbgRegParms</div><div>fetchLong32ofFloatObject(sqInt fieldIndex, sqInt oop)</div><div>{</div><div>    usqLong bits;</div><div>    usqLong rot;</div><div><br></div><div>    if (!(oop & (smallFloatTag()))) {</div><div>        return long32At((oop + BaseHeaderSize) + (((sqInt)((usqInt)(fieldIndex) << 2))));</div><div>    }</div><div>    /* begin smallFloatBitsOf: */</div><div>    assert(isImmediateFloat(oop));</div><div>    rot = ((usqInt) (((usqInt)oop))) >> (numTagBits());</div><div>    if (rot > 1) {</div><div><br></div><div>        /* a.k.a. ~= +/-0.0 */</div><div>        rot += ((sqInt)((usqInt)((smallFloatExponentOffset())) << ((smallFloatMantissaBits()) + 1)));</div><div>    }</div><div>    /* begin rotateRight: */</div><div>    rot = (rot << 0x3F) + (((usqInt) (((usqInt)rot))) >> 1);</div><div>    bits = rot;</div><div>    return (((int *) ((&bits))))[fieldIndex];</div><div>}</div></div><div><br></div><div>Note that [+/-]100.0 is an immediate float.  Nicolas, is this kind of pointer punning still OK?  Notice that at no time do we pun the bits to a double.  They are always integral bits.  What we're doing is manipulating the bit pattern for -100.0, an immediate SmallFloat64. (Spur64BitMemoryManager new smallFloatObjectOf: -100.0) hex '16r859000000000000C'  where 16r4 is the tag pattern for an immediate float and 16r8 is the sign bit in the least significant position.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
Levente<br>
<br>
On Fri, 22 Feb 2019, Tobias Pape wrote:<br>
<br>
> <br>
> Hi Levente.<br>
> <br>
>> On 22.02.2019, at 00:43, Levente Uzonyi <<a href="mailto:leves@caesar.elte.hu" target="_blank">leves@caesar.elte.hu</a>> wrote:<br>
>> <br>
>> I had a look at the generated code and I think the generated code for primitiveAsFloat has no issues. The machine code is simple and looks correct with gcc 8, thought the pxor operation is unnecessary (and not generated by gcc 4) as cvtsi2sd will zero fill the higher 64 bits of xmm0.<br>
>> <br>
>> GCC 4.8:<br>
>> <br>
>> 0000000000432ff0 <primitiveAsFloat>:<br>
>> {   DECL_MAYBE_SQ_GLOBAL_STRUCT<br>
>>  432ff0:       53                      push   %rbx<br>
>>        rcvr = longAt(GIV(stackPointer));<br>
>>  432ff1:       48 8b 1d 20 24 36 00    mov    0x362420(%rip),%rbx        # 795418 <stackPointer><br>
>>        longAtput((sp = GIV(stackPointer) + ((1 - 1) * BytesPerWord)), floatObjectOf(((double) ((rcvr >> 3)) )));<br>
>>  432ff8:       48 8b 03                mov    (%rbx),%rax<br>
>>  432ffb:       48 c1 f8 03             sar    $0x3,%rax<br>
>>  432fff:       f2 48 0f 2a c0          cvtsi2sd %rax,%xmm0<br>
>>  433004:       e8 37 ee ff ff          callq  431e40 <floatObjectOf><br>
>>  433009:       48 89 03                mov    %rax,(%rbx)<br>
>>        GIV(stackPointer) = sp;<br>
>>  43300c:       48 89 1d 05 24 36 00    mov    %rbx,0x362405(%rip)        # 795418 <stackPointer><br>
>> }<br>
>>  433013:       5b                      pop    %rbx<br>
>>  433014:       c3                      retq<br>
>>  433015:       90                      nop<br>
>>  433016:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)<br>
>>  43301d:       00 00 00<br>
>> <br>
>> GCC 8.2:<br>
>> <br>
>> 000000000004a1f0 <primitiveAsFloat>:<br>
>> {   DECL_MAYBE_SQ_GLOBAL_STRUCT<br>
>>   4a1f0:       53                      push   %rbx<br>
>>        rcvr = longAt(GIV(stackPointer));<br>
>>   4a1f1:       48 8b 1d 80 32 36 00    mov    0x363280(%rip),%rbx        # 3ad478 <stackPointer><br>
>>        longAtput((sp = GIV(stackPointer) + ((1 - 1) * BytesPerWord)), floatObjectOf(((double) ((rcvr >> 3)) )));<br>
>>   4a1f8:       66 0f ef c0             pxor   %xmm0,%xmm0<br>
>>   4a1fc:       48 8b 03                mov    (%rbx),%rax<br>
>>   4a1ff:       48 c1 f8 03             sar    $0x3,%rax<br>
>>   4a203:       f2 48 0f 2a c0          cvtsi2sd %rax,%xmm0<br>
>>   4a208:       e8 b3 eb ff ff          callq  48dc0 <floatObjectOf><br>
>>   4a20d:       48 89 03                mov    %rax,(%rbx)<br>
>>        GIV(stackPointer) = sp;<br>
>>   4a210:       48 89 1d 61 32 36 00    mov    %rbx,0x363261(%rip)        # 3ad478 <stackPointer><br>
>> }<br>
>>   4a217:       5b                      pop    %rbx<br>
>>   4a218:       c3                      retq<br>
>>   4a219:       0f 1f 80 00 00 00 00    nopl   0x0(%rax)<br>
>> <br>
>> <br>
>> Actually the returned float object is -100.0 (-100 asFloat < 0.0 returns true), but the code used for printing doesn't work properly, because #basicAt: returns 0 (when the argument is 1 or 2), and the method used to print the number uses #signBit to print the negative sign, and #signBit relies on #basicAt: returning a non-zero value when the number is negative.<br>
>> <br>
>> So, we're back to the original issue I found: #basicAt: (primitive 38) doesn't work with newer gcc versions.<br>
>> <br>
><br>
> Wow. Great analysis! Thank you :)<br>
><br>
> Best regards<br>
>       -Tobias<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div></div></div></div></div></div>