More return scope fixes #77

skoppe · 2023-04-02T09:29:04Z

It turns out there is a slight difference between annotating a member function with scope return or return scope.

The scope in both return scope and scope return means you aren't allowed to escape a pointer member. It is also required if you want to call a method on a variable that has a scope annotation.

The return in scope return means you are allowed to return a reference to a struct's member. Whereas the return in return scope allows you to return pointer members.

struct RS {
    int* p;
    auto get() return scope {
        return p; // returning a pointer member
    }
}

struct SR {
    int p;
    auto get() scope return {
        return &p; // returning a reference to a member, essentially a pointer+offset to the struct
    }
}

Many of the connect functions were annotated with scope return, but we don't care much about the Sender's lifetime - they are just a collection of values needed to connect the async operation - but rather about the lifetime of Operation returned.

kinke · 2023-04-03T11:42:27Z

The ASan 'regressions' are caused by a druntime 2.102 change - exception traces are now malloc'd, not allocated by the GC anymore. And freed in the Throwable finalizer.

skoppe · 2023-04-03T15:58:52Z

The ASan 'regressions' are caused by a druntime 2.102 change - exception traces are now malloc'd, not allocated by the GC anymore. And freed in the Throwable finalizer.

But why do they leak though? You would expect the system to clean up after itself.

kinke · 2023-04-03T16:05:25Z

I'm not up to speed wrt. whether finalizers are called during program shutdown. And whether the ASan leaks are reported after they have run if they are.

kinke · 2023-04-11T13:02:51Z

I've run the ASan-enabled tests with LDC v1.32.0 with a custom druntime, featuring some printf outputs.

There are 63 Throwable.info mallocs in total.
56 of these are free'd.
81 Throwables are finalized in total; 60 with a non-null .info.
- The only finalized Throwables that have a non-null .info but a null .infoDeallocator are concurrency's ThrowableClone (fine). There are 4 of those, so 56 Throwables with non-null .info and non-null .infoDeallocator are finalized, matching the 56 .info frees above.
ASan complains about 6 objects leaking, but they should be 63-56=7?

Maybe the 7+ non-finalized Throwables are still ref'd at program shutdown somewhere? Or their finalization is skipped when terminating the program?

Edit: Or maybe these Throwables aren't even GC-allocated? Some are emplaced into their static init symbol in druntime IIRC (staticError).

kinke · 2023-04-11T13:12:00Z

Or maybe these Throwables aren't even GC-allocated? Some are emplaced into their static init symbol in druntime IIRC (staticError).

Looks like this is it. 7 Throwables are emplaced into static init symbols via staticError.

kinke · 2023-04-11T13:25:52Z

Little correction: Throwables emplaced into their init symbol actually don't get a trace (.info), there's an according special case in _d_createTrace(). BUT staticError emplaces into a separate TLS buffer: https://github.com/dlang/dmd/blob/f8cfdcd7ca96eb445cfda1b752904b34347aa1f4/druntime/src/core/exception.d#L856-L875

=> IMO safe to add _D4core7runtime19defaultTraceHandlerFPvZC6object9Throwable9TraceInfo to the ASan suppressions.

kinke · 2023-04-11T14:39:57Z

I can confirm that this fixes the compile errors we previously hit at Symmetry. There are still some deprecations, e.g.:

/home/mkinkelin/dev/concurrency/source/concurrency/stream/stream.d(79): Deprecation: scope variable `this` assigned to non-scope parameter `stream` calling `this`
/home/mkinkelin/dev/concurrency/source/concurrency/stream/tolist.d(20): Deprecation: scope variable `this` assigned to non-scope parameter `stream` calling `this`

kinke · 2023-04-11T15:01:14Z

With more attributes inference, there's a single remaining deprecation when compiling the stdlib/core-concurrency unittests:

