While messing with atomicLoad [1], I noticed that dmd lets me implicitly convert values to/from shared without restrictions. It's in the spec [2]. This seems bad to me.

Here's how I understand shared. If anything's wrong, I'd appreciate if someone would educate me.

Shared is there to prevent me from accidentally writing code that's not thread-safe (at a low level).

When I never use shared (or __gshared or other mean hacks), I only ever get thread-local variables and I can't share data between threads. I get thread safety by avoiding the issue entirely.

When I do use shared, the compiler is supposed to reject (low-level) operations that are not thread-safe. That's why ++x is deprecated for a shared x, and I'm told to use atomicOp!"+=" instead.

Of course I can still do a verbose unsafe version which the compiler can't catch:

    atomicStore(x, atomicLoad(x) + 1);

The compiler can't possibly know which high-level operations need to run uninterrupted, so there's nothing it can do about this.

Back to implicit conversions. Loading and storing is as low-level as it gets. When D disallows unsafe ++ on shared, surely it should also disallow unsafe loading and storing.

Copying a shared value type to an unshared one could be defined as doing an atomic load, and copying the other way around could result in an atomic store. I don't think it's specified or implemented like this at the moment, but it would make sense to me. Loading/storing overly large value types can probably not be made thread-safe like that.

So, conclusion/proposal:

* Conversion of a value type from shared to unshared should only be allowed if it can be done with an atomic load. The compiler should generate the atomic load.
* Conversion of a value type from unshared to shared should only be allowed if it can be done with an atomic store. The compiler should generate the atomic store.
* Other conversions are not allowed.

Or alternatively, simply disallow all those conversions, and tell the user that they have to ensure thread safety themselves, e.g. by using atomicLoad/atomicStore.

Sorry for the noise if this is all known and part of the "shared isn't implemented" mantra. I couldn't find a bugzilla issue.


[1] https://github.com/dlang/druntime/pull/1605
[2] https://dlang.org/spec/const3.html#implicit_conversions (first sentence)

On Sunday, 10 July 2016 at 13:02:17 UTC, ag0aep6g wrote:
> While messing with atomicLoad [1], I noticed that dmd lets me implicitly convert values to/from shared without restrictions. It's in the spec [2]. This seems bad to me.
>
> [...]

Atomic loading and storing, from what I understand, is usually limited to about a word on most architectures. I don't think it would be good to implicitly define assignment and referencing as atomic operations, since it would be limited. IMO concurrent access is better off being explicit anyway.

I don't think there is an issue with converting unshared reference types to shared (ex. ref T -> ref shared(T) or T* -> shared(T)*); worst you get is some extra synchronization.

On 07/11/2016 01:52 AM, Alex Parrill wrote:
> Atomic loading and storing, from what I understand, is usually limited
> to about a word on most architectures. I don't think it would be good to
> implicitly define assignment and referencing as atomic operations, since
> it would be limited. IMO concurrent access is better off being explicit
> anyway.

Simply disallow reading and writing shared then, forcing something more explicit like atomicLoad/atomicStore?

That would be better than the current state, but it would make shared even more unwieldy.

Generating atomic operations would break less code, and feels like the obvious thing to me. It would be kinda explicit: shared being involved gives it away. Also, with other forms of ensuring thread safety, you have to cast shared away anyway, so atomic reading/writing wouldn't be applied there.

> I don't think there is an issue with converting unshared reference types
> to shared (ex. ref T -> ref shared(T) or T* -> shared(T)*); worst you
> get is some extra synchronization.

shared(T)* is not shared itself, so that's fine, yeah.

I think I've approached this from the wrong angle. Implicit conversions are not the problem. Reading/writing is. Copying a shared value to another shared location is unsafe, too, if not done atomically.

On 07/11/2016 07:26 AM, ag0aep6g wrote:
> On 07/11/2016 01:52 AM, Alex Parrill wrote:
[...]
>> I don't think there is an issue with converting unshared reference types
>> to shared (ex. ref T -> ref shared(T) or T* -> shared(T)*); worst you
>> get is some extra synchronization.
>
> shared(T)* is not shared itself, so that's fine, yeah.

The conversion T* -> shared(T)* isn't ok, of course. And it's correctly disallowed already. But it's for a different reason than this thread is about.

