Renovate core.atomic, improve and simplify the constraints on functions #2745

TurkeyMan · 2019-08-20T03:10:12Z

Add missing features to the API.
Most functions are slightly more efficient.
Concentrate more specific work into less surface area.

Fixes Issue 20107 - core.atomic : Memory order is missing keys
Fixes Issue 20106 - core.atomic : atomicFence doesn't accept MemoryOrder
Fixes Issue 20105 - core.atomic 'cas' function is incomplete
Fixes Issue 18643 - Compiling error when combining CAS and numeric literal.
Fixes Issue 15007 - core.atomic match C++11
Fixes Issue 8831 - core.atomic: add compare-and-swap function with other result type

dlang-bot · 2019-08-20T03:10:13Z

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

Auto-close	Bugzilla	Severity	Description
✓	8831	enhancement	core.atomic: add compare-and-swap function with other result type
✓	15007	enhancement	core.atomic match C++11
✓	18643	minor	Compiling error when combining CAS and numeric literal.
✓	20105	enhancement	core.atomic 'cas' function is incomplete
✓	20106	enhancement	core.atomic : atomicFence doesn't accept MemoryOrder
✓	20107	enhancement	core.atomic : Memory order is missing keys

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#2745"

TurkeyMan · 2019-08-20T05:31:44Z

That buildkite error is problematic... it's in master now.

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

I don't know how that ever worked; but it looks like I've broken it.
It loads a const object, assigns it to auto p, and then tries to assign to p... which should be const, because auto?

TurkeyMan · 2019-08-20T05:43:04Z

See, the old atomicLoad accepted a shared const T, and returned a T, which means the const was lost through the atomicLoad call... which is REALLY bad.
I fixed that... and now this issue where vibe.d is trying to assign to a const thing returned from atomicLoad shows up.

I think this is acceptable breakage, because this fixes a heinous const violation bug.

wilzbach · 2019-08-20T05:45:41Z

I don't know how that ever worked; but it looks like I've broken it.

Yes -> #2739 (comment)

I think this is acceptable breakage, because this fixes a heinous const violation bug.

You just broke ten of the top 50 dub packages. It would have been okay if Vibe.d was fixed before.

TurkeyMan · 2019-08-20T06:05:40Z

I can hack the bug back in there... but it's a terrible bug, no matter how you slice it.

TurkeyMan · 2019-08-20T06:06:19Z

Also, I didn't break it, I don't have merge rights!

Geod24 · 2019-08-20T06:08:22Z

I can hack the bug back in there... but it's a terrible bug, no matter how you slice it.

Surely is a terrible code, but breaking the ability to compile older Vibe.d with newer release is terrible for the ecosystem.

TurkeyMan · 2019-08-20T06:12:08Z

vibe-d/vibe.d#2348

I'll give them a day or 2 to respond, or I'll commit the bug back in there.
We missed 2.088, so we're months away from this code being released.

TurkeyMan · 2019-08-20T06:13:33Z

This PR fixes 5 issues, why does dlang-bot only mention one?

TurkeyMan · 2019-08-20T06:14:55Z

compile older Vibe.d with newer release

What do you suggest? I don't think tremendously bad bugs in druntime are acceptable... :/

wilzbach · 2019-08-20T06:20:24Z

Dlang-bot only looks at commit messages.

TurkeyMan · 2019-08-20T06:20:37Z

All 5 are in the commit message.

wilzbach · 2019-08-20T06:21:50Z

we're months away from this code being released.

The CI should never be red though :/

TurkeyMan · 2019-08-20T06:22:47Z

The CI should never be red though :/

I can hack the bug back in there... although I reckon they'll fix it in vibe.d by tomorrow.

Does one of you wanna push a patch to vibe.d now? Replace auto with GET_RANDOM... I can't do it, it builds with dub, I don't know how to work their weird build systems ;)

TurkeyMan · 2019-08-20T06:30:59Z

Why doesn't dlang-bot see the issues that are fixed? They won't appear in the changelog unless they're referenced here...

TurkeyMan · 2019-08-20T06:37:45Z

Oh well, I guess the changelog is just missing some great info!

