std.typecons: Make RefCounted pure nothrow @nogc when possible by nordlow · Pull Request #4832 · dlang/phobos

nordlow · 2016-10-02T22:01:10Z

This makes RefCounted!T pure/nothrow/@nogc when possible.

andralex · 2016-10-02T23:40:41Z

std/typecons.d

+    // qualified C memory allocations
+    private @safe pure nothrow @nogc:
+    void* malloc(size_t size);
+    void free(void* ptr);


free is not and cannot be safe

Ok. But its usage here can be @trusted.

I see, you made it private too. Hmmm...

andralex · 2016-10-03T00:02:18Z

OK how about you keep the safe but for now drop the pure. The notion of marking malloc as pure is disquieting - it never returns the same void* for the same number. Then we'll see how we can do about pure. Thanks!

jmdavis · 2016-10-03T03:34:49Z

The notion of marking malloc as pure is disquieting - it never returns the same void* for the same number.

I would point out that we're in the same boat with the GC and new. It's pure and doesn't necessarily return the same value - just an equivalent one. The only real difference is that with the GC, we have something to manage freeing that memory, whereas since you have to worry about it with malloc, you have to be concerned about whether you really do get the same value so that you don't free it twice.

So, I don't know what the right answer is, but there's definitely an argument that malloc should be considered pure given what we do with new.

andralex · 2016-10-03T03:43:13Z

@jmdavis new is special so we can special case it

nordlow · 2016-10-03T07:55:15Z

AFAICT, C-style memory allocations cannot be qualified correctly in D using pure. For instance if we declare malloc as pure the optimizer will optimize away successive calls to malloc which is incorrect. On the other hand, the return value of malloc shall not in the RefCounted case have any side-effect on the program. Only the values that are stored in it should. This semantics cannot currently be expressed in D.

Instead we have to qualify the code that it's using C-style allocation logic and privately import *alloc and free locally for each use. I believe the same holds for GC.addRange.

aG0aep6G · 2016-10-03T09:01:32Z

@andralex:

The notion of marking malloc as pure is disquieting - it never returns the same void* for the same number.

That's not a requirement of D's pure attribute. Or if it is, we have to fix the compiler to enforce it and probably break lots of code.

new is special so we can special case it

GC.malloc and GC.free aren't really special, though. They can be used just like C's malloc and free. Yet GC.malloc and GC.free are pure and the C functions are not. I don't see the reason for the disparity.

GC.malloc being (weakly) pure is ok because void* has a mutable indirection, so the compiler must not reuse the result (but: issue 15862). GC.free being (weakly) pure is ok because the visible effect it has is on its mutable argument. The same points apply to C's malloc and free.

See also http://forum.dlang.org/post/hrwnhprgcjlbypaecpzg@forum.dlang.org.

@nordlow:

if we declare malloc as pure the optimizer will optimize away successive calls to malloc

Actually, the compiler must not do that. It does right now, but that's issue 15862.

andralex · 2016-10-03T12:51:36Z

@aG0aep6G I think it's a slippery slope that GC.malloc and free are marked as pure. I don't like that, and I don't like that used as a precedent.

There must be some acknowledgment at a point that our type system has limitations that doesn't allow it to express certain valid programs.

aG0aep6G · 2016-10-03T13:42:10Z

@aG0aep6G I think it's a slippery slope that GC.malloc and free are marked as pure. I don't like that, and I don't like that used as a precedent.

This needs to be sorted out. Either it's wrong that GC.malloc and GC.free are marked pure, or we should be able to mark C's malloc and free too. Either we have a disaster waiting to happen with the GC functions, or we're needlessly preventing code from being pure.

There must be some acknowledgment at a point that our type system has limitations that doesn't allow it to express certain valid programs.

Ok, but I don't think it's a good idea to draw that line between GC.malloc and C's malloc.

andralex · 2016-10-03T21:12:04Z

Hmmm, this notion of a function that returns data with mutable indirection is very interesting. In particular, if the data returned has mutable indirections leading to values that cannot be present in the parameters, that means that's "fresh" data, i.e. newly allocated. Consider:

