Extract atomic platform specific implementation into an implementation file

[TurkeyMan]

Pulled all the noise out into core.internal.atomic, which GDC/LDC can easily replace with intrinsics, and can be a reference for DMD intrinsics if that work happens.

core.atomic becomes platform agnostic.

[dlang-bot]

Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2739"

[TurkeyMan]

Why can't it find the file? What have I missed?

[thewilsonator]

This is green, good to go?

[TurkeyMan]

I'm just gonna add another patch, rather than open another PR.

[TurkeyMan]

Almost there... and I think this'll be the last of my atomic mischief.

[thewilsonator]

Enforce whitespace before opening parenthesis
grep -nrE "\<(for|foreach|foreach_reverse|if|while|switch|catch|version)\(" $(find src -name '*.d') ; test $? -eq 1
src/core/internal/atomic.d:81:        else version(D_InlineAsm_X86_64)

[TurkeyMan]

Okay, I'm done... when it's green it's finished.

[thewilsonator]

../druntime/import/core/internal/atomic.d(237): Error: cannot modify `shared const` expression `*dest`
../druntime/import/core/atomic.d(148): Error: template instance `core.internal.atomic.atomicStore!(cast(MemoryOrder)2, shared(const(Logger)))` error instantiating
std/experimental/logger/core.d(1691):        instantiated from here: `atomicStore!(cast(MemoryOrder)2, Logger, shared(Logger))`
../druntime/import/core/atomic.d(148): Error: function `core.internal.atomic.atomicStore!(cast(MemoryOrder)2, shared(const(Logger))).atomicStore(shared(const(Logger))* dest, shared(const(Logger)) value)` is not callable using argument types `(Logger*, shared(Logger))`
../druntime/import/core/atomic.d(148):        cannot pass argument `cast(Logger*)&val` of type `Logger*` to parameter `shared(const(Logger))* dest`
../druntime/import/core/internal/atomic.d(237): Error: cannot modify `shared const` expression `*dest`
../druntime/import/core/atomic.d(148): Error: template instance `core.internal.atomic.atomicStore!(cast(MemoryOrder)2, shared(const(Logger)))` error instantiating
std/experimental/logger/core.d(1691):        instantiated from here: `atomicStore!(cast(MemoryOrder)2, Logger, shared(Logger))`
../druntime/import/core/atomic.d(148): Error: function `core.internal.atomic.atomicStore!(cast(MemoryOrder)2, shared(const(Logger))).atomicStore(shared(const(Logger))* dest, shared(const(Logger)) value)` is not callable using argument types `(Logger*, shared(Logger))`
../druntime/import/core/atomic.d(148):        cannot pass argument `cast(Logger*)&val` of type `Logger*` to parameter `shared(const(Logger))* dest`

Hmm, phobos problems.

[TurkeyMan]

It's weird; why doesn't it show the instantiation context :/
Also, why don't all the build machines show the same error?

[TurkeyMan]

I mean, that just looks like a bug in phobos that I've exposed... how can anyone expect to call atomicStore on a const object?

[thewilsonator]

Why is that being inferred as const at all? The offending line
https://github.com/dlang/phobos/blob/master/std/experimental/logger/core.d#L1691
has both arguments as non-const https://github.com/dlang/phobos/blob/master/std/experimental/logger/core.d#L1620.

[TurkeyMan]

Mmmm, I can't find any evidence of const feeding into this :/

[thewilsonator]

Indeed, it looks very strange.

[TurkeyMan]

I seem to have made it worse... I have no idea what's going on :/
I can't even repro this locally.

[TurkeyMan]

I think it might have something to do with this:
https://issues.dlang.org/show_bug.cgi?id=20138

[thewilsonator]

Seems likely.

[TurkeyMan]

Everything always has to be so hard! >_<

[aG0aep6G]

I think it might have something to do with this:
https://issues.dlang.org/show_bug.cgi?id=20138

I might have a fix: dlang/dmd#10317

[aG0aep6G]

As far as I can tell, this inout stuff isn't necessary.

Going from const T to T is fine if it's a copy. Only the (copied) head of the type is made mutable.

Example:

import core.atomic;
shared const int* x;
pragma(msg, typeof(atomicLoad(x))); /* shared(const(int))* */

Note that the referenced int is still const.

[TurkeyMan]

I'd like to think that's true, I didn't write this and it's a huge mess, but if I change anything at all it all breaks spectacularly.

I think some of the downstream code is maladjusted to this complexity, so it would be good to tidy up generally, but that's a different PR.

[TurkeyMan]

The worst part here is that atomics shouldn't be shared at all... unless the user passed a shared thing, in which case it should work properly with that.

[aG0aep6G]

I didn't write this and it's a huge mess, but if I change anything at all it all breaks spectacularly.

You added inout which is what I'm commenting on. I'm saying you should be able to leave it as it is (const instead of inout).

I expect that the DMD bug you filed is the real issue, and I hope that we can fix it.

[TurkeyMan]

The T in the code should contain and propagate the qualifiers, I'd like to get the explicit qualifiers out of the code.

[TurkeyMan]

This should just return a copy of T, whatever T is, and comply with whether T is copyable or not.
Theres this whole TailShared machine in core.atomic; I have no idea what it does, or why it's there, but it's very complicated and seems to make problems.

