www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Feedback Thread: DIP 1035-- system Variables--Community Review Round 1

reply Mike Parker <aldacron gmail.com> writes:
This is the feedback thread for the first round of Community 
Review of DIP 1035, " system Variables".

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules 
outlined in the Reviewer Guidelines (and listed at the bottom of 
this post).

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

That document also provides guidelines on contributing feedback 
to a DIP review. Please read it before posting here. If you would 
like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/tgtrbqrjetdveznzxokh forum.dlang.org
==================================

You can find DIP 1035 here:

https://github.com/dlang/DIPs/blob/148c78e6e7eeb5609715cb31a45c0aa5c8ebdce7/DIPs/DIP1035.md

The review period will end at 11:59 PM ET on June 24, or when I 
make a post declaring it complete. Feedback posted to this thread 
after that point may be ignored.

At the end of this review round, the DIP will be moved into the 
Post-Community Round 1 state. Significant revisions resulting 
from this review round may cause the DIP manager to require 
another round of Community Review, otherwise the DIP will be 
queued for the Final Review.

==================================
Posts in this thread that do not adhere to the following rules 
will be deleted at the DIP author's discretion:

* All posts must be a direct reply to the DIP manager's initial 
post, with only two exceptions:

     - Any commenter may reply to their own posts to retract 
feedback contained in the original post

     - The DIP author may (and is encouraged to) reply to any 
feedback solely to acknowledge the feedback with agreement or 
disagreement (preferably with supporting reasons in the latter 
case)

* Feedback must be actionable, i.e., there must be some action 
the DIP author can choose to take in response to the feedback, 
such as changing details, adding new information, or even 
retracting the proposal.

* Feedback related to the merits of the proposal rather than to 
the contents of the DIP (e.g., "I'm against this DIP.") is 
allowed in Community Review (not Final Review), but must be 
backed by supporting arguments (e.g., "I'm against this DIP 
because..."). The supporting arguments must be reasonable. 
Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as 
bullet points (those who take the time to do an in-depth review 
and provide feedback in the form of answers to the questions in 
this document will receive much gratitude). Information 
irrelevant to the DIP or is not provided in service of clarifying 
the feedback is unwelcome.
Jun 10 2020
next sibling parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 10 June 2020 at 08:39:48 UTC, Mike Parker wrote:

 This is the feedback thread for the first round of Community 
 Review of DIP 1035, " system Variables".


Typo in the "Using private" section under "Alternatives":


 private only acts on the module level, so a  trusted member 
 function cannot assume anything about member functions just 
 because they are private.


s/member functions/member variables/
Jun 10 2020
parent Dennis <dkorpel gmail.com> writes:
On Wednesday, 10 June 2020 at 11:45:39 UTC, Paul Backus wrote:

 s/member functions/member variables/


Good catch.
Jun 10 2020
prev sibling next sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
On Wednesday, 10 June 2020 at 08:39:48 UTC, Mike Parker wrote:

In proposed-changes:


 struct T {
      system int y;
      system int z = 3; // allowed
     this(int y, int z)  safe {
         this.y = y; // allowed, this is initialization
         this.y = y; // second time disallowed, this is 
 assignment to a ` system` variable
         this.z = z; // disallowed, this is assignment
     }
 }


The last one (`this.z = z`) is not an assignment, it is 
initialization, and should be allowed; that is, unless you want 
to change the constructor spec for this case.

Further down:


 struct Wrapper(T) {
      system T t;
     this(T t)  trusted {
         this.t = t; // Oops! Calls a ` system` copy constructor
     }
 }


...is a bad example for two reasons. First, appropriate 
implementation of Wrapper won't be calling a copy constructor 
here, it will move. Second, T may have a  system destructor which 
would be called when constructor returns.

---


 bool becomes an unsafe type


Nice one.

---

In Examples:


 void main()  safe {
     Var v = 1.0;
     v.type = Var.Type.Str; // not allowed after this DIP
     writeln(v.getString()); // memory corruption: the union 
 messed up the string
 }


The last line is not memory corruption, it's undefined behavior.


 void main()  safe {
     auto stack = getStack!string();
     stack._top += 1; // not allowed after this DIP
     writeln(stack.pop); // memory corruption: garbage string is 
 printed
 }


Ditto.

---

A custom allocator.


 void main()  safe {
     string[] strArr = customAlloc!string(3);
     heapIndex = 0; // mess up allocator integrity! not allowed 
 after this DIP.
     int[] intArr = customAlloc!int(3);
     intArr[] = -1; // overwrites string array
     writeln(strArr[0]); // memory corruption: constructed 
 pointer
 }


The `intArr[] = -1;` is memory corruption. Last line, as before, 
is just UB.

---

A custom slice type

...is self-contradicting. "...one does not want 4 or 8 bytes to 
store the length when 2 suffices... ...SmallSlice!T is no larger 
than a regular T[]---it still fits in two CPU registers."


 struct SmallSlice(T) if (__traits(isPOD, T)) {
      system private T* ptr = null;
      system private ushort _length = 0;
     ubyte[size_t.sizeof-2] extraSpace; // 2 on 32-bit, 6 on 
 64-bit


How, then, is it "small", if SmallSlice!T.sizeof == (T[]).sizeof 
? That you only use two out of (4 or 8) bytes doesn't make it 
"small" :)

---

A zero-terminated string


 void main()  safe {
     Cstring c = cStringLiteral!"hello";
     c.ptr = new char('D'); // not allowed after this DIP
     writeln(c.length); // memory corruption: strlen likely goes 
 past the single 'D' character
 }


As before, that last line is not memory corruption, it's UB. 
Corruption happens on the "not allowed after this DIP" line.
Jun 10 2020
parent Dennis <dkorpel gmail.com> writes:
On Wednesday, 10 June 2020 at 12:36:13 UTC, Stanislav Blinov 
wrote:

 The last one (`this.z = z`) is not an assignment, it is 
 initialization, and should be allowed; that is, unless you want 
 to change the constructor spec for this case.


Yes. I tried out some examples with immutable to figure what dmd 
counts as initialization and what as assignment, but I think I 
made a mistake there.


 Further down:


 struct Wrapper(T) {
      system T t;
     this(T t)  trusted {
         this.t = t; // Oops! Calls a ` system` copy constructor
     }
 }


 ...is a bad example for two reasons. First, appropriate 
 implementation of Wrapper won't be calling a copy constructor 
 here, it will move. Second, T may have a  system destructor 
 which would be called when constructor returns.


Maybe the name Wrapper is misleading since it gives some semantic 
expectations. I'm considering simply calling it 'S'.
The fact that a  system constructor gets implicitly trusted as 
well only supports the example, which is trying to show the 
problems with  trusted constructors.


 As before, that last line is not memory corruption, it's UB.


Each example has two consecutive moments:
A) Memory gets in a state that violates the programmer's intended 
invariants.
B) Because of that, immutable memory is mutated, or memory 
outside a pointer's allocated block is accessed.