diff --git a/source/concurrency/operations/then.d b/source/concurrency/operations/then.d
index 9f564e9..93a4570 100644
--- a/source/concurrency/operations/then.d
+++ b/source/concurrency/operations/then.d
@@ -58,7 +58,7 @@ struct ThenSender(Sender, Fun) if (models!(Sender, isSender)) {
   alias Value = ReturnType!fun;
   Sender sender;
   Fun fun;
-  auto connect(Receiver)(return Receiver receiver) @safe return scope {
+  auto connect(Receiver)(return Receiver receiver) @safe {
     alias R = ThenReceiver!(Receiver, Sender.Value, Fun);
     // ensure NRVO
     auto op = sender.connect(R(receiver, fun));
diff --git a/source/concurrency/stream/stream.d b/source/concurrency/stream/stream.d
index fb98f54..b8d3af2 100644
--- a/source/concurrency/stream/stream.d
+++ b/source/concurrency/stream/stream.d
@@ -74,7 +74,7 @@ auto fromStreamOp(StreamElementType, SenderValue, alias Op, Args...)(Args args)
     alias Value = SenderValue;
     Args args;
     DG dg;
-    auto connect(Receiver)(return Receiver receiver) @safe return scope {
+    auto connect(Receiver)(return Receiver receiver) @safe {
       // ensure NRVO
       auto op = Op!(Receiver)(args, dg, receiver);
       return op;
diff --git a/source/concurrency/stream/tolist.d b/source/concurrency/stream/tolist.d
index efae5bf..0f8f78b 100644
--- a/source/concurrency/stream/tolist.d
+++ b/source/concurrency/stream/tolist.d
@@ -15,7 +15,7 @@ struct ToListSender(Stream) {
   alias Properties = StreamProperties!Stream;
   alias Value = Properties.ElementType[];
   Stream stream;
-  auto connect(Receiver)(return Receiver receiver) @safe return scope {
+  auto connect(Receiver)(return Receiver receiver) @safe {
     // ensure NRVO
     auto op = ToListOp!(Stream, Receiver)(stream, receiver);
     return op;
diff --git a/source/concurrency/syncwait.d b/source/concurrency/syncwait.d
index 000816d..f62eb10 100644
--- a/source/concurrency/syncwait.d
+++ b/source/concurrency/syncwait.d
@@ -127,7 +127,7 @@ auto syncWait(Sender)(auto scope ref Sender sender) {
   return result;
 }
 
-private Result!(Sender.Value) syncWaitImpl(Sender)(auto scope ref Sender sender, StopSource stopSource) @safe {
+private Result!(Sender.Value) syncWaitImpl(Sender)(auto ref Sender sender, StopSource stopSource) @safe {
   import mir.algebraic : Algebraic, Nullable;
   static assert(models!(Sender, isSender));
   import concurrency.signal;

/home/mkinkelin/dev/SIL/stdlib/core-concurrency/test/ut/concurrency/stream.d(41): Deprecation: `@safe` function `__unittest_L36_C1` calling `syncWait`
/home/mkinkelin/dev/concurrency/source/concurrency/syncwait.d(124):        which would be `@system` because of:
/home/mkinkelin/dev/concurrency/source/concurrency/syncwait.d(124):        scope variable `sender` assigned to non-scope parameter `sender` calling `syncWaitImpl`

kinke · 2023-04-11T15:07:42Z

Oh, just needed another manual scope removal. No more deprecations for core-concurrency with:

diff --git a/source/concurrency/operations/then.d b/source/concurrency/operations/then.d
index 9f564e9..93a4570 100644
--- a/source/concurrency/operations/then.d
+++ b/source/concurrency/operations/then.d
@@ -58,7 +58,7 @@ struct ThenSender(Sender, Fun) if (models!(Sender, isSender)) {
   alias Value = ReturnType!fun;
   Sender sender;
   Fun fun;
-  auto connect(Receiver)(return Receiver receiver) @safe return scope {
+  auto connect(Receiver)(return Receiver receiver) @safe {
     alias R = ThenReceiver!(Receiver, Sender.Value, Fun);
     // ensure NRVO
     auto op = sender.connect(R(receiver, fun));
diff --git a/source/concurrency/stream/stream.d b/source/concurrency/stream/stream.d
index fb98f54..b8d3af2 100644
--- a/source/concurrency/stream/stream.d
+++ b/source/concurrency/stream/stream.d
@@ -74,7 +74,7 @@ auto fromStreamOp(StreamElementType, SenderValue, alias Op, Args...)(Args args)
     alias Value = SenderValue;
     Args args;
     DG dg;
-    auto connect(Receiver)(return Receiver receiver) @safe return scope {
+    auto connect(Receiver)(return Receiver receiver) @safe {
       // ensure NRVO
       auto op = Op!(Receiver)(args, dg, receiver);
       return op;
diff --git a/source/concurrency/stream/tolist.d b/source/concurrency/stream/tolist.d
index efae5bf..0f8f78b 100644
--- a/source/concurrency/stream/tolist.d
+++ b/source/concurrency/stream/tolist.d
@@ -15,7 +15,7 @@ struct ToListSender(Stream) {
   alias Properties = StreamProperties!Stream;
   alias Value = Properties.ElementType[];
   Stream stream;
-  auto connect(Receiver)(return Receiver receiver) @safe return scope {
+  auto connect(Receiver)(return Receiver receiver) @safe {
     // ensure NRVO
     auto op = ToListOp!(Stream, Receiver)(stream, receiver);
     return op;
diff --git a/source/concurrency/syncwait.d b/source/concurrency/syncwait.d
index 000816d..46fbe8e 100644
--- a/source/concurrency/syncwait.d
+++ b/source/concurrency/syncwait.d
@@ -115,7 +115,7 @@ auto syncWait(Sender, StopSource)(auto ref Sender sender, StopSource stopSource)
   return syncWaitImpl(sender, (()@trusted=>cast()stopSource)());
 }
 
-auto syncWait(Sender)(auto scope ref Sender sender) {
+auto syncWait(Sender)(auto ref Sender sender) {
   import concurrency.signal : globalStopSource;
   auto childStopSource = new shared StopSource();
   StopToken parentStopToken = parentStopSource ? StopToken(parentStopSource) : StopToken(globalStopSource);
@@ -127,7 +127,7 @@ auto syncWait(Sender)(auto scope ref Sender sender) {
   return result;
 }
 
-private Result!(Sender.Value) syncWaitImpl(Sender)(auto scope ref Sender sender, StopSource stopSource) @safe {
+private Result!(Sender.Value) syncWaitImpl(Sender)(auto ref Sender sender, StopSource stopSource) @safe {
   import mir.algebraic : Algebraic, Nullable;
   static assert(models!(Sender, isSender));
   import concurrency.signal;

kinke · 2023-04-11T15:11:24Z

... but this patch segfaults DMD & LDC v2.101/2 when compiling the concurrency unittests here...

skoppe · 2023-04-11T20:52:13Z

Thanks for looking into this @kinke

I can confirm that this fixes the compile errors we previously hit at Symmetry. There are still some deprecations, e.g.:

/home/mkinkelin/dev/concurrency/source/concurrency/stream/stream.d(79): Deprecation: scope variable `this` assigned to non-scope parameter `stream` calling `this`
/home/mkinkelin/dev/concurrency/source/concurrency/stream/tolist.d(20): Deprecation: scope variable `this` assigned to non-scope parameter `stream` calling `this`

Where are these deprecations exactly?

Oh, just needed another manual scope removal. No more deprecations for core-concurrency with:

Hmm, removing scope is really not an option.

nordlow

LGTM.

nordlow · 2023-04-12T11:37:39Z

No merge yet?

More return scope fixes

d626208

ASan: Account for a few new harmless leaks with druntime v2.102

2b2cd68

kinke mentioned this pull request Apr 11, 2023

Avoid GC for allocating traceinfo dlang/dmd#14710

Merged

nordlow reviewed Apr 12, 2023

View reviewed changes

skoppe merged commit 25a61c3 into master Apr 12, 2023

kinke deleted the returnscope branch April 12, 2023 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More return scope fixes #77

More return scope fixes #77

skoppe commented Apr 2, 2023 •

edited

Loading

kinke commented Apr 3, 2023

skoppe commented Apr 3, 2023

kinke commented Apr 3, 2023

kinke commented Apr 11, 2023 •

edited

Loading

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023 •

edited

Loading

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023 •

edited

Loading

skoppe commented Apr 11, 2023

nordlow left a comment

nordlow commented Apr 12, 2023

More return scope fixes #77

More return scope fixes #77

Conversation

skoppe commented Apr 2, 2023 • edited Loading

kinke commented Apr 3, 2023

skoppe commented Apr 3, 2023

kinke commented Apr 3, 2023

kinke commented Apr 11, 2023 • edited Loading

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023 • edited Loading

kinke commented Apr 11, 2023

kinke commented Apr 11, 2023 • edited Loading

skoppe commented Apr 11, 2023

nordlow left a comment

Choose a reason for hiding this comment

nordlow commented Apr 12, 2023

skoppe commented Apr 2, 2023 •

edited

Loading

kinke commented Apr 11, 2023 •

edited

Loading

kinke commented Apr 11, 2023 •

edited

Loading

kinke commented Apr 11, 2023 •

edited

Loading