Ensure that the sync object is mutable in a `synchronize(obj) {...}` statement. #5564

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@JohanEngelen
Contributor

JohanEngelen commented Mar 22, 2016

This PR is related to Issue 14251. I think it fixes it completely, but I am uncertain what the correct behavior should be for pure functions.

The PR adds a check for mutability of the sync object: it it passed to druntime functions that require mutability of it, and so passing an immutable object may result in bad runtime behavior.

A simple testcase that shows the problem in LDC is:

import std.stdio : writeln;
interface Foo {}
void main() {
    writeln(cast(size_t) typeid(Foo).__monitor);
    synchronized(typeid(Foo)) { }
    writeln(cast(size_t) typeid(Foo).__monitor);
}

typeid returns an immutable(TypeInfo) in LDC. Without optimizations, this prints zero and a non-zero value; with optimizations turned on it prints two zeroes.

Current Phobos contains one case where synchronization inside a const method is done: std.concurrency. I think this is a latent bug, however that method is marked for removal in March 2016.
https://github.com/D-Programming-Language/phobos/blob/master/std/concurrency.d#L1815

@klickverbot

This comment has been minimized.

Show comment
Hide comment
@klickverbot

klickverbot Mar 26, 2016

Member

Ping @WalterBright @andralex since this is technically a language change.

Member

klickverbot commented Mar 26, 2016

Ping @WalterBright @andralex since this is technically a language change.

+ +/
+ if (!exp.type.isMutable())
+ {
+ error("can only synchronize on a mutable object, not on '%s' of type '%s'", exp.toChars(), exp.type.toChars());

This comment has been minimized.

@MartinNowak

MartinNowak Mar 29, 2016

Member

Just because you fixed phobos doesn't mean you can directly turn this into an error, please start with a deprecation.

@MartinNowak

MartinNowak Mar 29, 2016

Member

Just because you fixed phobos doesn't mean you can directly turn this into an error, please start with a deprecation.

This comment has been minimized.

@JohanEngelen

JohanEngelen Mar 29, 2016

Contributor

Perhaps DMD does not optimize enough for this to result in bad codegen. But for LDC, this will have to be an error, because it results in crashing code and was never supported.

@JohanEngelen

JohanEngelen Mar 29, 2016

Contributor

Perhaps DMD does not optimize enough for this to result in bad codegen. But for LDC, this will have to be an error, because it results in crashing code and was never supported.

This comment has been minimized.

@klickverbot

klickverbot Apr 17, 2016

Member

@MartinNowak: Doesn't DMD ever put immutable objects into .rodata? If it does, this is merely fixing an accepts-invalid bug and should go straight to an error.

@klickverbot

klickverbot Apr 17, 2016

Member

@MartinNowak: Doesn't DMD ever put immutable objects into .rodata? If it does, this is merely fixing an accepts-invalid bug and should go straight to an error.

JohanEngelen added a commit to JohanEngelen/ldc that referenced this pull request Apr 5, 2016

JohanEngelen added a commit to JohanEngelen/ldc that referenced this pull request Apr 5, 2016

JohanEngelen added a commit to JohanEngelen/ldc that referenced this pull request Apr 29, 2016

JohanEngelen added a commit to JohanEngelen/ldc that referenced this pull request Apr 29, 2016

JohanEngelen added a commit to JohanEngelen/ldc that referenced this pull request May 1, 2016

@JohanEngelen

This comment has been minimized.

Show comment
Hide comment
@JohanEngelen

JohanEngelen May 7, 2016

Contributor

Needs more in-depth study and discussion

Contributor

JohanEngelen commented May 7, 2016

Needs more in-depth study and discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment