Issue 23709 - Cannot use synchronized on shared class with -preview=nosharedaccess
Summary: Cannot use synchronized on shared class with -preview=nosharedaccess
Status: REOPENED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86_64 Linux
: P1 regression
Assignee: No Owner
URL:
Keywords: pull, rejects-valid
Depends on:
Blocks:
 
Reported: 2023-02-15 09:22 UTC by Atila Neves
Modified: 2023-03-27 21:37 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Atila Neves 2023-02-15 09:22:40 UTC
class Class {}
void main() {
    synchronized(new shared Class) {}
}

% clear && dmd -g -debug -preview=nosharedaccess d.d && ./d
% d.d(3): Error: direct access to shared `new shared(Class)` is not allowed, see `core.atomic`


This means that it's not actually possible to synchronize on any given mutex.
Comment 1 RazvanN 2023-02-15 12:14:56 UTC
Actually, the issue does not seem to have anything to do with synchronized (although, looking at the code it seems that Synchronized Statements have their own issues), but rather that you cannot create shared objects:

class Class {}                                                                                                                                                                                                                                   
void main() {
    auto b = new shared Class();   // Error: direct access to shared `new shared(Class)` is not allowed, see `core.atomic`
}

This is ridiculous, so you can't create shared objects with new?
Comment 2 Atila Neves 2023-02-15 12:20:12 UTC
Oh, wow. I assumed the problem was with `synchronized` because the problem being with `new shared Class` was just too fundamental to even consider!
Comment 3 RazvanN 2023-02-15 12:58:48 UTC
This is actually a 2.095 regression.
Comment 4 Dlang Bot 2023-02-15 13:04:54 UTC
@RazvanN7 created dlang/dmd pull request #14884 "Fix Issue 23709 - Cannot use synchronized on shared class with -preview=nosharedaccess" fixing this issue:

- Fix Issue 23709 - Cannot use synchronized on shared class with -preview=nosharedaccess

https://github.com/dlang/dmd/pull/14884
Comment 5 Atila Neves 2023-02-15 13:52:24 UTC
This is actually two bugs: creating a shared instance isn't possible with new, but the bug description is still accurate since this doesn't compile:


class Class {}
void main() {
    shared Class c;
    synchronized(c) {}
}

% d.d(4): Error: direct access to shared `c` is not allowed, see `core.atomic`
Comment 6 Atila Neves 2023-02-22 13:32:12 UTC
I submitted a new issue:

https://issues.dlang.org/show_bug.cgi?id=23732
Comment 7 RazvanN 2023-03-21 15:15:29 UTC
This is not a bug. The compiler rewrites the code to:

auto tmp = c;
_d_monitorenter(tmp);
try {  body } finally { _d_monitorexit(tmp); }

So the reading of c needs to be atomic. Note that the original example (synchronized(new shared Class)) currently compiles because creating a shared instance does not need to be protected by a mutex.

So just write:

import core.atomic;

class Class {}
void main()
{
    shared Class c;
    synchronized(atomicLoad(c)) {}                                                                                   
}

And the code will compile.

Arguably, the compiler could automatically wrap `c` into an `atomicLoad`, however, that's based on the assumption that druntime is available and that the users does not employ a different synchronization mechanism.

I will tentatively close this as INVALID, but please reopen if I am missing something.
Comment 8 mhh 2023-03-21 16:35:39 UTC
If the monitor bit is synchronized does the load of c need to be atomic?
Comment 9 RazvanN 2023-03-22 08:34:01 UTC
(In reply to mhh from comment #8)
> If the monitor bit is synchronized does the load of c need to be atomic?

Yes, because the pointer to the class could be modified before entering the monitor function.
Comment 10 Atila Neves 2023-03-27 21:37:29 UTC
It doesn't make sense from a usability point of view for the compiler to not rewrite using atomicLoad, especially if it's always going to be needed except for when creating a mutex to pass in. That's clearly not going to be common.