pure int[] fun(float[]);

Without even looking at the definition, the compiler can infer that the array of integers is freshly-allocated for the simple reason there's no other place it could come from :o). Being able to characterize "fresh" without making language addition can be a big win.

For now, I agree that fixing https://issues.dlang.org/show_bug.cgi?id=15862 would allow us to make malloc pure.

aG0aep6G · 2016-10-03T22:09:51Z

In particular, if the data returned has mutable indirections leading to values that cannot be present in the parameters, that means that's "fresh" data, i.e. newly allocated.

Yeah, dmd uses this to allow implicit conversion to immutable when the value comes from a pure function (and other requirements are met). This example compiles with pure and is correctly rejected without it:

int* f() pure { return new int; }
void main() { immutable p = f(); }

Consider:
pure int[] fun(float[]);
Without even looking at the definition, the compiler can infer that the array of integers is freshly-allocated for the simple reason there's no other place it could come from :o).

I'm not so sure about that example. fun is allowed to cast the argument to int[], isn't it? Then again, dmd seems to disregard that possibility, too. This is currently accepted:

pure int[] fun(float[] arg) { return cast(int[]) arg; }
void main() { float[] a = [1, 2, 3]; immutable i = fun(a); }

If dmd is correct here, that would mean casting from one array type to another is invalid (aka has undefined behavior), wouldn't it? That doesn't seem right. I'd say this is another bug in dmd, and it cannot be assumed that fun returns a unique result. Filed as issue 16585.

nordlow · 2016-10-05T16:58:46Z

@andralex I thought you had decided upon non-safety in #4692 ...

codecov-io · 2016-10-06T10:35:02Z

Current coverage is 89.27% (diff: 75.00%)

Merging #4832 into master will increase coverage by <.01%

@@             master      #4832   diff @@
==========================================
  Files           121        121          
  Lines         76114      76116     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          67950      67956     +6   
+ Misses         8164       8160     -4   
  Partials          0          0

Powered by Codecov. Last update 08c587e...1055e95

andralex · 2016-10-13T01:11:32Z

@andralex I thought you had decided upon non-safety in #4692 ...

@nordlow not sure I understand?

wilzbach · 2016-12-21T14:39:47Z

For now, I agree that fixing https://issues.dlang.org/show_bug.cgi?id=15862 would allow us to make malloc pure.

The bug has been fixed ;-)

dlang/dmd#6197

So what's still blocking this?

nordlow · 2016-12-21T15:06:39Z

The RefCounted destructor calls free. It too has to be marked as pure. I can look into making it pure aswell and then go forward with this.

schveiguy · 2016-12-23T21:22:38Z

I think instead of marking free pure to make this PR successful, we can work around the purity.

Make a private binding to free, something like:

pragma(mangle, "free") extern(C) private purefree(void *mem) pure;

Then you can call it in one place where we can control the context, and not expose the whole world.

EDIT:

Well, now that I look at your PR, that's pretty much exactly what you are doing! Why would this require making the actual binding in core.stdc pure?

nordlow · 2017-01-06T12:15:51Z

@schveiguy, It doesn't. I just thought making free in core.stdc pure was the direction @andralex wanted. Andrei?

Further, we need to do the same for gc_addRange and gc_removeRange. Is that ok too, @andralex?

andralex · 2017-01-07T14:33:36Z

Let's go with the private declaration for now. Yes, in the future we do want to make functions like free pure.

nordlow · 2017-01-07T23:28:06Z

Ok, does that include gc_addRange and gc_removeRange?
And should I use the @schveiguy version with custom mangling:

pragma(mangle, "free") extern(C) private purefree(void *mem) pure;

or my existing?

andralex · 2017-01-07T23:34:31Z

yes those should be fine too (or at least subject to the same subtleties)
@schveiguy 's seems a bit clearer

nordlow · 2017-01-10T11:54:37Z

Made it so, @andralex

andralex

All good, please fix coverage.

andralex · 2017-01-10T13:03:49Z

std/typecons.d

                onOutOfMemoryError();
            static if (hasIndirections!T)
