Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

fix issue 16230 - core.atomic.atomicLoad removes shared from aggregat… #1605

Merged
merged 13 commits into from Sep 1, 2017
Merged

fix issue 16230 - core.atomic.atomicLoad removes shared from aggregat… #1605

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7efd0fe
fix issue 16230 - core.atomic.atomicLoad removes shared from aggregat…
aG0aep6G Jul 3, 2016
ee8a3aa
HeadUnshared -> TailShared
aG0aep6G Jul 8, 2016
6a96fbe
add assert message
aG0aep6G Jul 22, 2017
cbe5e3f
fixup! add assert message
aG0aep6G Jul 24, 2017
c87a1b9
add changelog entry for issue 16230
aG0aep6G Jul 30, 2017
0b1b6c8
fixup! HeadUnshared -> TailShared
aG0aep6G Aug 7, 2017
eab4c63
fixup! add changelog entry for issue 16230
aG0aep6G Aug 7, 2017
6a6051d
move declaration of S to where it's used
aG0aep6G Aug 20, 2017
612f502
@safe unittest
aG0aep6G Aug 20, 2017
2a0187e
wrapper for structs that has getters which cast to TailShared
aG0aep6G Aug 20, 2017
0f2b461
update changelog entry for struct wrapper
aG0aep6G Aug 20, 2017
6178e6f
implement a simple `canFind` locally
aG0aep6G Aug 20, 2017
927b48b
use __traits(compiles, ...) instead of has64BitCAS/has128BitCAS
aG0aep6G Aug 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 34 additions & 0 deletions changelog/atomicLoad-returns-shared.dd
View file
Open in desktop
@@ -0,0 +1,34 @@
`core.atomic.atomicLoad` now returns a `shared` result 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.

`atomicLoad` now leaves the `shared` qualifier on when loading such types
(classes, structs with pointer fields). Head and tail cannot be qualified
independently, so the head remains typed as `shared` even though it is being
copied from shared to thread-local memory.

In order to access the head, the `shared` qualifier can safely be casted away
from the result of `atomicLoad`. It is not safe to access the tail this way.

Example:
----
struct S { int head; int* tailPointer; }

shared int tail = 1;
auto shs = shared S(2, &tail);

void main()
{
import core.atomic : atomicLoad;
shared S s = atomicLoad(shs);

++(cast(S*) &s).head; // Ok, reading/writing thread-local head.

version (none) int* t = s.tailPointer; // Doesn't compile anymore.
shared(int)* t = s.tailPointer; // Ok, target of the pointer is shared.
}
----
120 changes: 99 additions & 21 deletions src/core/atomic.d
View file
Open in desktop
Expand Up @@ -32,12 +32,66 @@ else

private
{
template HeadUnshared(T)
/* Construct a type with a shared tail, and if possible with an unshared
head. */
template TailShared(T)
{
static if( is( T U : shared(U*) ) )
alias shared(U)* HeadUnshared;
/* Get the shared and unshared variants of T. T may be shared or not.
S is always shared. U is always unshared. */
alias S = shared T;
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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this assert. S is defined as shared T so this static if essentially extracts the T back under the name U. What is the purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S is defined as shared T so this static if essentially extracts the T back under the name U. What is the purpose?

U is different from T when T is shared. T may be shared or not. S is always shared. U is always unshared.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is worth placing in an explanatory comment.

Added a comment.


static if (is(S : U))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances is U different from T?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered above.

alias TailShared = U;
else
alias T HeadUnshared;
alias TailShared = S;
}
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 struct S3 { int* p; }
static assert(is(TailShared!S3 == shared S3));
static assert(is(TailShared!(shared S3) == shared S3));

static struct S4 { shared(int)** p; }
static assert(is(TailShared!S4 == shared S4));
static assert(is(TailShared!(shared S4) == shared S4));

static class C { int i; }
static assert(is(TailShared!C == shared C));
static assert(is(TailShared!(shared C) == shared C));
}
}

Expand Down Expand Up @@ -76,10 +130,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
Copy link
Member

Choose a reason for hiding this comment

The 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.
Can you recheck that TailShared!T is always implicitly convertible to shared T?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Sorry, I don't follow. What am I allowing that wasn't allowed before?

Can you recheck that TailShared!T is always implicitly convertible to shared T?

I can put static assert(is(TailShared : shared T)); at the end of TailShared. Is that what you have in mind?

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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Sorry, I don't follow. What am I allowing that wasn't allowed before?

This function atomicOp can't be used w/ classes, TailShared!T is less restrictive than HeadUnshared!T (IIRC), so that function doesn't need any deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function atomicOp can't be used w/ classes, TailShared!T is less restrictive than HeadUnshared!T (IIRC), so that function doesn't need any deprecation.

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 !is(TailShared!T == shared)? Does that make sense?

if( __traits( compiles, mixin( "*cast(T*)&val" ~ op ~ "mod" ) ) )
{
return HeadUnshared!(T).init;
return TailShared!T.init;
}


Expand Down Expand Up @@ -119,9 +173,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should get a deprecated overload with if (is(T == class)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should get a deprecated overload with if (is(T == class)).

But calling atomicLoad on a class isn't being deprecated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because TailShared!T is more restrictive than HeadUnshared!T for classes, we should keep an overload with HeadUnshared!T for classes and deprecate it.
Not yet sure how to implement the deprecation though.

{
return HeadUnshared!(T).init;
return TailShared!T.init;
}


Expand Down Expand Up @@ -166,7 +220,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;
Expand All @@ -187,13 +241,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
{
Expand All @@ -213,7 +267,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
Expand All @@ -233,7 +287,7 @@ else version( AsmX86_32 )
op == "%=" || op == "^^=" || op == "&=" || op == "|=" ||
op == "^=" || op == "<<=" || op == ">>=" || op == ">>>=" ) // skip "~="
{
HeadUnshared!(T) get, set;
TailShared!T get, set;

do
{
Expand Down Expand Up @@ -391,7 +445,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()" );
Expand Down Expand Up @@ -654,7 +708,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
{
Expand All @@ -681,13 +735,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
{
Expand All @@ -707,7 +761,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
Expand All @@ -727,7 +781,7 @@ else version( AsmX86_64 )
op == "%=" || op == "^^=" || op == "&=" || op == "|=" ||
op == "^=" || op == "<<=" || op == ">>=" || op == ">>>=" ) // skip "~="
{
HeadUnshared!(T) get, set;
TailShared!T get, set;

do
{
Expand Down Expand Up @@ -928,7 +982,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()" );
Expand Down Expand Up @@ -1267,7 +1321,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)
Expand Down Expand Up @@ -1548,4 +1602,28 @@ version( unittest )
atomicOp!"-="( c, d );
assert(c == 1);
}

pure nothrow @safe unittest // issue 16230
{
static struct S { int* p; }
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)[]));

shared S s;
static assert(is(typeof(atomicLoad(s)) == shared S));

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));
}
}