ldc icon indicating copy to clipboard operation
ldc copied to clipboard

Slightly different behavior in LDC & DMD with alias this involved

Open AndrejMitrovic opened this issue 2 years ago • 5 comments
trafficstars

A very contrived example:

import core.atomic;

struct Padded(T)
{
align(64):
    T data;
    alias data this;  // probably works in DMD but not LDC?
}

struct Wrap(T : bool)
{
    shared(Padded!T)* value;

    void opAssign(T newValue) {
        atomicStore!(MemoryOrder.raw)(*this.value, Padded!T(newValue));
    }
}

alias X = Wrap!bool;

void main() { }

Test compiles:

DMD:

# Ok
$ dmd -m64 test.d

LDC:

$ ldc2 -m64 test.d
C:\Apps\ldc\bin\..\import\core\internal\atomic.d(171): 
  Error: static assert:  "Cannot atomically load/store type of size 64LU"

I haven't looked too deeply into this, but I think maybe this has something to do with alias this working differently in DMD / LDC? Or could it be that the druntime's are a bit different? I suspect it's the latter.

I'm not sure if this should be classified as a bug. Probably the user code should not try to use alias this with atomics as it could be dangerous.

AndrejMitrovic avatar Jan 19 '23 17:01 AndrejMitrovic

If you are comparing against a matching DMD version, the alias this behavior should be identical. The druntime implementation differs though: https://github.com/ldc-developers/ldc/blob/1755bccdc88e33a314326ce27a683b314bb8b826/runtime/druntime/src/core/internal/atomic.d#L15-L175

So I expect DMD stores the whole struct too as LDC attempts, but supports (or claims to support) that 64-bytes atomic store (in druntime).

kinke avatar Jan 23 '23 22:01 kinke

Yep - the AST (e.g., on run.dlang.io) for your example with DMD contains a totally atomic:

atomicStore!(MemoryOrder.raw, Padded!bool)
{
	pure nothrow @nogc @trusted void atomicStore(Padded!bool* dest, Padded!bool value)
	{
		*dest = value;
	}

}

kinke avatar Jan 23 '23 22:01 kinke

I guess here one can argue that the store does not have to be 64 bytes, but only as large as T (the padding does not need to be written to). Not sure how much this would complicate the implementation of atomicStore...

JohanEngelen avatar Jan 23 '23 23:01 JohanEngelen

Ah, I thought the DMD version used alias this to then only atomically store the boolean. Honestly not really a blocker for me, can assign this as low-priority or a wontfix.

AndrejMitrovic avatar Jan 26 '23 09:01 AndrejMitrovic

Ah, I thought the DMD version used alias this to then only atomically store the boolean.

Perhaps it would work if we add a size restriction as template condition, then template instantiation fails with Padded!bool and then the alias this is tried ?

JohanEngelen avatar Jan 26 '23 18:01 JohanEngelen