fix issue 16230 - core.atomic.atomicLoad removes shared from aggregat… #1605
Conversation
|
Thanks for your pull request, @aG0aep6G! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
The phobos changes need to be pulled first. But before that, the changes here should be reviewed. |
|
LGTM |
|
Looks goods, could we do it w/o the breakage, e.g. by instantiating a deprecated HeadUnsharedDeprecated for the old conversion? |
src/core/atomic.d
Outdated
| else | ||
| alias T HeadUnshared; | ||
| alias HeadUnshared = shared T; |
There was a problem hiding this comment.
HeadUnshared didn't remove shared from structs before your PR, so the bug description is confusing.
There was a problem hiding this comment.
HeadUnshared didn't remove shared from structs before your PR, so the bug description is confusing.
The problem is that HeadUnshared is never instantiated with fully shared types, only ever with tail-shared ones or completely unshared ones. That happens because atomicLoad itself strips shared already, and only then does it apply HeadUnshared. That means, the instantiations of HeadUnshared have been nops.
Now, shared needs to be re-added at some point to fix the issue with structs. I hadn't really thought this all through before, so my solution was a bit hacky.
I've now renamed HeadUnshared to TailShared and made it so that it constructs a type with a shared tail, ignoring T's given shared-ness. If T has no tail (no indirections), an unshared T is returned. If T's tail cannot be typed independently (aggregates with indirections), a shared T is returned.
Hope it makes more sense now. Tell me if this version is better. I'd squash the commits then.
I don't know how a deprecated atomicLoad could be distinguished from the fixed one. The parameters aren't changing, only the return type does. |
Well, I though we can attach a deprecation message to template instantiations of TailShared that will change their behavior. |
| @@ -76,10 +126,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.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
A deprecation means that the thing should not be used, and that it's going to get removed. But the changed parts should be good to use, and their removal is not planned. If a deprecation stage is a must, then I think we have to add a new function that replaces atomicLoad (and probably the same for atomicOp). Then atomicLoad and atomicOp can be meaningfully deprecated. |
Yes, but it also means that we're going the break code, so |
But then I have no way to call the fixed version. So I can't fix my code properly until the old version is actually removed and the new one takes over. And until then I have to live with the deprecation message. To me that would be more frustrating than just getting an error. |
|
@MartinNowak (or anyone else), it's been about a year. Can we find a way forward for this? This is a safety issue that should be resolved. I'd love to mark the old behavior as The only way I see is to add the new behavior under a new name, and deprecate the old name. Eventually, when the old stuff has been removed, the new stuff can get the old name. If I should go for this, please say so. Otherwise, I think I need it spelled out in detail how you'd do this. |
Another week later. Anyone? |
|
@aG0aep6G I'll take a look shortly. |
src/core/atomic.d
Outdated
| alias S = shared T; | ||
|
|
||
| static if (is(S U == shared U)) {} | ||
| else static assert(false); |
There was a problem hiding this comment.
Needs an error message
Added a message: "S should be shared." But I'm not sure if it's any good. The assert shouldn't ever be hit, no matter what T is. So I'm not sure what expectation to express in the message.
There was a problem hiding this comment.
I would specify that it should never be hit in the error message and the reason why (the latter of it looks like you've already done).
There was a problem hiding this comment.
I would specify that it should never be hit in the error message and the reason why
I've added some more text. I may have gone overboard. If it's too verbose now, I'd appreciate a concrete suggestion.
|
@ZombineDev not sure if you've seen it but this has a corresponding Phobos pull that should be reviewed and pulled as well this one: dlang/phobos#4551 |
|
@aG0aep6G Sorry for taking so long to get back to you, I had limited time. This PR looks good to me, essentially making this module work as I imagined it would before looking at the code. I have only one request - can you please add a changelog entry detailing the before/now difference (HeadUnshared vs TailShared behavior), as this is an important change IMO. |
done |
There was a problem hiding this comment.
Thanks @aG0aep6G, looks great! The only thing left is to resubmit the Phobos PR.
ping |
|
It seems Martin is too busy at the moment. Jonathan, could you take a look? |
There was a problem hiding this comment.
First of all, this still breaks a prominent use-case of core.atomic.
import core.atomic;
shared Object so;
void test()
{
Object o = atomicLoad(so);
}Second, I'm somewhat warily of the recent trend to enforce more usage of shared. While it's good to be correct, shared still isn't a full mature language feature.
Now this PR changes the basic primitive to interact with shared data from one brokenness to a different one, i.e. from overly loose (but possibly correct depending on the programmer) to overly restrictive (but requiring the programmer to add correct casts).
Ultimately I'm worried a bit this will drive users to a cast() attitude, making shared basically useless.
How about we try a different correct approach, implement a HeadUnshared that actually wraps the return type (similar to Rebindable), and has @trusted @property ref getters, that returns the appropriate field types. Mostly structs returned from atomicLoad are just plain data fields.
Implicit conversion via alias this would still return a shared struct/class, to avoid any holes by calling unshared methods.
|
Wrapping a struct is somewhat tricky for private fields. |
I don't think we need to discuss why this code is broken.
I see this the other way around. For someone coming off from TDPL,
I see your point, but I don't agree with you. It's like saying:
Looking at the project tester it looks like this is already the case :( . People either don't use
I also thought about writing a proper typecons for shared (in my case it was
I'm not saying that I'm in principle against this idea. I just think that this need to be very carefully designed, it is out of scope for this PR and could easily be a breaking change to atomicLoad's return type. Probably sounds more like a job for something called |
struct S { int head; int* tailPointer; }
atomicLoad!(shared(S*)) -> shared(S)*
atomicLoad!(shared(S)) -> HeadUnshared!S
struct HeadUnshared!S
{
@property ref int head() { return impl.head; }
@property ref shared(int*) tailPointer() { return impl.tailPointer; } // just use atomic* on the second layer yourself
alias this __impl;
shared S __impl;
}Sketch struct HeadUnshared(T)
{
static foreach (mem; __traits(getMembers, T))
{
alias MemT = typeof(__traits(getMember, T.init, mem));
static if (is(MemT == function))
continue;
static if (hasIndirections!MemT)
mixin("@property ref shared(MemT) " ~ mem ~ "() @safe { return __impl. " ~ mem ~ "; }");
else
mixin("@property ref MemT " ~ mem ~ "() @trusted { return *cast(MemT*)&__impl. " ~ mem ~ "; }");
}
alias this __impl;
shared T __impl;
}
Sure, but this PR is already a breaking change, so it's a good time to look at where we want to go. |
|
As far as I see, the wrapper idea can only work as intended with structs. With a class, atomicLoad only loads the class reference. All fields have to be treated as shared. Doesn't matter if they have indirections or not. I don't see For structs, it has more appeal. Instead of generating properties, we could also generate a struct type that has the same fields, but applies template TailShared2000(T)
{
static if (is(T == struct) && is(TailShared!T == shared))
{
struct TailShared2000
{
private import std.traits : hasIndirections, isAggregateType;
static foreach (i; 0 .. T.tupleof.length)
{
mixin("TailShared!(typeof(T.tupleof[i])) " ~
__traits(identifier, T.tupleof[i]) ~ ";");
//TODO: alignment
}
@property shared(T) _fullyShared()
{
return * cast(shared(T)*) &this;
}
alias _fullyShared this;
}
static assert(TailShared2000.sizeof == T.sizeof);
}
else alias TailShared2000 = TailShared!T;
}
unittest
{
struct S { int head; int* tailPointer; }
TailShared2000!S s;
static assert(!is(typeof(s) == shared));
static assert(is(typeof(s.head) == int));
static assert(is(typeof(s.tailPointer) == shared(int)*));
static assert(!is(typeof(s) : S));
static assert(is(typeof(s) : shared S));
} |
|
Keep in mind that atomic loading a whole struct is rather rare, since at most you can load 2 * size_t.sizeof. |
Sure, for classes it doesn't provide anything, the example above wasn't too useful.
Yes, the use-cases are mostly lock-free data structures and such. They are limited to rather simple data types, hence the wrapping approach might work.
If that still supports implicit conversion to the shared original type (e.g. for method calls), that would be fine as well. Preserving the exact struct layout (alignment) is critical for this. Guess there isn't much that can be done to support protection attributes in such a wrapper, as we cannot add a type to the original module. But as those are only simple data types (mostly POD), it might be acceptable. |
Yeah, and it's a good bit more complicated than generating getters. So I'm backing away from that idea. @ZombineDev, @andralex: Are you on board with returning a wrapper for structs when we can't just strip This is what I have at the moment: template TailShared2001(T)
{
static if (is(T == struct) && is(TailShared!T == shared))
{
struct TailShared2001
{
static foreach (i, alias field; T.tupleof)
{
mixin("
@property ref " ~ __traits(identifier, field) ~ "()
{
alias R = TailShared!(typeof(field));
return * cast(R*) &_impl.tupleof[i];
}
");
}
shared(T) _impl;
alias _impl this;
}
}
else alias TailShared2001 = TailShared!T;
}
unittest
{
struct S { int head; int* tailPointer; }
TailShared2001!S s;
static assert(!is(typeof(s) == shared));
static assert(is(typeof(s.head) == int));
static assert(is(typeof(s.tailPointer) == shared(int)*));
static assert(!is(typeof(s) : S));
static assert(is(typeof(s) : shared S));
} |
Should be |
Can't use phobos's because this is druntime (duh).
|
I've pushed the wrapper. As far as i can tell, private fields are not an issue, because One limitation of the wrapper is with code like this: Unfortunately, this still works with the wrapper: |
I.e., instead of trying to figure out if the load is supported and getting it wrong, just ask the compiler if it works.
That's what |
| } | ||
| return false; | ||
| } | ||
| while (canFind(fieldNames, name)) name ~= "_"; |
There was a problem hiding this comment.
Just use __ here, they are reserved for internal (compiler&std lib) usage. To use a suffix that won't clash w/ phobos, how about __tailSharedImpl.
| When loading a class reference, `atomicLoad` now leaves the `shared` qualifier | ||
| on. | ||
|
|
||
| --- |
There was a problem hiding this comment.
Spurious code block delimiter
There was a problem hiding this comment.
Great catch! Do we have a bugzilla for the missing line number in the error message "Error: unmatched --- in DDoc comment"?
That prints: I.e., it shows that |
…e types too eagerly
https://issues.dlang.org/show_bug.cgi?id=16230
This breaks two places in phobos. Pull request follows.