If I understand correctly, you claim that A) should be called 
memory corruption and B) is undefined behavior, while I call B) 
memory corruption. In the D specification, corrupting memory is 
undefined behavior, and undefined behavior can manifest in memory 
corruption, so when talking about  safe the terms are sometimes 
used interchangeably. Also, as far as the D language is 
concerned, there's nothing wrong with e.g. setting the pointer to 
Cstring to something not null terminated, only the resulting out 
of bounds memory accesses are.
Jun 10 2020
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 10.06.20 10:39, Mike Parker wrote:

 This is the feedback thread for the first round of Community Review of 
 DIP 1035, " system Variables".
 ...


I think this is a very well-written DIP that addresses an important problem.

However, I don't think the examples should be using `assert` to validate 
input data. At the very least, those asserts should be in `in` 
contracts, but even then, I am not sure if the semantics of `assert` 
supports your use case. In particular, does `-release` mean "disable 
memory safety checks" like `-noboundscheck` does?

(Besides that, probably `assert` should not be used at all, at least 
outside of contracts, if you care about memory safety:

https://dlang.org/spec/contracts.html

"Undefined Behavior: The subsequent execution of the program after an 
assert contract is false.")


Also, making initialization of ` system` variables ` safe` is not sound. 
` system` variables are variables that need to satisfy additional 
invariants. The constructor has to establish those invariants. Memory 
safety cannot depend on the correctness of a ` safe` constructor.

Consider the following slightly adapted example from the DIP:

enum Opcode : ubyte {
     decrement, increment, print,
}

struct VmInstruction {
      system Opcode opcode; // this need not be private, just a valid 
enum member
     this(Opcode opcode)  safe {
         this.opcode = opcode; // forgot to check
     }
}

int gCounter;
void decrementImpl() {gCounter++;};
void incrementImpl() {gCounter--;};
void printImpl() {import std; writeln(gCounter);};

immutable void function()[3] jumpTable = [
    &decrementImpl, &incrementImpl, &printImpl,
];

void execute(VmInstruction[] code)  trusted {
     foreach(instruction; code) {
         // indexing using .ptr to avoid bounds checks
         jumpTable.ptr[instruction.opcode]();
     }
}

void main()  safe {
     auto code = [VmInstruction(cast(Opcode)20)];
     execute(code);
}



Minor:
- "Ownsership and borrowing in D"
- "static initializtion"
Jun 10 2020
parent Dennis <dkorpel gmail.com> writes:
On Wednesday, 10 June 2020 at 13:56:11 UTC, Timon Gehr wrote:

 However, I don't think the examples should be using `assert` to 
 validate input data. At the very least, those asserts should be 
 in `in` contracts, but even then, I am not sure if the 
 semantics of `assert` supports your use case. In particular, 
 does `-release` mean "disable memory safety checks" like 
 `-noboundscheck` does?


Personally I often find myself writing `if (!x) {assert(0);}` 
instead of `assert(x);` for exactly that reason: I want to assert 
something that's necessary for  safe.
I'm not sure if that's the idiomatic way to do it though. I'll 
change the examples to use that unless you have a better 
suggestion.


 Memory safety cannot depend on the correctness of a ` safe` 
 constructor.



 Consider the following slightly adapted example from the DIP:


You could say that the problem is that `execute` is wrongly 
` trusted` since it assumed something about `opcode` that is not 
enforced by VmInstruction. I get what you're saying though, and 
it comes back to " trusted assumptions about  safe code", which 
unfortunately did not come to concensus.

https://forum.dlang.org/thread/rahiuh$2t8h$1 digitalmars.com?page=1

I'll come back to this later in the discussion thread, but the 
short version is:
Ideally all important trusted parts of the program have a 
 trusted annotation nearby, but I'm afraid it is infeasible. I 
was unable to find a satisfying design, though I'm open to 
suggestions.


 Minor:
 - "Ownsership and borrowing in D"
 - "static initializtion"


Thanks.
Jun 10 2020