On Mon, Oct 22, 2018 at 2:30 AM Walter Bright via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On 10/22/2018 1:34 AM, Manu wrote:
> > I posted it, twice... 2 messages, back to back, and you're responding to this one, and not that one. I'll post it again...
>
>
> Posting it over and over is illustrative of the failure of posting proposal documents to the n.g. instead of posting it as a DIP which can be referred to:
>
> 1. nobody knows which of your 70 messages are the ones with the proposal in it
>
> 2. with multiple posts of the proposal, nobody knows which one is the most up-to-date one

It hasn't changed. Not one single bit. I haven't changed a single detail in this thread.

> Doing it this way does not work. Continuing to repost it is a waste of your time. Post it as a DIP and link to it.

And you STILL ignored my post >_<
Please look at the proper code that implements your broken example. I
know you've seen it now.

On 22.10.18 02:54, Manu wrote:
> On Sun, Oct 21, 2018 at 5:40 PM Timon Gehr via Digitalmars-d
> <digitalmars-d@puremagic.com> wrote:
>>
>> On 21.10.18 21:04, Manu wrote:
>>> On Sun, Oct 21, 2018 at 12:00 PM Timon Gehr via Digitalmars-d
>>> <digitalmars-d@puremagic.com> wrote:
>>>>
>>>> On 21.10.18 17:54, Nicholas Wilson wrote:
>>>>>
>>>>>> As soon as that is done, you've got a data race with the other
>>>>>> existing unshared aliases.
>>>>>
>>>>> You're in @trusted code, that is the whole point. The onus is on the
>>>>> programmer to make that correct, same with regular @safe/@trusted@system
>>>>> code.
>>>>
>>>> Not all of the parties that participate in the data race are in @trusted
>>>> code. The point of @trusted is modularity: you manually check @trusted
>>>> code according to some set of restrictions and then you are sure that
>>>> there is no memory corruption.
>>>>
>>>> Note that you are not allowed to look at any of the @safe code while
>>>> checking your @trusted code. You will only see an opaque interface to
>>>> the @safe code that you call and all you know is that all the @safe code
>>>> type checks according to @safe rules. Note that there might be an
>>>> arbitrary number of @safe functions and methods that you do not see.
>>>>
>>>> Think about it this way: you first write all the @trusted and @system
>>>> code, and some evil guy who does not like you comes in after you looks
>>>> at your code and writes all the @safe code. If there is any memory
>>>> corruption, it will be your fault and you will face harsh consequences.
>>>> Now, design the @safe type checking rules. It won't be MP!
>>>>
>>>> Note that there may well be a good way to get the good properties of MP
>>>> without breaking the type system, but MP itself is not good because it
>>>> breaks @safe.
>>>
>>> Show me. Nobody has been able to show that yet. I'd really like to know this.
>>>
>>
>> I just did,
> 
> There's no code there... just a presumption that the person who wrote
> the @trusted code did not deliver the promise they made.
> ...

Yes, because there is no way to write @trusted code that holds its promise while actually doing something interesting in multiple threads if @safe code can implicitly convert from unshared to shared.

>> but if you really need to, give me a non-trivial piece of> correct multithreaded code that accesses some declared-unshared field
>> from a shared method and I will show you how the evil guy would modify
>> some @safe code in it and introduce race conditions. It needs to be your
>> code, as otherwise you will just claim again that it is me who wrote bad
>> @trusted code.
> 
> You can pick on any of my prior code fragments. They've all been ignored so far.
> 

I don't want "code fragments". Show me the real code.

I manually browsed through posts now (thanks a lot) and found this implementation:

struct Atomic(T){
  void opUnary(string op : "++")() shared { atomicIncrement(&val); }
  private T val;
}

This is @system code. There is no @safe or @trusted here, so I am ignoring it.


Then I browsed some more, because I had nothing better to do, and I found this. I completed it so that it is actually compilable, except for the unsafe implicit conversion.