Why would dlang-bot only look at the commit messages? He should look at the PR description... I always forget to add text to the commit message, and then I need to cause a full rebuild of the CI to amend the message... that's terribly wasteful.

wilzbach · 2019-08-20T07:03:53Z

Why would dlang-bot only look at the commit messages? He should look at the PR description

Because only the commit messages are available when you look at the git log between two releases.

although I reckon they'll fix it in vibe.d by tomorrow.

No, I doubt it will get fixed within the two months. Vibe.d isn't really maintained + you need to release the change in Vibe.d and then upgrade all libraries/apps that use it + release them.

wilzbach · 2019-08-20T07:05:42Z

Why doesn't dlang-bot see the issues that are fixed?

Looks like it only looks at the first "fixes" string. However, there's this:

https://github.com/dlang/dlang-bot#referencing-multiple-issues

TurkeyMan · 2019-08-20T07:26:20Z

No, I doubt it will get fixed within the two months.

So, should I put the bug back? Should we give it a short while and put it back later?
I think anyone of you who can build vibe.d could make the PR in about 3 minutes...

wilzbach · 2019-08-20T07:35:47Z

So, should I put the bug back?

Unfortunately, I think you have to :/

I think anyone of you who can build vibe.d could make the PR in about 3 minutes...

Yes, but you also need to release a new version which only happens every 2-3 months and afterwards update all the other projects that depend on Vibe.d.
So I guess the only thing we can do is revert it for now, and open a revert of that revert, s.t. it's not forgotten and can be merged once Vibe.d is updated.

Geod24 · 2019-08-21T20:11:56Z

What do you suggest? I don't think tremendously bad bugs in druntime are acceptable... :/

According to #2745 (comment) the "tremendous" part was quite exaggerated.

As mentioned in the Vibe.d PR, the bug you fixed didn't have any impact on the user. But fixing it means that:

Other projects not depending on Vibe.d could use this pattern and be broken by the change
People updating to newer compilers directly loose the ability to bisect on Vibe.d
We force anyone updating to update to the newest compiler to update their Vibe.d dependency
We potentially force any library depending on Vibe.d to release a new version

Don't get me wrong, this needs to be fixed eventually, but fixing it only makes life worse for users of druntime at the moment, not better. So having everything fixed beforehand and giving it at least a release to propagate is, I think, not too much to ask for.

TurkeyMan · 2019-08-21T20:22:09Z

According to #2745 (comment) the "tremendous" part was quite exaggerated.

Well, it's just applicable to class and struct, but not to int* or int[] as I initially thought... so the bad-ness is identical, it just affects less things that I initially thought.

I'm not good with a function that silently casts const away from a struct.

So having everything fixed beforehand and giving it at least a release to propagate is, I think, not too much to ask for.

I buy your arguments, which is why I reintroduced it to this PR. Fix is also in isolation here: #2753
Someone should just click merge on that immediately, irrespective if/what we decide to do elsewise.

TurkeyMan · 2019-08-22T03:07:20Z

I separated the whitespace change and casWeak into separate patches.

TurkeyMan · 2019-08-22T03:18:47Z

And split it some more for good measure.

n8sh · 2019-08-23T22:06:31Z

I think this is acceptable breakage, because this fixes a heinous const violation bug.

The old behavior was not a bug because it only could occur for types without indirections. It is legal to convert const int to int. You almost never want to return a const scalar because it causes problems with auto, like happened to the code that broke in vibe.d.

aG0aep6G · 2019-08-23T22:18:38Z

The old behavior was not a bug

There is a bug. To clear up the confusion (to which I contributed earlier), here is an example:

import core.atomic;
struct S { int* p; }
void main()
{
    immutable int* p = new immutable int(42);
    auto s = immutable S(p);
    auto x = atomicLoad(s);
    pragma(msg, typeof(x.p)); /* shared(int)* - uh oh */
    *x.p = 13; /* mutating an immutable int */
}

The problem is that atomicLoad strips away const from structs and classes. There is no issue with scalars, pointers, arrays. It's just structs and classes.

TurkeyMan · 2019-08-25T20:19:36Z

Is there something wrong with this?

src/core/atomic.d

lesderid · 2019-08-25T20:41:21Z

