[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 Iain Buclaw changed: What|Removed |Added Priority|P1 |P3 --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 moonlightsenti...@disroot.org changed: What|Removed |Added See Also||https://issues.dlang.org/sh ||ow_bug.cgi?id=22748 --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #23 from Dlang Bot --- @WalterBright updated dlang/dmd pull request #6092 "fix Issue 14251 - synchronized (mtx) doesn't check attributes (pure, …" fixing this issue: - fix Issue 14251 - synchronized (mtx) doesn't check attributes (pure, const) https://github.com/dlang/dmd/pull/6092 --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #22 from Andrei Alexandrescu--- (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!!! --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #21 from Martin Nowak--- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #20 from Andrei Alexandrescu--- (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/xpliectmvwrwthamq...@forum.dlang.org. Fair enough. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #19 from Andrei Alexandrescu--- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #18 from Martin Nowak--- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #17 from Martin Nowak--- (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/xpliectmvwrwthamq...@forum.dlang.org. 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 --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #16 from ZombineDev--- (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*) } @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 = 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 } } --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #15 from Andrei Alexandrescu--- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #14 from Lodovico Giaretta--- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 Andrei Alexandrescuchanged: What|Removed |Added CC||and...@erdani.com --- Comment #13 from Andrei Alexandrescu --- Can someone produce an example in which invariants promised by D's system are broken? --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #12 from ZombineDev--- (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 . --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #11 from ZombineDev--- 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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #10 from Walter Bright--- 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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 Walter Brightchanged: What|Removed |Added Keywords||pull --- Comment #9 from Walter Bright --- https://github.com/dlang/dmd/pull/6092 --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #8 from Walter Bright--- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 Lodovico Giarettachanged: What|Removed |Added CC||lodov...@giaretart.net --- Comment #6 from Lodovico Giaretta --- (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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #7 from ZombineDev--- 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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 Walter Brightchanged: What|Removed |Added CC||bugzi...@digitalmars.com --- Comment #5 from Walter Bright --- 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. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 ZombineDevchanged: What|Removed |Added Keywords||safe CC||petar.p.ki...@gmail.com --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #4 from github-bugzi...@puremagic.com --- Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/4b74c5f687a06d95c4ffe6e8aefe10f88acc098b Remove deprecated `std.concurrency.MessageBox.isClosed() const`. Within Phobos, this is the only occurrence of synchronizing on an immutable object. Removal of this deprecated function allows us to resolve Issue 14251. --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #3 from Johan Engelen--- https://github.com/D-Programming-Language/dmd/pull/5564 --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 --- Comment #2 from Johan Engelen--- 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; } } --
[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)
https://issues.dlang.org/show_bug.cgi?id=14251 Johan Engelenchanged: What|Removed |Added CC||jbc.enge...@gmail.com --- Comment #1 from Johan Engelen --- 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 } } --