Please read this code, and then carefully read the comments below it before you respond. I will totally ignore any of your answers that arrive in the next two hours.

---
module borked;

void atomicIncrement(int* p)@system{
    import core.atomic;
    atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
}

struct Atomic(T){
    private T val;
    void opUnary(string op : "++")() shared @trusted {
        atomicIncrement(cast(T*)&val);
    }
}
void main()@safe{
    Atomic!int i;
    auto a=&[i][0];// was: Atomic!int* a = &i;
    import std.concurrency;
    spawn((shared(Atomic!int)* a){ ++*a; }, a);
    ++i.val; // race
}
---


Oh no! The author of the @trusted function (i.e. you) did not deliver on the promise they made!

Now, before you go and tell me that I am stupid because I wrote bad code, consider the following:

- It is perfectly @safe to access private members from the same module.

- You may not blame the my @safe main function for the problem. It is @safe, so it cannot be blamed for UB. Any UB is the result of a bad @trusted function, a compiler bug, or hardware failure.

- The only @trusted function in this module was written by you.

You said that there is a third implementation somewhere. If that one actually works, I apologize and ask you to please paste it again in this subthread.

On Monday, 22 October 2018 at 00:22:19 UTC, Manu wrote:

> No no, they're repeated, not scattered, because I seem to have to keep repeating it over and over, because nobody is reading the text, or perhaps imaging there is a lot more text than there is.
> ...
> You mean like every post in opposition which disregards the rules and baselessly asserts it's a terrible idea? :/
> ...
> I responded to your faulty program directly with the correct program, and you haven't acknowledged it. Did you see it?

Right back at you.

Quote:

I think this is a typical sort of construction:

struct ThreadsafeQueue(T)
{
  void QueueItem(T*) shared;
  T* UnqueueItem() shared;
}

struct SpecialWorkList
{
  struct Job { ... }

  void MakeJob(int x, float y, string z) shared  // <- any thread may
produce a job
  {
    Job* job = new Job; // <- this is thread-local
    PopulateJob(job, x, y, z); // <- preparation of a job might be
complex, and worthy of the SpecialWorkList implementation

    jobList.QueueItem(job);  // <- QueueItem encapsulates
thread-safety, no need for blunt casts
  }

  void Flush() // <- not shared, thread-local consumer
  {
    Job* job;
    while (job = jobList.UnqueueItem()) // <- it's obviously safe for
a thread-local to call UnqueueItem even though the implementation is
threadsafe
    {
      // thread-local dispatch of work...
      // perhaps rendering, perhaps deferred destruction, perhaps
deferred resource creation... whatever!
    }
  }

  void GetSpecialSystemState() // <- this has NOTHING to do with the
threadsafe part of SpecialWorkList
  {
    return os.functionThatChecksSystemState();
  }

  // there may be any number of utility functions that don't interact
with jobList.

private:
  void PopulateJob(ref Job job, ...)
  {
    // expensive function; not thread-safe, and doesn't have any
interaction with threading.
  }

  ThreadsafeQueue!Job jobList;
}


This isn't an amazing example, but it's typical of a thing that's
mostly thread-local, and only a small controlled part of it's
functionality is thread-safe.
The thread-local method Flush() also deals with thread-safety
internally... because it flushes a thread-safe queue.

All thread-safety concerns are composed by a utility object, so there's no need for locks, magic, or casts here.

EndQuote;

The above:
1) Will not compile, not currently, not under your proposal (presumably you forgot in frustration to cast before calling PopulateJob?..)
2) Does not in any way demonstrate a practical @safe application of an implicit conversion. As I wrote in the original response to that code, with that particular code it seems more like you just need forwarding methods that call `shared` methods under the hood (i.e. MakeJob), and it'd be "nice" if you didn't have to write those and could just call `shared` MakeJob on an un-`shared` reference directly. But these are all assumptions without seeing the actual usage.