On 7/10/16 9:02 AM, ag0aep6g wrote:
> While messing with atomicLoad [1], I noticed that dmd lets me implicitly
> convert values to/from shared without restrictions. It's in the spec
> [2]. This seems bad to me.

I think you misunderstand the problem here. Conversion means changing the type.

Once you have loaded the shared data into a register, or whatever, it's no longer shared, it's local. Writing it out to another place doesn't change anything. It's once you add references into the mix where you may have a problem.

What I think you mean (and I think you realize this now), is that the actual copying of the data should not be implicitly allowed. The type change is fine, it's the physical reading or writing of shared data that can cause issues. I agree we should extend the rules to prevent this.

In other words:

shared int x;

void main()
{
   // ++x; // not allowed
   int x2 = x + 1; // but this is
   x = x2; // and it shouldn't be
}

-Steve

On 07/11/2016 02:54 PM, Steven Schveighoffer wrote:
> I think you misunderstand the problem here.

Yes.

> Conversion means changing
> the type.
>
> Once you have loaded the shared data into a register, or whatever, it's
> no longer shared, it's local. Writing it out to another place doesn't
> change anything. It's once you add references into the mix where you may
> have a problem.

Right.

> What I think you mean (and I think you realize this now), is that the
> actual copying of the data should not be implicitly allowed. The type
> change is fine, it's the physical reading or writing of shared data that
> can cause issues. I agree we should extend the rules to prevent this.

Exactly.

> In other words:
>
> shared int x;
>
> void main()
> {
>     // ++x; // not allowed
>     int x2 = x + 1; // but this is
>     x = x2; // and it shouldn't be
> }

I think I would prefer if the compiler would generate atomic operations, but I'm clearly far from being an expert on any of this. Simply rejecting the code would be fine with me, too.

Also:

shared int x;
shared int y;
x = y; /* should be rejected too (or be atomic, if that's even possible) */

On Monday, 11 July 2016 at 05:26:42 UTC, ag0aep6g wrote:
> Simply disallow reading and writing shared then, forcing something more explicit like atomicLoad/atomicStore?
>
> That would be better than the current state, but it would make shared even more unwieldy.

Atomic loads are only needed for volatile variables, not for all kinds of shared data. Also currently atomicLoad doesn't provide functionality equivalent to raw load.

> Generating atomic operations would break less code, and feels like the obvious thing to me.

Multithreaded code can't be generated.

On 07/11/2016 03:31 PM, Kagamin wrote:
> Atomic loads are only needed for volatile variables, not for all kinds
> of shared data.

Volatile just means that another thread can mess with the data, right? So shared data that's not being written to from elsewhere isn't volatile, and one doesn't need an atomic load to read it.

If I got that right: Sure. But the compiler can't know if a shared variable is volatile or not, so it has to assume that it is. If the programmer knows that it's not volatile, they can cast shared away and use a normal load.

> Also currently atomicLoad doesn't provide functionality
> equivalent to raw load.

Is a "raw load" just a non-atomic load, or is it something special?
What's the relevance of atomicLoad's capabilities?

>> Generating atomic operations would break less code, and feels like the
>> obvious thing to me.
>
> Multithreaded code can't be generated.

For primitive types, atomic loads and stores can be generated, no? It's clear that this doesn't make the code automatically thread-safe. It just guards against an easily made mistake. Like shared is supposed to do, as far as I understand.

On 07/11/2016 03:23 PM, ag0aep6g wrote:
> I think I would prefer if the compiler would generate atomic operations,

Backpedaling on that one.

With automatic atomic loads and stores, one could accidentally write this:

    shared int x;
    x = x + 1; /* atomic load + atomic != atomic increment */

Easy to miss the problem, because the code looks so innocent.

But when the atomic loads and stores must be spelled out it would look like in my original post:

    shared int x;
    atomicStore(x, atomicLoad(x) + 1);

Way more obvious that the code isn't actually thread-safe.

So now I'm leaning towards requiring the verbose version.

On Monday, 11 July 2016 at 13:54:49 UTC, ag0aep6g wrote:
> If I got that right: Sure. But the compiler can't know if a shared variable is volatile or not, so it has to assume that it is. If the programmer knows that it's not volatile, they can cast shared away and use a normal load.

If you cast shared away, an unshared postblit will be called instead of shared one.