fix issue 16230 - core.atomic.atomicLoad removes shared from aggregat… #1605
Changes from all commits
7efd0fe
ee8a3aa
6a96fbe
cbe5e3f
c87a1b9
0b1b6c8
eab4c63
6a6051d
612f502
2a0187e
0f2b461
6178e6f
927b48b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
`core.atomic.atomicLoad`'s returns types have changed for aggregate types that have indirections. | ||
|
||
$(REF atomicLoad, core,atomic) used to strip the `shared` qualifier off too | ||
eagerly. When an aggregate type has a "head" and a "tail", connected by an | ||
indirection, then `atomicLoad` used to strip `shared` off the tail. That was a | ||
bug ($(BUGZILLA 16230)). `atomicLoad` only loads the head. The tail remains in | ||
shared memory, and must keep the `shared` qualifier. | ||
|
||
When loading a struct that contains indirections, `atomicLoad` now returns a | ||
wrapper that provides getters which return properly typed values. | ||
|
||
When loading a class reference, `atomicLoad` now leaves the `shared` qualifier | ||
on. | ||
|
||
--- | ||
|
||
Example: | ||
---- | ||
class C { int value; } | ||
shared C shc = new C; | ||
|
||
struct S { int head; int* tailPointer; } | ||
shared int tail = 1; | ||
auto shs = shared S(2, &tail); | ||
|
||
void main() | ||
{ | ||
import core.atomic : atomicLoad, atomicOp; | ||
|
||
// Loading a class reference: | ||
shared C c = atomicLoad(shc); | ||
// c itself is not actually shared. It's safe to copy it non-atomically: | ||
shared C c2 = c; // ok | ||
// c's fields are still shared and need to be loaded atomically: | ||
int v = atomicLoad(c.value); | ||
|
||
// Loading a struct that has an indirection: | ||
auto s = atomicLoad(shs); | ||
// The struct's head has been copied and can be modified non-atomically: | ||
++s.head; | ||
// The tail is still shared and needs to be handled atomically: | ||
shared(int)* t = s.tailPointer; | ||
atomicOp!"+="(*t, 1); | ||
} | ||
---- |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,12 +32,117 @@ else | |
|
||
private | ||
{ | ||
template HeadUnshared(T) | ||
/* Construct a type with a shared tail, and if possible with an unshared | ||
head. */ | ||
template TailShared(U) if (!is(U == shared)) | ||
{ | ||
static if( is( T U : shared(U*) ) ) | ||
alias shared(U)* HeadUnshared; | ||
alias TailShared = .TailShared!(shared U); | ||
} | ||
template TailShared(S) if (is(S == shared)) | ||
{ | ||
// Get the unshared variant of S. | ||
static if (is(S U == shared U)) {} | ||
else static assert(false, "Should never be triggered. The `static " ~ | ||
"if` declares `U` as the unshared version of the shared type " ~ | ||
"`S`. `S` is explicitly declared as shared, so getting `U` " ~ | ||
"should always work."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused by this assert. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
U is different from T when T is shared. T may be shared or not. S is always shared. U is always unshared. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, so it T is e.g. shared(int), then S is also shared(int) and U is int. This is worth placing in an explanatory comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Added a comment. |
||
|
||
static if (is(S : U)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Under what circumstances is U different from T? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answered above. |
||
alias TailShared = U; | ||
else static if (is(S == struct)) | ||
{ | ||
enum implName = () { | ||
/* Start with "_impl". If S has a field with that name, append | ||
underscores until the clash is resolved. */ | ||
string name = "_impl"; | ||
string[] fieldNames; | ||
static foreach (alias field; S.tupleof) | ||
{ | ||
fieldNames ~= __traits(identifier, field); | ||
} | ||
static bool canFind(string[] haystack, string needle) | ||
{ | ||
foreach (candidate; haystack) | ||
{ | ||
if (candidate == needle) return true; | ||
} | ||
return false; | ||
} | ||
while (canFind(fieldNames, name)) name ~= "_"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just use |
||
return name; | ||
} (); | ||
struct TailShared | ||
{ | ||
static foreach (i, alias field; S.tupleof) | ||
{ | ||
/* On @trusted: This is casting the field from shared(Foo) | ||
to TailShared!Foo. The cast is safe because the field has | ||
been loaded and is not shared anymore. */ | ||
mixin(" | ||
@trusted @property | ||
ref " ~ __traits(identifier, field) ~ "() | ||
{ | ||
alias R = TailShared!(typeof(field)); | ||
return * cast(R*) &" ~ implName ~ ".tupleof[i]; | ||
} | ||
"); | ||
} | ||
mixin(" | ||
S " ~ implName ~ "; | ||
alias " ~ implName ~ " this; | ||
"); | ||
} | ||
} | ||
else | ||
alias T HeadUnshared; | ||
alias TailShared = S; | ||
} | ||
@safe unittest | ||
{ | ||
// No tail (no indirections) -> fully unshared. | ||
|
||
static assert(is(TailShared!int == int)); | ||
static assert(is(TailShared!(shared int) == int)); | ||
|
||
static struct NoIndir { int i; } | ||
static assert(is(TailShared!NoIndir == NoIndir)); | ||
static assert(is(TailShared!(shared NoIndir) == NoIndir)); | ||
|
||
// Tail can be independently shared or is already -> tail-shared. | ||
|
||
static assert(is(TailShared!(int*) == shared(int)*)); | ||
static assert(is(TailShared!(shared int*) == shared(int)*)); | ||
static assert(is(TailShared!(shared(int)*) == shared(int)*)); | ||
|
||
static assert(is(TailShared!(int[]) == shared(int)[])); | ||
static assert(is(TailShared!(shared int[]) == shared(int)[])); | ||
static assert(is(TailShared!(shared(int)[]) == shared(int)[])); | ||
|
||
static struct S1 { shared int* p; } | ||
static assert(is(TailShared!S1 == S1)); | ||
static assert(is(TailShared!(shared S1) == S1)); | ||
|
||
static struct S2 { shared(int)* p; } | ||
static assert(is(TailShared!S2 == S2)); | ||
static assert(is(TailShared!(shared S2) == S2)); | ||
|
||
// Tail follows shared-ness of head -> fully shared. | ||
|
||
static class C { int i; } | ||
static assert(is(TailShared!C == shared C)); | ||
static assert(is(TailShared!(shared C) == shared C)); | ||
|
||
/* However, structs get a wrapper that has getters which cast to | ||
TailShared. */ | ||
|
||
static struct S3 { int* p; int _impl; int _impl_; int _impl__; } | ||
static assert(!is(TailShared!S3 : S3)); | ||
static assert(is(TailShared!S3 : shared S3)); | ||
static assert(is(TailShared!(shared S3) == TailShared!S3)); | ||
|
||
static struct S4 { shared(int)** p; } | ||
static assert(!is(TailShared!S4 : S4)); | ||
static assert(is(TailShared!S4 : shared S4)); | ||
static assert(is(TailShared!(shared S4) == TailShared!S4)); | ||
} | ||
} | ||
|
||
|
@@ -76,10 +181,10 @@ version( CoreDdoc ) | |
* Returns: | ||
* The result of the operation. | ||
*/ | ||
HeadUnshared!(T) atomicOp(string op, T, V1)( ref shared T val, V1 mod ) pure nothrow @nogc @safe | ||
TailShared!T atomicOp(string op, T, V1)( ref shared T val, V1 mod ) pure nothrow @nogc @safe | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't do ops on classes, so this is more permissive than before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't follow. What am I allowing that wasn't allowed before?
I can put By the way, are the implicit conversion rules on shared sound? D lets me copy large types (that can't be loaded/stored atomically) to and from shared. Shouldn't that be disallowed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, so atomicOp cannot possibly work correctly with classes, right? The current code, using HeadUnshared, fails on the cas call. Looks like a lucky coincidence that the atomicOp call is rejected. With TailShared it goes through if and only if the class has both a shared and an unshared opOpCall, because the template constraint checks the unshared case, and the actual operation is done on the result of TailShared, which is shared for classes. The same thing happens with structs that have both variants of opOpAssign. So, it's not just classes. atomicOp can't work with types that have an inseparable head and tail. Can't call an unshared opOpAssign, because it would assume that the referenced data is unshared. Shouldn't call a shared opOpAssign, because atomicOp would just add overhead then, and the shared opOpAssign may be implemented with locks instead of atomic operations. So, add a constraint |
||
if( __traits( compiles, mixin( "*cast(T*)&val" ~ op ~ "mod" ) ) ) | ||
{ | ||
return HeadUnshared!(T).init; | ||
return TailShared!T.init; | ||
} | ||
|
||
|
||
|
@@ -119,9 +224,9 @@ version( CoreDdoc ) | |
* Returns: | ||
* The value of 'val'. | ||
*/ | ||
HeadUnshared!(T) atomicLoad(MemoryOrder ms = MemoryOrder.seq,T)( ref const shared T val ) pure nothrow @nogc @safe | ||
TailShared!T atomicLoad(MemoryOrder ms = MemoryOrder.seq,T)( ref const shared T val ) pure nothrow @nogc @safe | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should get a deprecated overload with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But calling atomicLoad on a class isn't being deprecated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because |
||
{ | ||
return HeadUnshared!(T).init; | ||
return TailShared!T.init; | ||
} | ||
|
||
|
||
|
@@ -166,7 +271,7 @@ version( CoreDdoc ) | |
else version( AsmX86_32 ) | ||
{ | ||
// Uses specialized asm for fast fetch and add operations | ||
private HeadUnshared!(T) atomicFetchAdd(T)( ref shared T val, size_t mod ) pure nothrow @nogc @safe | ||
private TailShared!(T) atomicFetchAdd(T)( ref shared T val, size_t mod ) pure nothrow @nogc @safe | ||
if( T.sizeof <= 4 ) | ||
{ | ||
size_t tmp = mod; | ||
|
@@ -187,13 +292,13 @@ else version( AsmX86_32 ) | |
return cast(T)tmp; | ||
} | ||
|
||
private HeadUnshared!(T) atomicFetchSub(T)( ref shared T val, size_t mod ) pure nothrow @nogc @safe | ||
private TailShared!(T) atomicFetchSub(T)( ref shared T val, size_t mod ) pure nothrow @nogc @safe | ||
if( T.sizeof <= 4) | ||
{ | ||
return atomicFetchAdd(val, -mod); | ||
} | ||
|
||
HeadUnshared!(T) atomicOp(string op, T, V1)( ref shared T val, V1 mod ) pure nothrow @nogc | ||
TailShared!T atomicOp(string op, T, V1)( ref shared T val, V1 mod ) pure nothrow @nogc | ||
if( __traits( compiles, mixin( "*cast(T*)&val" ~ op ~ "mod" ) ) ) | ||
in | ||
{ | ||
|
@@ -213,7 +318,7 @@ else version( AsmX86_32 ) | |
op == "==" || op == "!=" || op == "<" || op == "<=" || | ||
op == ">" || op == ">=" ) | ||
{ | ||
HeadUnshared!(T) get = atomicLoad!(MemoryOrder.raw)( val ); | ||
TailShared!T get = atomicLoad!(MemoryOrder.raw)( val ); | ||
mixin( "return get " ~ op ~ " mod;" ); | ||
} | ||
else | ||
|
@@ -233,7 +338,7 @@ else version( AsmX86_32 ) | |
op == "%=" || op == "^^=" || op == "&=" || op == "|=" || | ||
op == "^=" || op == "<<=" || op == ">>=" || op == ">>>=" ) // skip "~=" | ||
{ | ||
HeadUnshared!(T) get, set; | ||
TailShared!T get, set; | ||
|
||
do | ||
{ | ||
|
@@ -391,7 +496,7 @@ else version( AsmX86_32 ) | |
} | ||
|
||
|
||
HeadUnshared!(T) atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)( ref const shared T val ) pure nothrow @nogc @safe | ||
TailShared!T atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)( ref const shared T val ) pure nothrow @nogc @safe | ||
if(!__traits(isFloating, T)) | ||
{ | ||
static assert( ms != MemoryOrder.rel, "invalid MemoryOrder for atomicLoad()" ); | ||
|
@@ -654,7 +759,7 @@ else version( AsmX86_32 ) | |
else version( AsmX86_64 ) | ||
{ | ||
// Uses specialized asm for fast fetch and add operations | ||
private HeadUnshared!(T) atomicFetchAdd(T)( ref shared T val, size_t mod ) pure nothrow @nogc @trusted | ||
private TailShared!(T) atomicFetchAdd(T)( ref shared T val, size_t mod ) pure nothrow @nogc @trusted | ||
if( __traits(isIntegral, T) ) | ||
in | ||
{ | ||
|
@@ -681,13 +786,13 @@ else version( AsmX86_64 ) | |
return cast(T)tmp; | ||
} | ||
|
||
private HeadUnshared!(T) atomicFetchSub(T)( ref shared T val, size_t mod ) pure nothrow @nogc @safe | ||
private TailShared!(T) atomicFetchSub(T)( ref shared T val, size_t mod ) pure nothrow @nogc @safe | ||
if( __traits(isIntegral, T) ) | ||
{ | ||
return atomicFetchAdd(val, -mod); | ||
} | ||
|
||
HeadUnshared!(T) atomicOp(string op, T, V1)( ref shared T val, V1 mod ) pure nothrow @nogc | ||
TailShared!T atomicOp(string op, T, V1)( ref shared T val, V1 mod ) pure nothrow @nogc | ||
if( __traits( compiles, mixin( "*cast(T*)&val" ~ op ~ "mod" ) ) ) | ||
in | ||
{ | ||
|
@@ -707,7 +812,7 @@ else version( AsmX86_64 ) | |
op == "==" || op == "!=" || op == "<" || op == "<=" || | ||
op == ">" || op == ">=" ) | ||
{ | ||
HeadUnshared!(T) get = atomicLoad!(MemoryOrder.raw)( val ); | ||
TailShared!T get = atomicLoad!(MemoryOrder.raw)( val ); | ||
mixin( "return get " ~ op ~ " mod;" ); | ||
} | ||
else | ||
|
@@ -727,7 +832,7 @@ else version( AsmX86_64 ) | |
op == "%=" || op == "^^=" || op == "&=" || op == "|=" || | ||
op == "^=" || op == "<<=" || op == ">>=" || op == ">>>=" ) // skip "~=" | ||
{ | ||
HeadUnshared!(T) get, set; | ||
TailShared!T get, set; | ||
|
||
do | ||
{ | ||
|
@@ -928,7 +1033,7 @@ else version( AsmX86_64 ) | |
} | ||
|
||
|
||
HeadUnshared!(T) atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)( ref const shared T val ) pure nothrow @nogc @safe | ||
TailShared!T atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)( ref const shared T val ) pure nothrow @nogc @safe | ||
if(!__traits(isFloating, T)) | ||
{ | ||
static assert( ms != MemoryOrder.rel, "invalid MemoryOrder for atomicLoad()" ); | ||
|
@@ -1267,7 +1372,7 @@ else version( AsmX86_64 ) | |
// floats and doubles to ints and longs, atomically loads them, then puns | ||
// them back. This is necessary so that they get returned in floating | ||
// point instead of integer registers. | ||
HeadUnshared!(T) atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)( ref const shared T val ) pure nothrow @nogc @trusted | ||
TailShared!T atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)( ref const shared T val ) pure nothrow @nogc @trusted | ||
if(__traits(isFloating, T)) | ||
{ | ||
static if(T.sizeof == int.sizeof) | ||
|
@@ -1548,4 +1653,45 @@ version( unittest ) | |
atomicOp!"-="( c, d ); | ||
assert(c == 1); | ||
} | ||
|
||
pure nothrow @safe unittest // issue 16230 | ||
{ | ||
shared int i; | ||
static assert(is(typeof(atomicLoad(i)) == int)); | ||
|
||
shared int* p; | ||
static assert(is(typeof(atomicLoad(p)) == shared(int)*)); | ||
|
||
shared int[] a; | ||
static assert(is(typeof(atomicLoad(a)) == shared(int)[])); | ||
|
||
static struct S { int* _impl; } | ||
shared S s; | ||
static assert(is(typeof(atomicLoad(s)) : shared S)); | ||
static assert(is(typeof(atomicLoad(s)._impl) == shared(int)*)); | ||
auto u = atomicLoad(s); | ||
assert(u._impl is null); | ||
u._impl = new shared int(42); | ||
assert(atomicLoad(*u._impl) == 42); | ||
|
||
static struct S2 { S s; } | ||
shared S2 s2; | ||
static assert(is(typeof(atomicLoad(s2).s) == TailShared!S)); | ||
|
||
static struct S3 { size_t head; int* tail; } | ||
shared S3 s3; | ||
static if (__traits(compiles, atomicLoad(s3))) | ||
{ | ||
static assert(is(typeof(atomicLoad(s3).head) == size_t)); | ||
static assert(is(typeof(atomicLoad(s3).tail) == shared(int)*)); | ||
} | ||
|
||
static class C { int i; } | ||
shared C c; | ||
static assert(is(typeof(atomicLoad(c)) == shared C)); | ||
|
||
static struct NoIndirections { int i; } | ||
shared NoIndirections n; | ||
static assert(is(typeof(atomicLoad(n)) == NoIndirections)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spurious code block delimiter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Do we have a bugzilla for the missing line number in the error message "Error: unmatched --- in DDoc comment"?