Please just stop acting like everyone here is opposing *you*. All you're doing is dismissing everyone with a "nuh-huh, you no understand, you bad". If it was just me, fine, it would mean I'm dumb and not worthy of this discussion. But this isn't the case, which means *you are not getting your point across*. And yet instead of trying to fix that, you're getting all snarky.

On Monday, 22 October 2018 at 10:26:14 UTC, Timon Gehr wrote:
> ---
> module borked;
>
> void atomicIncrement(int* p)@system{
>     import core.atomic;
>     atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
> }
>
> struct Atomic(T){
>     private T val;
>     void opUnary(string op : "++")() shared @trusted {
>         atomicIncrement(cast(T*)&val);
>     }
> }
> void main()@safe{
>     Atomic!int i;
>     auto a=&[i][0];// was: Atomic!int* a = &i;
>     import std.concurrency;
>     spawn((shared(Atomic!int)* a){ ++*a; }, a);
>     ++i.val; // race
> }
> ---
>
>
> Oh no! The author of the @trusted function (i.e. you) did not deliver on the promise they made!

hi, if you change the private val in Atomic to be “private shared T val”, is the situation the same?

On 22.10.18 12:26, Timon Gehr wrote:
> ---
> module borked;
> 
> void atomicIncrement(int* p)@system{
>      import core.atomic;
>      atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
> }
> 
> struct Atomic(T){
>      private T val;
>      void opUnary(string op : "++")() shared @trusted {
>          atomicIncrement(cast(T*)&val);
>      }
> }
> void main()@safe{
>      Atomic!int i;
>      auto a=&[i][0];// was: Atomic!int* a = &i;
>      import std.concurrency;
>      spawn((shared(Atomic!int)* a){ ++*a; }, a);
>      ++i.val; // race
> }
> ---

Obviously, this should have been:

---
module borked;

void atomicIncrement(int*p)@system{
    import core.atomic;
    atomicOp!"+="(*cast(shared(int)*)p,1);
}
struct Atomic(T){
    private T val;
    void opUnary(string op:"++")()shared @trusted{
        atomicIncrement(cast(T*)&val);
    }
}
void main()@safe{
    auto a=new Atomic!int;
    import std.concurrency;
    spawn((shared(Atomic!int)* a){ ++*a; }, a);
    ++a.val; // race
}
---

