[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

Dlang Bot  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #20 from Dlang Bot  ---
dlang/dmd pull request #9782 "Fix 15984 - [REG2.071] Interface contracts
retrieve garbage instead of parameters" was merged into stable:

- be4013b2aa1f5c7076869cbd9d1a48f3908277b6 by سليمان السهمي  (Suleyman Sahmi):
  Fix issue 15984 - Pass parameters explicitly by ref to contract functions

https://github.com/dlang/dmd/pull/9782

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #19 from Dlang Bot  ---
@SSoulaimane updated dlang/dmd pull request #9782 "Fix 15984 - [REG2.071]
Interface contracts retrieve garbage instead of parameters" fixing this issue:

- Fix issue 15984 - Pass parameters explicitly by ref to contract functions

https://github.com/dlang/dmd/pull/9782

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

Walter Bright  changed:

   What|Removed |Added

 CC||bugzi...@digitalmars.com

--- Comment #18 from Walter Bright  ---
https://github.com/dlang/dmd/pull/9030
as a replacement for
https://github.com/dlang/dmd/pull/8988
which is a rebase of
https://github.com/dlang/dmd/pull/5765

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

FeepingCreature  changed:

   What|Removed |Added

 CC||default_357-l...@yahoo.de

--- Comment #17 from FeepingCreature  ---
Helpful reminder that it's Q4 2018 and DMD still generates crashing code with
no warning when using a simple, officially documented and advertised language
feature.

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #16 from MichaelZ  ---
(In reply to robin.kupper from comment #15)
> While the first example works now, parameters in out contracts on interface
> functions are still garbage.
> 
> See example https://run.dlang.io/is/c5XDwJ.

Thaynes Example is a little shorter and also still fails, including incorrect
values in the "in" contract.


> Placing the contract on the implementing function currently seems to be the
> only  workaround.

Is this really an issue that purely occurs with interfaces?  I was under the
impression that inheritance can trigger this as well, but I may be mistaken.

If you're right, then can we - pending a "real" solution - have the compiler
just ignore interface contracts (and emit a warning), rather than generating
bad code?

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

robin.kup...@rwth-aachen.de changed:

   What|Removed |Added

 CC||robin.kup...@rwth-aachen.de

--- Comment #15 from robin.kup...@rwth-aachen.de ---
While the first example works now, parameters in out contracts on interface
functions are still garbage.

See example https://run.dlang.io/is/c5XDwJ.

Placing the contract on the implementing function currently seems to be the
only  workaround.

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #14 from ki...@gmx.net ---
> changing all nested functions to use closures allocated on the stack or GC 
> (depending on escaping)

FWIW, that's what LDC does.

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #13 from Martin Nowak  ---
(In reply to anonymous4 from comment #12)
> AFAIK, normal nested function doesn't trigger closure allocation if it's
> only called and its delegate is not taken.

That's why it said "special nested function", although changing all nested
functions to use closures allocated on the stack or GC (depending on escaping)
would get rid of the tricky direct addressing in the stacks of parent
functions.

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #12 from anonymous4  ---
AFAIK, normal nested function doesn't trigger closure allocation if it's only
called and its delegate is not taken.

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #11 from Martin Nowak  ---
An analysis of the current state of affairs, took me a while to dig through the
numerous hacks, related changes and PRs.

== Context information ==

- Accessing upvalues (variables from outer scopes) in nested functions is
currently done via 2 different mechanisms, either by passing a pointer to a GC
closure [¹] or by direct-stack access [²][³]. The layouts of those 2 are very
different and it would be difficult to make them identical, the former is
generated by the frontend the latter by the backend.

- frequire (in contracts) gets treated like a nested function and would
therefor also uses either of the access variants. At the moment frequire also
supports escaping of parameters (triggers a GC closure allocation), just like a
normal nested function.

- frequire of base classes/interfaces are also called as nested functions, but
from the class implementing/overriding the method. So currently Derived.foo is
calling Base.foo.frequire (nested in Base.foo), but uses the access method
(passes sclosure or sthis pointer) for Derived.foo.frequire.

- Code for Base.foo is generated independent of any derived classes, thus the
current implementation requires that all nested frequire implementations use
the same access method, or that both access methods have the same memory
layout. Furthermore the offsets of all accessed variables must be identical.

- Whether or not a closure for parameters and variables is allocated, depends
on the individual and separately compiled, i.e. invisible, implementation of
each implementing/overriding method (see issue 9383).

[¹]:
[buildClosure](https://github.com/dlang/dmd/blob/e087760eda0dd0242d6edb1c4acdfea36bce8821/src/toir.d#L779)
[²]:
[getEthis](https://github.com/dlang/dmd/blob/0e4152265a2e16ca49ea8ea34a82109ce6c59cbc/src/toir.d#L99)
[³]:
[e2ir/SymbolExp](https://github.com/dlang/dmd/blob/e087760eda0dd0242d6edb1c4acdfea36bce8821/src/e2ir.d#L1049))

== Recent attempts at fixing the problem ==

Issue 9383 was somewhat fixed by https://github.com/dlang/dmd/pull/4788. This
is a hack that tried to make frequire a special nested function, that will
always access upvalues directly through the stack [¹].
It relies on very specific and fragile backend details of the function
prologue. 

- As a first step in a function with GC closure, memory will be allocated.
Before calling _d_allocmemory, all parameters need to be saved to the stack.
So indeed anything that could be referenced from a nested frequire is already
on the stack.

- But frequire is only called after the closure allocation has been completed,
so all parameters (and variables) have already been moved to the GC closure,
and the variable offsets in the frontend are adjusted accordingly.

- The forceStackAccess hack ignores those offsets, and instead uses the now
stale backend offsets for the initial parameter spilling [²].

- Nobody ever told the backend, that those initial stack locations are
referenced by the frontend.

This of course led to subtle bugs when trying to bend (in the frontend) how
parameters are accessed (https://github.com/dlang/dmd/pull/5420/files).

https://github.com/dlang/dmd/pull/5765 tries to fix `this` usage in interfaces,
b/c accessing that was bend to the spilled `this` parameter in some class'
function implementation. The idea of that PR is to pass the class-to-interface
offset as "hidden" argument to the nested frequire function.

[¹]:
https://github.com/dlang/dmd/pull/4788/files#diff-6e3ab8a500e476994f345ede433811bbR961
[²]:
https://github.com/dlang/dmd/pull/4788/files#diff-6e3ab8a500e476994f345ede433811bbR982

== Currently remaining issues ==

- When the parameters aren't used in the implementation class, they won't get
saved on the stack and the nested frequire function will find garbage instead.
  This works for normal nested functions, so likely some of the frequire hacks
break this.

- Adjusting the this pointer for interfaces isn't yet solved, the PR adds a lot
more hacks to an already hard to maintain implementation.

- Escaping parameters in frequire, e.g. by using them in a delegate, will no
longer work when being forced to use the stack access.

== Proposed resolution ==

- We make frequire a special nested function that always get's called with a
closure. When no GC closure is needed, the closure can be allocated with the
same layout on the stack.

- Allows to get rid of existing Ffakeeh, frame pointer, and function prolog
hacks.

- The variable locations in the closure (on the stack) can replace any other
stack location of that variable, so no overhead on stack space should be
necessary. It might be somewhat tricky to get the backend to enregister
variables after the frequire contracts have been validated.

- One way to support interfaces would be to set a 'this' pointer in the closure
to the correct offset before each 

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

Martin Nowak  changed:

   What|Removed |Added

 CC||c...@dawg.eu

--- Comment #10 from Martin Nowak  ---
Apparently introduced by https://github.com/dlang/dmd/pull/4788.

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

--- Comment #9 from MichaelZ  ---
If this can't be readily fixed, then what exactly must we do (or rather, not
do) in our code to avoid triggering the issue?

Must we avoid using pre- (and post?) conditions in an interface at all?
Or must we avoid using pre- (and post?) conditions on 'override' functions?
Or something else entirely?

So far we've had the issue pop up roughly 4 times, some of those after making
apparently unrelated changes; cleaning up the problematic places once and for
all now that we have the issue in mind, will avoid frustrating bug hunting in
the future...

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

MichaelZ  changed:

   What|Removed |Added

 CC||dlang@bregalad.de

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

anonymous4  changed:

   What|Removed |Added

 CC||be...@caraus.de

--- Comment #8 from anonymous4  ---
*** Issue 16565 has been marked as a duplicate of this issue. ***

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

Thayne  changed:

   What|Removed |Added

 CC||astrotha...@gmail.com

--- Comment #7 from Thayne  ---
I still see this in dmd 2.071.1, and I see similar behavior for out contracts:

---
import std.stdio;
import std.conv;
import std.algorithm : canFind;

interface I {
string foo(int input)
in {
writefln("In in, input = %s", input);
}
out (result) {
writefln("In out, input = %s", input);
writefln("Result = %s", result);
assert(result.canFind(to!string(input)));
}
}

class C : I {
string foo(int input) in { assert(false); } body {
writefln("Input = %s", input);
return "Foo " ~ to!string(input);
}
}

void main() {
auto c = new C;

c.foo(5);
}
---

--

[Issue 15984] [REG2.071] Interface contracts retrieve garbage instead of parameters

[via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=15984

Kenji Hara  changed:

   What|Removed |Added

   Keywords||pull
Summary|Interface contracts |[REG2.071] Interface
   |retrieve garbage instead of |contracts retrieve garbage
   |parameters  |instead of parameters

--- Comment #6 from Kenji Hara  ---
https://github.com/dlang/dmd/pull/5765

--