[aG0aep6G]

I'm the author of TailShared. It's supposed to be an improvement over the previous HeadUnshared which would strip shared to eagerly (#1605).

I wouldn't mind if you got rid of it, changing the return type to simply T. But changing return types is always risky business, and shared is still underspecified, as far as I know. So that's probably not going to be easy.

[TurkeyMan]

Oh perfect!

Well, I need to understand what it does and why it's there before removing it... changing API in here breaks lots of client code, so I haven't been able to make much simplification.
But I also think some of the unittests have bugs which are there to hide some bugs in the implementation... like a double-negative situation.

I had trouble with it because it wraps the type and changes the calling convention. It generates extra code in loadAtomic by doing a copy from one calling convention to another.
Slice/delegate returns in RAX;RDX, but TailShared returns by stack, so you can see atomicLoad has that thunk in the return statement which is there so that it transfers the return value from one ABI to the other. It would be better if it wasn't there... but like I say, I don't understand the problem it was trying to solve.

I've been working with Andrei and Walter and friends on what shared needs to be in the future... we should talk, do you have IM?
Have you been contributing to core.atomic a lot? (Your github profile gives me no idea who you are)

[aG0aep6G]

Well, I need to understand what it does and why it's there before removing it... changing API in here breaks lots of client code, so I haven't been able to make much simplification.

TailShared does (or is supposed to do) what it says in the comment: "Construct a type with a shared tail, and if possible with an unshared head."

shared(T*) becomes shared(T)*. As for why that's done, I suppose it's because dealing with shared is cumbersome, so it's removed as much as possible. But I didn't add that, I just refined it. I can't say for sure what the original motivation was.

T* also becomes shared(T)*. If I remember correctly, this was just easier to implement and/or use, because atomicLoad and friends always operate on shared types anyway.

shared SomeClass remains shared SomeClass. This is the whole improvement over HeadUnshared which would remove shared.

But I also think some of the unittests have bugs which are there to hide some bugs in the implementation... like a double-negative situation.

Which tests do you think are wrong?

I had trouble with it because it wraps the type and changes the calling convention. It generates extra code in loadAtomic by doing a copy from one calling convention to another.
Slice/delegate returns in RAX;RDX, but TailShared returns by stack, so you can see atomicLoad has that thunk in the return statement which is there so that it transfers the return value from one ABI to the other. It would be better if it wasn't there... but like I say, I don't understand the problem it was trying to solve.

Being a type template, TailShared doesn't return anything in the sense of calling conventions. And (unfortunately) I can't shed light on what atomicLoad does. I didn't write it and I know next to nothing about atomic operations, memory fences, etc. Sorry.

I've been working with Andrei and Walter and friends on what shared needs to be in the future... we should talk, do you have IM?

You can email me at gmail, but I don't have anything to offer when it comes to technical details of dealing with shared data. At best I can make amateurish comments on the type system side of things.

Have you been contributing to core.atomic a lot?

No. I'm pretty sure that turning HeadUnshared into TailShared was my most significant contribution here. Might actually be the only one. And that required no actual knowledge of atomics, just of D's type system.

Sorry again if I got your hopes up. I'm not actually knowledgeable about most of this.

[TurkeyMan]

I had trouble with it because it wraps the type and changes the calling convention. It generates extra code in loadAtomic by doing a copy from one calling convention to another.
Slice/delegate returns in RAX;RDX, but TailShared returns by stack, so you can see atomicLoad has that thunk in the return statement which is there so that it transfers the return value from one ABI to the other. It would be better if it wasn't there... but like I say, I don't understand the problem it was trying to solve.

Being a type template, TailShared doesn't return anything in the sense of calling conventions.

Well it does, because the function T fun(), and TailShared!T fun() have a different calling convention in some cases... which is problematic when there's thunk's all over the code. I'm trying to remove all the ugly shit as best I can.

I've made some changes that isolate TailShared to a single overload which operates on that exact case, and tried to narrow it down.
I think it's better now, and the tests pass, but what I think might be better if the user called the function with a TailShared!T manually. They can use it when they want it.

[TurkeyMan]

FFS, someone needs to kill ip-172-31-7-111 already!

[wilzbach]

WHY was this merged with a failing buildkite check???

https://github.com/vibe-d/vibe.d/blob/master/crypto/vibe/crypto/cryptorand.d#L173

[WalterBright]

The buildkite/dmd error:

../../crypto/vibe/crypto/cryptorand.d(173,6): Error: cannot modify `const` expression `p`

The code https://github.com/vibe-d/vibe.d/blob/master/crypto/vibe/crypto/cryptorand.d

auto p = atomicLoad(*cast(const shared GET_RANDOM*) &hasGetRandom);
if (p == GET_RANDOM.UNINITIALIZED)
{
	p = initHasGetRandom() ? GET_RANDOM.AVAILABLE   <= error on this line
					: GET_RANDOM.NOT_AVAILABLE;

[ibuclaw]

This introduced a regression from the original:

T.sizeof <= 16

In that now atomics fail on ix86 and x32 platforms where 128-bit atomics is supported in a libatomic library (or natively with a lock cmpxchg16b on x32)