(I was short on time and had to fix Manu's code because it was not actually compilable.)

On 22.10.18 14:39, Aliak wrote:
> On Monday, 22 October 2018 at 10:26:14 UTC, Timon Gehr wrote:
>> ---
>> module borked;
>>
>> void atomicIncrement(int* p)@system{
>>     import core.atomic;
>>     atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
>> }
>>
>> struct Atomic(T){
>>     private T val;
>>     void opUnary(string op : "++")() shared @trusted {
>>         atomicIncrement(cast(T*)&val);
>>     }
>> }
>> void main()@safe{
>>     auto a=new Atomic!int;
>>     import std.concurrency;
>>     spawn((shared(Atomic!int)* a){ ++*a; }, a);
>>     ++a.val; // race
>> }
>> ---
>>
>>
>> Oh no! The author of the @trusted function (i.e. you) did not deliver on the promise they made!
> 
> hi, if you change the private val in Atomic to be “private shared T val”, is the situation the same?

It's a bit different, because then there is no implicit unshared->shared conversion happening, and this discussion is only about that. However, without further restrictions, you can probably construct cases where a @safe function in one module escapes a private shared(T)* member to somewhere else that expects a different synchronization strategy.

Therefore, even if we agree that unshared->shared conversion cannot be implicit in @safe code, the 'shared' design is not complete, but it would be a good first step to agree that this cannot happen, such that we can then move on to harder issues.

E.g. probably it would be good to have something like @trusted data that cannot be manipulated from @safe code, such that @trusted functions can rely on some invariants.

On Monday, 22 October 2018 at 10:26:14 UTC, Timon Gehr wrote:
> module borked;
>
> void atomicIncrement(int* p)@system{
>     import core.atomic;
>     atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
> }
>
> struct Atomic(T){
>     private T val;
>     void opUnary(string op : "++")() shared @trusted {
>         atomicIncrement(cast(T*)&val);
>     }
> }
> void main()@safe{
>     Atomic!int i;
>     auto a=&[i][0];// was: Atomic!int* a = &i;
>     import std.concurrency;
>     spawn((shared(Atomic!int)* a){ ++*a; }, a);
>     ++i.val; // race
> }
> ---
>
>
> Oh no! The author of the @trusted function (i.e. you) did not deliver on the promise they made!
>
> Now, before you go and tell me that I am stupid because I wrote bad code, consider the following:
>
> - It is perfectly @safe to access private members from the same module.

Yes, so you need to place the @trusted code in a separate module. The @trusted code will be very rare (Atomic!T, lockfree queue, mutex, semaphore...). This will be in the standard library, possibly some other library. You should not be writing your own implementations of these. You should not be writing @trusted code without very good reason. You should be using @safe functions all over your code if at all possible.


> - You may not blame the my @safe main function for the problem. It is @safe, so it cannot be blamed for UB. Any UB is the result of a bad @trusted function, a compiler bug, or hardware failure.

No, we blame the fact you have not blocked off non-thread-safe access to Atomic!T.val. What you have made is this:

https://i.imgur.com/PnKMigl.jpg

The piece of code to blame is the @trusted function - you can't trust it, since non-thread-safe access to Atomic!T.val has not been blocked off. As long as anyone can extend its interface, Atomic!T can't be thread-safe.

I'll quote myself:

> For clarity: the interface of a type is any method, function,
> delegate or otherwise that may affect its internals. That
> means any free function in the same module, and any
> non-private members.

I've actually missed some possibilities there - member functions of other types in the same module must also count as part of the interface. Because of this wide net, modules that implement thread-safe types with shared methods should be short and sweet.


> - The only @trusted function in this module was written by you.
>
> You said that there is a third implementation somewhere. If that one actually works, I apologize and ask you to please paste it again in this subthread.

Here's the correct version:

module atomic;

void atomicIncrement(int* p) @system {
    import core.atomic;
    atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
}

struct Atomic(T) {
    // Should probably mark this shared for extra safety,
    // but it's not strictly necessary
    private T val;
    void opUnary(string op : "++")() shared @trusted {
        atomicIncrement(cast(T*)&val);
    }
}
---------
module unborked;
import atomic;

void main() @safe {
    auto a = new Atomic!int;
    import std.concurrency;
    spawn((shared(Atomic!int)* a){ ++*a; }, a);
    //++i.val; // Cannot access private member
}

Once more, Joe Average Programmer should not be writing the @trusted code in Atomic!T.opUnary - he should be using libraries written by people who have studied the exact issues that make multithreading hard.

--
  Simen

On 22.10.18 03:01, Manu wrote:
> On Sun, Oct 21, 2018 at 5:55 PM Timon Gehr via Digitalmars-d
> <digitalmars-d@puremagic.com> wrote:
>>
>> On 22.10.18 02:45, Manu wrote:
>>> On Sun, Oct 21, 2018 at 5:35 PM Timon Gehr via Digitalmars-d
>>> <digitalmars-d@puremagic.com> wrote:
>>>>
>>>> On 21.10.18 20:46, Manu wrote:
>>>>>> Shared data is only useful if, at some point, it is read/written, presumably by
>>>>>> casting it to unshared in @trusted code. As soon as that is done, you've got a
>>>>>> data race with the other existing unshared aliases.
>>>>> If such a race is possible, then the @trusted function is not
>>>>> threadsafe, so it is not @trusted by definition.
>>>>> You wrote a bad @trusted function, and you should feel bad.
>>>>> ...
>>>>
>>>> I wonder where this "each piece of code is maintained by only one person
>>>> and furthermore this is the only person that will suffer if the code has
>>>> bugs" mentality comes from. It is very popular as well as obviously
>>>> nonsense.
>>>>
>>>>> The simplest way to guarantee that no unsafe access is possible is to
>>>>> use encapsulation to assure no unregulated access exists.
>>>>
>>>> This only works if untrusted programmers (i.e. programmers who are only
>>>> allowed to write/modify @safe code) are not allowed to change your
>>>> class. I.e. it does not work.
>>>
>>> Have you ever cracked open std::map and 'fixed' it because you thought
>>> it was bad?

(Also, yes, some people do that because std::map does not provide an interface to augment the binary search tree.)

>>> Of course not. Same applies here. Nobody 'fixes' core.atomic.Atomic
>>> without understanding what they're doing.
>>>
>>
>> You are not proposing to let core.atomic.Atomic convert to shared
>> implicitly, you are proposing to do that for all classes.
> 
> You can always implicitly convert to shared.

Yes, exactly what I said.

> Where did I ever say anything like that? I'm sure I've never said
> this.

???

I said that you are proposing to allow implicit conversions to shared for all classes, not only core.atomic.Atomic, and the last time you said it was the previous sentence of the same post.

> How do these transformations of what I've said keep happening?
> ...

You literally said that nobody changes core.atomic.Atomic. Anyway, even if I bought that @safe somehow should not be checked within druntime (I don't), bringing up this example does not make for a coherent argument why implicit conversion to shared should be allowed for all classes.

>>> You seem to be stuck on the detail whether you can trust the @trusted
>>> author though...
>>
>> Again: the @safe author is the problem.
> 
> I don't follow. The @safe author is incapable of doing threadsafety
> violation.

They are capable of doing so as soon as you provide them a @trusted function that treats data as shared that they can access as unshared.

> They can only combine threadsafe functions.
> They can certainly produce a program that doesn't work, and they are
> capable of ordering issues, but that's not the same as data-race
> related crash bugs.
> 

Accessing private members of aggregates in the same module is @safe. tupleof is @safe too.

On 22.10.18 15:26, Simen Kjærås wrote:
> Here's the correct version:
> 
> module atomic;
> 
> void atomicIncrement(int* p) @system {
>      import core.atomic;
>      atomicOp!("+=",int,int)(*cast(shared(int)*)p,1);
> }
> 
> struct Atomic(T) {
>      // Should probably mark this shared for extra safety,
>      // but it's not strictly necessary
>      private T val;
>      void opUnary(string op : "++")() shared @trusted {
>          atomicIncrement(cast(T*)&val);
>      }
> }
> ---------
> module unborked;
> import atomic;
> 
> void main() @safe {
>      auto a = new Atomic!int;
>      import std.concurrency;
>      spawn((shared(Atomic!int)* a){ ++*a; }, a);
>      //++i.val; // Cannot access private member
> }
> 
> Once more, Joe Average Programmer should not be writing the @trusted code in Atomic!T.opUnary - he should be using libraries written by people who have studied the exact issues that make multithreading hard.
> 
> -- 
>    Simen

module reborked;
import atomic;

void main()@safe{
    auto a=new Atomic!int;
    import std.concurrency;
    spawn((shared(Atomic!int)* a){ ++*a; }, a);
    ++a.tupleof[0];
}

On Monday, 22 October 2018 at 13:40:39 UTC, Timon Gehr wrote:
> module reborked;
> import atomic;
>
> void main()@safe{
>     auto a=new Atomic!int;
>     import std.concurrency;
>     spawn((shared(Atomic!int)* a){ ++*a; }, a);
>     ++a.tupleof[0];
> }

Finally! Proof that MP is impossible. On the other hand, why the hell is that @safe? It breaks all sorts of guarantees about @safety. At a minimum, that should be un-@safe.

Filed in bugzilla: https://issues.dlang.org/show_bug.cgi?id=19326

--
  Simen