Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

JohanEngelen
Copy link
Contributor

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

@dnadlinger
Copy link
Contributor

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dnadlinger dnadlinger Apr 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants