It's possible to run `synchronized (mtx)` in a `pure const` function even though neither of both attributes apply.
This happens because the compiler doesn't run semantic on the _d_monitorenter/exit function calls.
Comment #1 by jbc.engelen — 2016-03-22T16:30:35Z
Another effect of the missing semantic on _d_monitorenter/exit is that this code compiles while I think it shouldn't (_d_monitorenter/exit will write to klass.__monitor):
void foo() {
immutable Klass klass = new Klass;
synchronized (klass)
{
// do smth
}
}
Comment #2 by jbc.engelen — 2016-03-22T17:06:34Z
Synchronizing inside a const method is done in a deprecated method in std.concurrency:
// @@@DEPRECATED_2016-03@@@
/++
$(RED Deprecated. isClosed can't be used with a const MessageBox.
It will be removed in March 2016).
+/
deprecated("isClosed can't be used with a const MessageBox")
final @property bool isClosed() const
{
synchronized( m_lock )
{
return m_closed;
}
}
I'm not convinced this is a bug. The _monitor field is one that is totally managed by the language, and it outside of its rules.
Comment #6 by lodovico — 2016-07-12T12:21:05Z
(In reply to Walter Bright from comment #5)
> I'm not convinced this is a bug. The _monitor field is one that is totally
> managed by the language, and it outside of its rules.
That is true. But having "logically inexistent" fields with different attributes from "normal" fields means that these attributes can not be "physically" exploited, but only "logically" enforced.
E.g.: if an immutable object has a mutable mutex, than an immutable object cannot reside in read-only memory.
So it's a tradeoff, and it must be clearly defined what is D's way here.
Comment #7 by petar.p.kirov — 2016-07-12T12:21:55Z
The OP issue doesn't mention class monitors. The bug also affects raw sync primitives like core.sync.mutex and in general everything using Object.IMonitor.
Comment #8 by bugzilla — 2016-07-12T22:36:57Z
(In reply to ZombineDev from comment #7)
> The OP issue doesn't mention class monitors. The bug also affects raw sync
> primitives like core.sync.mutex and in general everything using
> Object.IMonitor.
'Klass' is a class, and so has a class monitor.
I have serious misgivings about this being even a bug. The monitor is outside of the type system, and should work regardless of the type of the class object. Also, I don't see a good reason why it shouldn't work for pure functions, too. So I may withdraw the PR to 'fix' it.
Comment #11 by petar.p.kirov — 2016-08-27T07:57:15Z
A const reference to an object may actually refer to a immutable object. If such object sits in ROM than failure of the compiler to prevent such error (modification of the monitor) may have bad consequences (depending on the implementation). Also most of the time, an object is being synchronized to ensure exclusive write access to it. However there's no point in that if the object is not mutable.
As for pure, personally I don't think that pure functions should be allowed to cause a deadlock (I consider it to be a global side effect). Furthermore, 99% of the time synchronized is used on shared objects, which pure functions have no business looking at.
Comment #12 by petar.p.kirov — 2016-08-27T08:09:08Z
(In reply to Walter Bright from comment #8)
> (In reply to ZombineDev from comment #7)
> > The OP issue doesn't mention class monitors. The bug also affects raw sync
> > primitives like core.sync.mutex and in general everything using
> > Object.IMonitor.
>
> 'Klass' is a class, and so has a class monitor.
No, by OP, I meant Martin's comment.
E.g. https://github.com/dlang/druntime/blob/v2.071.2-b2/src/core/sync/mutex.d#L260 .
Comment #13 by andrei — 2016-08-30T11:12:29Z
Can someone produce an example in which invariants promised by D's system are broken?
Comment #14 by lodovico — 2016-08-30T13:52:02Z
(In reply to Andrei Alexandrescu from comment #13)
> Can someone produce an example in which invariants promised by D's system
> are broken?
immutable provides a strong guarantee, that allows me to put my immutable data in ROM. If I manage to have an immutable object allocated in ROM and someone tries to synchronize on it, my program will (in the best case) crash with a segmentation fault, as the synchronized statement tries to modify a mutex that is located in ROM.
Comment #15 by andrei — 2016-08-30T14:01:44Z
(In reply to Lodovico Giaretta from comment #14)
> (In reply to Andrei Alexandrescu from comment #13)
> > Can someone produce an example in which invariants promised by D's system
> > are broken?
>
> immutable provides a strong guarantee, that allows me to put my immutable
> data in ROM. If I manage to have an immutable object allocated in ROM and
> someone tries to synchronize on it, my program will (in the best case) crash
> with a segmentation fault, as the synchronized statement tries to modify a
> mutex that is located in ROM.
That's not the case. The compiler knows the object has mutable metadata and won't allow placing it in read-only pages.
Comment #16 by petar.p.kirov — 2016-08-30T17:06:04Z
(In reply to Andrei Alexandrescu from comment #15)
> (In reply to Lodovico Giaretta from comment #14)
> > (In reply to Andrei Alexandrescu from comment #13)
> > > Can someone produce an example in which invariants promised by D's system
> > > are broken?
> >
> > immutable provides a strong guarantee, that allows me to put my immutable
> > data in ROM. If I manage to have an immutable object allocated in ROM and
> > someone tries to synchronize on it, my program will (in the best case) crash
> > with a segmentation fault, as the synchronized statement tries to modify a
> > mutex that is located in ROM.
>
> That's not the case. The compiler knows the object has mutable metadata and
> won't allow placing it in read-only pages.
Wrong. See for yourself: https://dpaste.dzfl.pl/be0f23bf35c0
BTW, what do you think about pure? Should locking of shared objects really be allowed in pure code? According to http://dlang.org/spec/function.html#pure-functions:
> a pure function: does not read or write any global or static mutable state
// Live demo: https://dpaste.dzfl.pl/be0f23bf35c0
import core.sync.mutex : Mutex;
void main()
{
// case 1: Allows unsafe use of core.sync.Mutex
const stdMutex = new const Mutex();
constAndpurityTest(stdMutex);
// case 2: Breaks immutability guarantee
immutable myMutex = new immutable MyMutex();
assert (myMutex.flag == 0);
//myMutex.lock(); // correctly disalowed
synchronized(myMutex)
{
// my fail depending on the optimization level
assert (myMutex.flag == 1); // wrong!!!
}
// case 3: Modifies normal object that could be stored in ROM
immutable c = new immutable C();
assert (c.__monitor is null);
synchronized (c) { }
assert (c.__monitor !is null); // WRONG!
}
class C { }
class MyMutex : Object.Monitor
{
int flag;
// See https://github.com/dlang/druntime/blob/v2.071.2-b2/src/rt/monitor_.d#L204
// and https://github.com/dlang/druntime/blob/v2.071.2-b2/src/core/sync/mutex.d#L81
// for details.
Object.Monitor necessaryIndirection;
this() pure immutable
{
this.necessaryIndirection = this;
this.__monitor = cast(void*)&this.necessaryIndirection;
}
@trusted void lock()
{
this.flag++;
}
@trusted void unlock()
{
//this.flag--;
}
}
void constAndpurityTest(const Mutex stdMutex) pure
{
import std.traits : FA = FunctionAttribute, fattrs = functionAttributes;
auto stdMutexLock = &stdMutex.lock;
static assert((fattrs!stdMutexLock & FA.pure_) == 0);
static assert((fattrs!stdMutexLock & FA.const_) == 0);
synchronized (stdMutex) // Accepts invalid!
{
// synchronized happily calls the core.sync.Mutex.lock() method which is
// neither pure, nor const
}
}
Comment #17 by code — 2016-09-21T00:57:32Z
(In reply to Andrei Alexandrescu from comment #15)
> That's not the case. The compiler knows the object has mutable metadata and
> won't allow placing it in read-only pages.
Not allowing to put any class into read-only segments, just b/c someone might want to synchronize on it, is not very convincing.
Also remember that we wanted to get rid of the extra 8-byte for monitor unless explicitly requested http://forum.dlang.org/post/[email protected].
Turning synchronized into a lowering for lock/unlock (with monitor support as fallback) would not only allow correct attribute inference (w/o making global decisions for everyone, @nogc, ...), it's also a less overhead for the core.sync classes to not go through the virtual monitor indirections [¹].
[¹]: https://github.com/dlang/druntime/blob/15a227477a344583c4748d95492703901417f4f8/src/rt/monitor_.d#L59
Comment #18 by code — 2016-09-21T01:03:55Z
(In reply to Andrei Alexandrescu from comment #13)
> Can someone produce an example in which invariants promised by D's system
> are broken?
Just look at core.sync, none of the methods can be implemented const or pure, still they get called from const/pure code.
In fact Object.Monitor simply declares that lock/unlock doesn't need to have any attributes https://github.com/dlang/druntime/blob/e9c7878928330aa34e6ba5c5863ed5507e02248e/src/object.d#L97-L101, but synchronized forges guarantees that aren't there.
Comment #19 by andrei — 2016-09-21T03:00:07Z
(In reply to Martin Nowak from comment #18)
> (In reply to Andrei Alexandrescu from comment #13)
> > Can someone produce an example in which invariants promised by D's system
> > are broken?
>
> Just look at core.sync, none of the methods can be implemented const or
> pure, still they get called from const/pure code.
That's not a problem, the runtime is expected to contain nonportable code.
Comment #20 by andrei — 2016-09-21T03:00:48Z
(In reply to Martin Nowak from comment #17)
> Not allowing to put any class into read-only segments, just b/c someone
> might want to synchronize on it, is not very convincing.
> Also remember that we wanted to get rid of the extra 8-byte for monitor
> unless explicitly requested
> http://forum.dlang.org/post/[email protected].
Fair enough.
Comment #21 by code — 2016-09-22T16:29:51Z
(In reply to Andrei Alexandrescu from comment #19)
> > Just look at core.sync, none of the methods can be implemented const or
> > pure, still they get called from const/pure code.
>
> That's not a problem, the runtime is expected to contain nonportable code.
I'd say the real attribute problem is not the automatic mutex generated for `synchronized (instance)`, b/c we can control how that behaves. But b/c it's possible to assign an arbitrary Object.Monitor implementation, people can run very different Mutex implementations.
As usual there is no deprecation path for adding attributes to an interface/base-class, and the last time we tried to make Object.Monitor nothrow, we broke valid use cases in vibe.d that we're using async/event-based mutexes.
Now this bug report is about the effect that _d_monitorenter/exit do call any Mutex implementation w/o checking for attributes.
We had so many problems w/ attributes and old C compiler-runtime APIs, and it always boils down to this:
Forcing everyone to use the same attributes is too limiting/breaks code, let's replace the old C API w/ templated library code.
Comment #22 by andrei — 2016-09-22T16:38:29Z
(In reply to Martin Nowak from comment #21)
> Forcing everyone to use the same attributes is too limiting/breaks code,
> let's replace the old C API w/ templated library code.
Bestimmt!!!