[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)

[d-bugmail--- via Digitalmars-d-bugs]

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)

[d-bugmail--- via Digitalmars-d-bugs]

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)

[d-bugmail--- via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14251

Andrei Alexandrescu  changed:

   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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14251

Walter Bright  changed:

   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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14251

Lodovico Giaretta  changed:

   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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14251

Walter Bright  changed:

   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)

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14251

ZombineDev  changed:

   What|Removed |Added

   Keywords||safe
 CC||petar.p.ki...@gmail.com

--

[Issue 14251] synchronized (mtx) doesn't check attributes (pure, const)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

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)

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=14251

Johan Engelen  changed:

   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
}
}

--