src/core/atomic.d

@@ -539,9 +666,9 @@ private
    }

    // TODO: it'd be nice if we had @trusted scopes; we could remove this...


Could be replaced with a @trusted lambda (something like (() @trusted => casWeak(&val, &get, set))())? Not sure if that's better here though.

Yeah, possibly the lamest pattern D ever manifested. I don't want to put another another another function call around an asm opcode. There's already 3-4 more than enough.

dnadlinger · 2019-08-25T22:35:19Z

src/core/atomic.d

 {
-    static if ( __traits(isFloating, T) )
+    // resolve implicit conversions
+    T arg = newval;


Hmm, doesn't this end up @trusting potentially arbitrary user code?

That's an interesting point.
It's invalid for any type that interacts with atomic to have non-trivial copy anyway (because copy semantics are bypassed by the atomics)... so I can assert that in the constraint and then we can have confidence this is safe.
I actually thought about that while doing this (not for this exact reason), but it's stuck on https://issues.dlang.org/show_bug.cgi?id=19902, which is a really bad regression, and it's been sitting there for months!
I figured I'd add that restriction whenever this is solved.

I put the assert.
I'm not sure if the same assert should be everywhere though; atomicStore is definitely a copying assignment, whereas exchange and cas are both actually swap operations, which are move operations, so they don't actually interact with copy semantics.

@TurkeyMan: This line causes a regression for:

struct S { int x; alias x this; } shared S s; s = 0x1234_5678; atomicStore(s, 0); // => Error: cannot implicitly convert expression `newval` of type `int` to `S` assert(s.x == 0);

Note that the removed __traits( compiles, { val = newval; } ) constraint doesn't play a role due to alias this magic, i.e., assignment is fine, but construction via implicit conversion fails. An explicit cast would work.

interesting.
so you're saying the semantic difference is that i changed a copy to a construction?

is this real code? does this occur somewhere?

It's only an additional LDC test (https://github.com/ldc-developers/druntime/blob/77066d7d399b0da1e94699da37436066589898ef/src/core/atomic.d#L1073-L1081), so I hope code like this isn't in the wild.

I'm not sure how to handle it best, i.e., whether such code should be disallowed or whether an explicit cast in atomicStore is justified (or even T arg = void; arg = newval;?).

Most functions are slightly more efficient. Concentrate more specific work into less surface area. Fixes issues 20107, 18643, 15007, 8831

Fixes issue 20105

Fixes issue 20106

TurkeyMan · 2019-08-31T09:12:41Z

2 weeks feels like a long time to twiddle my thumbs... I could be programming, but I'm playing video games instead ;)

TurkeyMan requested review from ibuclaw and WalterBright as code owners August 20, 2019 03:10

TurkeyMan force-pushed the upgrade_core_atomic branch from f267ade to 5295e4e Compare August 20, 2019 03:18

thewilsonator approved these changes Aug 20, 2019

View reviewed changes

TurkeyMan force-pushed the upgrade_core_atomic branch from 5295e4e to 230dceb Compare August 20, 2019 05:30

TurkeyMan force-pushed the upgrade_core_atomic branch from 230dceb to 4255664 Compare August 20, 2019 06:01

dlang-bot added the Enhancement New functionality label Aug 20, 2019

TurkeyMan mentioned this pull request Aug 20, 2019

Const violation issue. vibe-d/vibe.d#2348

Closed

TurkeyMan force-pushed the upgrade_core_atomic branch from 4255664 to 7ad46d3 Compare August 20, 2019 06:36

TurkeyMan force-pushed the upgrade_core_atomic branch from 7ad46d3 to 8d789fa Compare August 20, 2019 07:25

TurkeyMan force-pushed the upgrade_core_atomic branch from 7114eb2 to fcc5dbe Compare August 22, 2019 03:04

TurkeyMan force-pushed the upgrade_core_atomic branch from fcc5dbe to 48749a7 Compare August 22, 2019 03:18

lesderid reviewed Aug 25, 2019

View reviewed changes

src/core/atomic.d Outdated Show resolved Hide resolved

lesderid reviewed Aug 25, 2019

View reviewed changes

dnadlinger reviewed Aug 25, 2019

View reviewed changes

TurkeyMan added 6 commits August 25, 2019 16:11

Fix whitespace.

fcbd229

Renovate core.atomic, improve and simplify the constraints on functions.

1b13903

Most functions are slightly more efficient. Concentrate more specific work into less surface area. Fixes issues 20107, 18643, 15007, 8831

Add MemoryOrder args to cas.

335d84f

Fixes issue 20105

Made atomicOp a bit more efficient.

67aa4f6

Add order to atomicFence.

1bd60a7

Fixes issue 20106

Add casWeak, because that's the one you usually want.

fbf560c

TurkeyMan force-pushed the upgrade_core_atomic branch 2 times, most recently from 7d0c478 to be56fff Compare August 25, 2019 23:16

Fix shared U, U, add assert on no elaborate copy.

a336e4d

TurkeyMan force-pushed the upgrade_core_atomic branch from be56fff to a336e4d Compare August 25, 2019 23:17

thewilsonator added the auto-merge label Aug 31, 2019

dlang-bot merged commit 6fb7f66 into dlang:master Aug 31, 2019

TurkeyMan deleted the upgrade_core_atomic branch August 31, 2019 19:06

kinke mentioned this pull request Oct 18, 2019

Upgrade front-end & libs to v2.089.0-beta.1 ldc-developers/ldc#3192

Merged

thewilsonator mentioned this pull request Apr 21, 2020

[core.atomic] atomicStore for alias this #3052

Closed

n8sh mentioned this pull request Jul 24, 2020

Refactor core.atomic : cas into a two-level template #3095

Merged

thaven mentioned this pull request Feb 22, 2021

core.atomic.cas: add 'const' attribute to local variable 'arg1' #3373

Merged

		@@ -539,9 +666,9 @@ private
		}

		// TODO: it'd be nice if we had @trusted scopes; we could remove this...

