Fix #22860 - DMD emits hardcoded calls to core.atomic

[dkorpel]

Closes #22860

Prompted

Remove loadCoreAtomic and in doAtomicOp use verifyHookExists on a _d_atomicOp template in @dmd/druntime/src/object.d instead

And asked for a changelog entry (which I edited), and simplified the argument list construction myself.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! 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.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

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

dub run digger -- build "master + dmd#22861"

[TurkeyMan]

this string op, T, V1 pattern feels kinda shit at the lowest level to me. it makes it hard to implement; what if the symbol were extern(C) or something? i reckon it'd be better to emit calls to _d_atomicAdd/_d_atomicSub with no template nonsense. Does DMD need the T or V1 even? I think the compiler only emits intrinsic calls to core.atomic in one or 2 places (shared constructors? I can't remember the exact invocations that lead to it). if the uses are narrow and contained, and the types are reliable (probably just a pointer or an int?), why not hard-type just what DMD needs?

[dkorpel]

It is indeed currently only used for += and -= in shared static constructors in templates.

/* Add this prefix to the constructor:
 * ```
 * static int gate;
 * if ([--|++]gate != [0|1]) return; // dependant on ctor/dtor
 * ```
 * or, for shared constructor:
 * ```
 * shared int gate;
 * enum op  = isDestructor ? "-=" : "+=";
 * enum cmp = isDestructor ? 0 : 1;
 * if (core.atomic.atomicOp!op(gate, 1) != cmp) return;
 * ```
 */

Druntime hooks used to be extern(C) functions, but that relied on TypeInfo and hid type errors, so the modern approach is to use templates. These are more flexible, as you can forward them to anything, and they are also pay-as-you go: They don't get linked in if you don't use them.

Template hooks can easily forward to extern(C) functions, the other way around is harder. Your object.d can probably get away with:

extern(C) void _d_atomicAdd(...);
extern(C) void _d_atomicSub(...);

alias _d_atomicOp(string op : "+") = _d_atomicAdd;
alias _d_atomicOp(string op : "-") = _d_atomicSub;

I left the generic string op because @atilaneves has been working on -nosharedaccess and the latest idea was to automatically generate atomic operations for shared(int) etc., so this hook is future proof with that in mind.

That being said, I don't have a strong opinion about this matter. I don't mind changing it to a specialized _d_staticCtorGate hook and crossing the generic _d_atomicOp bridge when we get there.

[TurkeyMan]

it's fine that they be a template in druntime, but the symbol name ideally shouldn't depend on template args...
Your code looks backwards, why not this:

alias _d_atomicAdd = _d_atomicOp(string op : "+");
alias _d_atomicSub = _d_atomicOp(string op : "-");

?
DMD just needs a symbol name; you can make that name resolve to whatever you want. DMD emits calls to _d_atomicAdd/Sub.

[dkorpel]

We used to have a mapping from operators to names for D1-style operator overloading, but we switched to D2 style template arguments. It's less mental overhead (don't need to remember exact names for each operator) and friendlier for generic code (template arg can be used in string mixin).

the symbol name ideally shouldn't depend on template args...

Why not?

Your code looks backwards, why not this:

Because then for N operators, you still need O(N) lines of boilerplate in the best case. With the template arg, you need O(1) lines of boilerplate in the best case and only O(N) in the worst case.

(Best case = forward to D api like core.atomic, worst case = forward to C api like stdatomic.h)

[TurkeyMan]

I just don't see why mangle the symbol names with template noise. It's the same either way; but one is a plain ordinary flat symbol name with no magic, the other requires unnecessary template resolution, and mangles the symbol with the template args.

We determined that N==2; there's add and sub, used in exactly one place. It's just simpler as not-a-template symbol. But yeah, it doesn't really matter.

[dkorpel]

D has type safe mangling for a reason. I've encountered linker errors many times because libphobos.a had a version or cli switch mismatch with the compiler or my code. With an extern(C) symbol the compiler makes an unchecked assumption about the ABI, which causes al kinds of trouble (recall class __monitor 🙂 ).

I prefer druntime hooks as a lazy, header-only D library, but if you prefer an extern(C) library, you can still do that by defining a template alias or adding pragma(mangle). I don't think the template resolution is unnecessary, it offers wanted flexibility.

We determined that N==2

Then I would call the hooks something like _d_staticCtorGate and _d_staticDtorGate instead of giving the guise that druntime's hook api contains atomic operations in general.

[TurkeyMan]

Then I would call the hooks something like [...]

I couldn't agree more!
Making it a single symbol name offers the highest degree of flexibility; including _d_staticCtorGate()() if that's what you prefer.

[TurkeyMan]

Wow, super fast, thanks!

[rikkimax]

Can you please move the hook to core.internal.atomic?

We've got functions for atomicFetchAdd and atomicFetchSub to do what you need there directly without all the wrapper code.

https://github.com/dlang/dmd/blob/master/druntime/src/core/internal/atomic.d

I'd like to make object.d smaller, not larger where possible.

[dkorpel]

Fix #22860 - DMD emits hardcoded calls to core.atomic

Can you please move the hook to core.internal.atomic?