-                GC.addRange(&_store._payload, T.sizeof);
+                pure_gc_addRange(&_store._payload, T.sizeof);


line not covered

andralex · 2017-01-10T13:04:20Z

std/typecons.d

 {
+    extern(C) private pure nothrow @nogc static // TODO remove pure when https://issues.dlang.org/show_bug.cgi?id=15862 has been fixed
+    {
+        pragma(mangle, "free") void pure_free( void *ptr );


unusual naming convention (pureFree would be preferred)

Ok to leave it as this?

andralex · 2017-01-10T13:04:41Z

std/typecons.d

+    extern(C) private pure nothrow @nogc static // TODO remove pure when https://issues.dlang.org/show_bug.cgi?id=15862 has been fixed
+    {
+        pragma(mangle, "free") void pure_free( void *ptr );
+        pragma(mangle, "gc_addRange") void pure_gc_addRange( in void* p, size_t sz, const TypeInfo ti = null );


Eh, apparently not that unusual :)

That's why I used underscore instead. Ok to leave it as this? Or should I use pureGCAddRange?

Kill the TODO, that bug has been fixed, and you still need pure anyway.

nordlow

Feedback on local namings.

nordlow · 2017-01-10T14:48:19Z

std/typecons.d

 {
+    extern(C) private pure nothrow @nogc static // TODO remove pure when https://issues.dlang.org/show_bug.cgi?id=15862 has been fixed
+    {
+        pragma(mangle, "free") void pure_free( void *ptr );


Ok to leave it as this?

nordlow · 2017-01-10T14:48:21Z

std/typecons.d

+    extern(C) private pure nothrow @nogc static // TODO remove pure when https://issues.dlang.org/show_bug.cgi?id=15862 has been fixed
+    {
+        pragma(mangle, "free") void pure_free( void *ptr );
+        pragma(mangle, "gc_addRange") void pure_gc_addRange( in void* p, size_t sz, const TypeInfo ti = null );


That's why I used underscore instead. Ok to leave it as this? Or should I use pureGCAddRange?

andralex · 2017-01-10T15:32:35Z

Don't worry about the name, let's get the coverage. Thx!

nordlow · 2017-01-10T23:24:40Z

std/typecons.d

+{
+    struct S { int* p; }
+
+    auto s = RefCounted!S(null);


This makes line 5003 covered.

nordlow · 2017-01-12T09:40:00Z

Next up is Mallocator. Right, @andralex?

schveiguy · 2017-01-12T15:39:10Z

I think this looks good. Let's wait and see whether dlang/druntime#1735 is merged or not. If so, then you need to have a pure_malloc as well.

nordlow · 2017-01-14T20:00:02Z

If dlang/druntime#1735 gets merged we can use pureMalloc instead, right?

Update: purity was reverted in dlang/druntime#1735

Should we merge this or use pureMalloc instead? If so, where should we put it (along wit pureCalloc and pureRelloc)?

andralex · 2017-01-30T18:17:02Z

@nordlow I guess this is next!

nordlow · 2017-01-30T18:47:23Z

Made it use pureMalloc.

What about local purity of free, gc_addRange and gc_removeRange? Shall we keep the locally/privately purefied versions of them here as is, @andralex?

andralex · 2017-01-31T13:18:45Z

local is fine for now

nordlow · 2017-01-31T15:37:10Z

Changed naming of local wrappers to D-style casing.

This is ready, @andralex !

Acked-by: Per Nordlöw <per.nordlow@gmail.com>

andralex · 2017-02-01T01:51:20Z

@nordlow you have style failures, see ci/circleci

JackStouffer · 2017-02-01T03:00:12Z

@andralex That's actually a dscanner error. It isn't working correctly with the new DIP1000 syntax.

nordlow · 2017-02-01T11:15:47Z

So what to do until DIP-1000 works, @andralex ?

andralex · 2017-02-01T13:25:07Z

Auto-merge toggled on

nordlow · 2017-02-01T13:48:55Z

Great!

nordlow changed the title ~~Make RefCounted pure when possible~~ std.typecons: Make RefCounted pure when possible Oct 2, 2016

nordlow force-pushed the pure-refcounted branch from 895012c to 642710c Compare October 2, 2016 22:05

andralex reviewed Oct 2, 2016

View reviewed changes

nordlow force-pushed the pure-refcounted branch from 642710c to 9894282 Compare October 3, 2016 17:36

nordlow force-pushed the pure-refcounted branch from 9894282 to 1055e95 Compare October 3, 2016 21:17

nordlow force-pushed the pure-refcounted branch from 1055e95 to b3ef825 Compare October 6, 2016 10:35

JackStouffer added the Merge:Blocked label Oct 26, 2016

wilzbach removed the Merge:Blocked label Dec 21, 2016

This was referenced Dec 21, 2016

Qualify free(void*) as pure dlang/druntime#1718

Closed

Qualify GC range add and remove as pure dlang/druntime#1719

Closed

nordlow force-pushed the pure-refcounted branch from b3ef825 to d2802dc Compare January 10, 2017 11:54

nordlow force-pushed the pure-refcounted branch from d2802dc to a9e7c9b Compare January 10, 2017 12:00

andralex approved these changes Jan 10, 2017

View reviewed changes

nordlow commented Jan 10, 2017

View reviewed changes

nordlow changed the title ~~std.typecons: Make RefCounted pure when possible~~ std.typecons: Make RefCounted pure nothrow @nogc when possible Jan 10, 2017

nordlow force-pushed the pure-refcounted branch from a9e7c9b to f036125 Compare January 10, 2017 23:24

nordlow commented Jan 10, 2017

View reviewed changes

std/typecons.d

{

struct S { int* p; }

auto s = RefCounted!S(null);

Copy link

Contributor Author

nordlow Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes line 5003 covered.

nordlow mentioned this pull request Jan 17, 2017

Add pure wrappers for C style allocation dlang/druntime#1746

Merged

nordlow force-pushed the pure-refcounted branch from f036125 to 4a9da6e Compare January 30, 2017 20:37

Make RefCounted pure when possible

ddebfaf

Acked-by: Per Nordlöw <per.nordlow@gmail.com>

nordlow force-pushed the pure-refcounted branch from 4a9da6e to ddebfaf Compare January 31, 2017 15:43

andralex merged commit 27a1553 into dlang:master Feb 1, 2017

nordlow mentioned this pull request Jun 11, 2017

Add pureFree to core.memory dlang/druntime#1836

Merged

nordlow deleted the pure-refcounted branch October 19, 2021 19:45

Uh oh!

Conversation

nordlow commented Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Oct 3, 2016

Uh oh!

jmdavis commented Oct 3, 2016

Uh oh!

andralex commented Oct 3, 2016

Uh oh!

nordlow commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aG0aep6G commented Oct 3, 2016

Uh oh!

andralex commented Oct 3, 2016

Uh oh!

aG0aep6G commented Oct 3, 2016

Uh oh!

andralex commented Oct 3, 2016

Uh oh!

aG0aep6G commented Oct 3, 2016

Uh oh!

nordlow commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 6, 2016

Current coverage is 89.27% (diff: 75.00%)

Uh oh!

andralex commented Oct 13, 2016

Uh oh!

wilzbach commented Dec 21, 2016

Uh oh!

nordlow commented Dec 21, 2016

Uh oh!

schveiguy commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordlow commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Jan 7, 2017

Uh oh!

nordlow commented Jan 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Jan 7, 2017

Uh oh!

nordlow commented Jan 10, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nordlow commented Oct 2, 2016 •

edited

Loading

nordlow commented Oct 3, 2016 •

edited

Loading

nordlow commented Oct 5, 2016 •

edited

Loading

schveiguy commented Dec 23, 2016 •

edited

Loading

nordlow commented Jan 6, 2017 •

edited

Loading

nordlow commented Jan 7, 2017 •

edited

Loading

nordlow commented Jan 14, 2017 •

edited

Loading

nordlow commented Jan 30, 2017 •

edited

Loading

nordlow commented Jan 31, 2017 •

edited

Loading

nordlow commented Feb 1, 2017 •

edited

Loading