Fix druntime -preview=nosharedaccess errors by atilaneves · Pull Request #23056 · dlang/dmd

atilaneves · 2026-05-01T08:17:53Z

This makes druntime build and run its unittest target with -preview=nosharedaccess enabled.

dlang-bot · 2026-05-01T08:17:57Z

Thanks for your pull request and interest in making D better, @atilaneves! 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 run digger -- build "master + dmd#23056"

dkorpel · 2026-05-04T12:27:42Z

    T t;
    auto aa1 = [0 : t, 1 : t];
-    assert(T.dtor == 2 && T.postblit == 4);
+    assert(T.postblit == 4);


Why are the .dtor assertions removed?

thewilsonator · 2026-05-04T23:26:56Z

Do ping when this is no longer draft

TurkeyMan · 2026-05-05T06:33:16Z

 private const(ubyte)[] toUbyte_aggregate_ctfe(T)(const return ref scope T val)
 {
    pragma(inline, false);
+


Worth an assert(__ctfe)? Obviously an unprotected cast away is unsafe at runtime.
Incidentally, I frequently find myself wanting a mechanism to enforce a function is only for CTFE. (mostly so that I can have certainty that codegen will never occur)

Approved, just haven't had a merge clicked: #22557

Ah, I'd click it in a heartbeat, but merge conflicts!

TurkeyMan · 2026-05-05T06:49:28Z

+
+            foreach (i; 0 .. val.length)
+            {
+                hash = hashOf(hashOf(atomicLoad!(MemoryOrder.raw)(val.ptr[i])), hash); // double hashing to match TypeInfo.getHash


I think this shows I'm not clear on the goal...
Are we just trying to make formerly accepted-but-broken code continue to compile?
This whole operation is obviously impossible, there's no synchronising primitive surrounding this data. It should rightfully be a compile error which the user needs to deal with.
Are we simply trying to get the runtime to compile with this flag despite being racy/broken?

In this case, I might personally just cast away shared and not bother with the separate branch; but really, we should lean into the compile error, because that's the honest reality. This code is broken, the whole point is that it shouldn't compile.

TurkeyMan · 2026-05-05T06:50:12Z

+
+        foreach (i; 0 .. val.length)
+        {
+            hash = hashOf(hashOf(atomicLoad!(MemoryOrder.raw)(val.ptr[i])), hash); // double hashing because TypeInfo.getHash doesn't allow to pass seed value


TurkeyMan · 2026-05-05T07:05:16Z

        version (Windows)
        {
-            static if (is(Q == Condition))
+            auto self = cast(Condition) this;


This is the right approach, the windows primitives are naturally threadsafe, but just a quick scan of the struct members:

HANDLE m_blockLock; // auto-reset event (now semaphore) HANDLE m_blockQueue; // auto-reset event (now semaphore) Mutex m_assocMutex; // external mutex/CS CRITICAL_SECTION m_unblockLock; // internal mutex/CS int m_numWaitersGone = 0; int m_numWaitersBlocked = 0; int m_numWaitersToUnblock = 0;

Those free int's make me nervous! They're referenced plenty...
I can't see a lock around the whole thing (good!), or carefully orchestrated access patterns. Without working it in depth, at a glance, this suggests to me that this tool is probably not thread-safe...?

TurkeyMan · 2026-05-05T07:09:33Z

    {
        auto mutex      = new shared Mutex;
-        auto condReady  = new shared Condition( mutex );
+        auto condReady  = new shared Condition( atomicLoad(mutex) );


Why would you need to atomic-load a mutex?
Anything that receives a mutex argument should naturally accept a shared mutex.

I'm surprised to see a mutex being handled by-value though, that feels like questionable API design to me, but that's not what we're here to consider.

Sorry, I realised further down that Mutex is a class 🤯 ... ffs.
Still, Condition should accept a shared mutex, or is there more to the story?

TurkeyMan · 2026-05-05T07:12:57Z

-            (cast() cargo) += 1;
+            // Mutex protects cargo, but -preview=nosharedaccess cannot infer
+            // the lock-based invariant from this shared method.
+            (cast(Resource) this).cargo += 1;


TurkeyMan · 2026-05-05T07:21:31Z

 // Multiple threads running shared fibers
 unittest
 {
    shared bool[10] locks;


I don't quite follow... shouldn't this be shared(bool)[10]?

TurkeyMan · 2026-05-05T07:22:53Z

+                    // Cast only to form the indexed element address; the actual
+                    // lock access below still goes through atomic operations.
+                    return cast(shared(bool)*) &((*cast(UnsharedLocks*) &locks)[idx]);
+                })();


shared(bool)[10] makes this go away... what have I misunderstood?
Are these 10 locks, or are they a single atomic multi-locking mechanism?

TurkeyMan · 2026-05-05T07:27:24Z

+        // dynamically loaded library; the library performs the actual access
+        // atomically, but address formation itself has no atomic API.
+        return cast(shared(UnsharedCounter)*) &(*cast(UnsharedCounter*) &finalizeCounter);
+    })();


Why is this pattern popping up a lot? What necessitates it?

TurkeyMan · 2026-05-05T07:34:09Z

Thanks for moving on this! I added some comments.

Fix all -preview=nosharedaccess errors in druntime

651d52c

atilaneves requested review from TurkeyMan and WalterBright May 1, 2026 08:18

thewilsonator reviewed May 4, 2026

View reviewed changes

Comment thread druntime/src/core/atomic.d Outdated

Comment thread memory.md Outdated

atilaneves force-pushed the shared-codex branch 6 times, most recently from bdaf3f7 to 7603e09 Compare May 4, 2026 11:26

dkorpel reviewed May 4, 2026

View reviewed changes

atilaneves force-pushed the shared-codex branch 2 times, most recently from 9f115bd to 91c4949 Compare May 4, 2026 15:34

Fix druntime nosharedaccess unittest build

b5c212a

atilaneves force-pushed the shared-codex branch from 91c4949 to b5c212a Compare May 4, 2026 16:07

atilaneves changed the title ~~Fix druntime -preview=nosharedaccess errors~~ Draft: Fix druntime -preview=nosharedaccess errors May 4, 2026

atilaneves changed the title ~~Draft: Fix druntime -preview=nosharedaccess errors~~ Fix druntime -preview=nosharedaccess errors May 4, 2026

atilaneves marked this pull request as draft May 4, 2026 17:46

TurkeyMan reviewed May 5, 2026

View reviewed changes

atilaneves mentioned this pull request May 7, 2026

Fix all -preview=nosharedaccess errors in druntime #23095

Draft

Uh oh!

Conversation

atilaneves commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented May 1, 2026

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented May 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurkeyMan May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurkeyMan commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

atilaneves commented May 1, 2026 •

edited

Loading

TurkeyMan May 5, 2026 •

edited

Loading