Comment #0 by jason.james.house — 2009-01-25T17:21:09Z
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.
Comment #1 by 2korden — 2009-01-26T00:25:26Z
(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...
Comment #2 by clugdbug — 2009-01-26T04:33:09Z
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.
Comment #3 by yebblies — 2012-01-29T22:04:24Z
Raising the severity as other assemblers apparently have different defaults, causing hard to find bugs.
Comment #4 by yebblies — 2012-01-29T22:04:33Z
*** Issue 7388 has been marked as a duplicate of this issue. ***
Comment #5 by bugzilla — 2020-08-21T06:57:19Z
The following code:
uint func()
{
asm
{
naked;
inc [EAX];
inc byte ptr [EAX];
inc short ptr [EAX];
inc int ptr [EAX];
inc long ptr [EAX];
}
}
generates:
__D5test24funcFZk:
inc byte ptr [EAX]
inc byte ptr [EAX]
inc word ptr [EAX]
inc dword ptr [EAX]
inc byte ptr [EAX] <== !!!!!
That last is certainly a problem; it should be rejected by the compiler. As for the first instruction, I'm concerned about changing the behavior. Messing with people's existing, working asm code is risky.
Comment #6 by dlang-bugzilla — 2020-08-21T07:23:09Z
(In reply to Walter Bright from comment #5)
> As for the first instruction, I'm concerned about changing the behavior.
> Messing with people's existing, working asm code is risky.
See issue 7388. The problem is that if you copy/paste code from another program, you'll get a different result. It probably should be deprecated, forcing users to explicitly specify the operand size.
Comment #7 by bugzilla — 2020-08-21T07:46:26Z
(In reply to Walter Bright from comment #5)
> The following code:
>
> uint func()
> {
> asm
> {
> naked;
> inc long ptr [EAX];
> }
> }
>
> generates:
>
> __D5test24funcFZk:
> inc byte ptr [EAX] <== !!!!!
>
Filed as https://issues.dlang.org/show_bug.cgi?id=21181 so as not to confuse this issue.
Comment #8 by bugzilla — 2020-08-21T08:30:27Z
Looking at my ancient Intel 8088 manual, it says:
INC [BX]
is ambiguous and so must use BYTE PTR or WORD PTR. Since the behavior of iasm is modeled after Intel's syntax, that pretty much settles it.