Renovate core.atomic, improve and simplify the constraints on functions #2745

Renovate core.atomic, improve and simplify the constraints on functions #2745

Conversation

TurkeyMan commented Aug 20, 2019 • edited Loading

dlang-bot commented Aug 20, 2019 • edited Loading

Bugzilla references

Testing this PR locally

TurkeyMan commented Aug 20, 2019 • edited Loading

TurkeyMan commented Aug 20, 2019 • edited Loading

wilzbach commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

Geod24 commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

wilzbach commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

wilzbach commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019 • edited Loading

TurkeyMan commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

wilzbach commented Aug 20, 2019

wilzbach commented Aug 20, 2019

TurkeyMan commented Aug 20, 2019

wilzbach commented Aug 20, 2019 • edited Loading

Geod24 commented Aug 21, 2019

TurkeyMan commented Aug 21, 2019 • edited Loading

TurkeyMan commented Aug 22, 2019

TurkeyMan commented Aug 22, 2019

n8sh commented Aug 23, 2019

aG0aep6G commented Aug 23, 2019

TurkeyMan commented Aug 25, 2019

lesderid Aug 25, 2019

Choose a reason for hiding this comment

TurkeyMan Aug 25, 2019

Choose a reason for hiding this comment

dnadlinger Aug 25, 2019

Choose a reason for hiding this comment

TurkeyMan Aug 25, 2019 • edited Loading

Choose a reason for hiding this comment

TurkeyMan Aug 25, 2019

Choose a reason for hiding this comment

kinke Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

TurkeyMan Oct 18, 2019

Choose a reason for hiding this comment

kinke Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

TurkeyMan commented Aug 31, 2019

TurkeyMan commented Aug 20, 2019 •

edited

Loading

dlang-bot commented Aug 20, 2019 •

edited

Loading

TurkeyMan commented Aug 20, 2019 •

edited

Loading

TurkeyMan commented Aug 20, 2019 •

edited

Loading

TurkeyMan commented Aug 20, 2019 •

edited

Loading

wilzbach commented Aug 20, 2019 •

edited

Loading

TurkeyMan commented Aug 21, 2019 •

edited

Loading

TurkeyMan Aug 25, 2019 •

edited

Loading

kinke Oct 18, 2019 •

edited

Loading

kinke Oct 18, 2019 •

edited

Loading