www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 2617] New: incorrect code generation for asm{ inc eax }

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2617

           Summary: incorrect code generation for asm{ inc eax }
           Product: D
           Version: 2.022
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Keywords: wrong-code
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: bugzilla digitalmars.com
        ReportedBy: jason.james.house gmail.com


The following code fails to assert.

import tango.core.Atomic;
void main(){
  int j;
  foreach (i; 1..500_000){
    atomicIncrement!(msync.raw, int)(j);
    assert(j == (i%256));
  }
}

Digging deeper, this is a problem because the atomicIncrement is translated to
the following assembly.  Note the critical line of "lock incb" which should be
"lock inc"

 80496b4:       55                      push   %ebp
 80496b5:       8b ec                   mov    %esp,%ebp
 80496b7:       50                      push   %eax
 80496b8:       8b 45 fc                mov    -0x4(%ebp),%eax
 80496bb:       f0 fe 00                lock incb (%eax)
 80496be:       8b 00                   mov    (%eax),%eax
 80496c0:       8b e5                   mov    %ebp,%esp
 80496c2:       5d                      pop    %ebp
 80496c3:       c3                      ret    

Here's the original assembly in the Tango module:
asm
{
  mov EAX, val;
  lock; // lock always needed to make this op atomic
  inc [EAX];
  mov EAX, [EAX];
}

This problem appears in both D1 and D2 and prevents lockless algorithms.


-- 
Jan 25 2009
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2617





------- Comment #1 from 2korden gmail.com  2009-01-26 00:25 -------
(In reply to comment #0)
 The following code fails to assert.
 
 import tango.core.Atomic;
 void main(){
   int j;
   foreach (i; 1..500_000){
     atomicIncrement!(msync.raw, int)(j);
     assert(j == (i%256));
   }
 }
 

Shouldn't that be: assert(j == i); // ? I have noted that Tango atomicIncrement increments least significant byte only, too, and was wondering whether it is intended... --
Jan 25 2009
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=2617


clugdbug yahoo.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|wrong-code                  |accepts-invalid
            Summary|incorrect code generation   |asm silently accepts
                   |for asm{ inc eax }          |ambiguous-sized operations
            Version|2.022                       |1.00




------- Comment #2 from clugdbug yahoo.com.au  2009-01-26 04:33 -------
The original bug is not valid. It's certainly not wrong-code. And there is no
'inc eax' involved! (inc [EAX]; is completely different). The Tango code has a
bug; it should be

inc int ptr [EAX];

You can argue that there's an accepts-invalid bug in the assembler. I hate that
it silently accepts ambiguous-sized operations, always assuming 'byte' size.
Applies also to

mul [EBX]; // multiplies AL by byte ptr [EAX].

I've been bitten by this quite a few times, on really ancient versions of DMD.
It's nothing to do with D2.


-- 
Jan 26 2009