Add pure wrappers for C style allocation by nordlow · Pull Request #1746 · dlang/druntime

nordlow · 2017-01-17T00:19:43Z

In response to dlang/phobos#4832 (comment)

PetarKirov · 2017-01-18T13:17:16Z

src/core/stdc/stdlib.d

+///
+void*   pureMalloc(size_t size) pure
+{
+    const errno = pureGetErrno();


Why not use void* or auto here? malloc's return type isn't going to change anytime soon .

Changed return type to void*.

PetarKirov · 2017-01-18T13:27:26Z

src/core/stdc/stdlib.d

+    cast(void)pureSetErrno(errno);
+    return ret;
+}
+extern (C) pure pragma(mangle, "getErrno") int pureGetErrno(); // for internal use


Checking in with LDC developers @kinke @JohanEngelen, does this work on LDC?

BTW, I bumped into a related extern (C) issue on LDC - #1728 (comment)

Also, since those functions are for internal use, why not make them private or package, or declare them inside pureMalloc? They should be used only in a controlled manner, and reviewed case-by-case.

Also, since those functions are for internal use,

I think the plan is to make this public

does this work on LDC?

Multiple extern (C) forward-declarations with the same mangled name aren't a problem. This works.

So are you saying I should rename these declarations to getErrno() and setErrno()?

Well just saying that ldc-developers/ldc#1020 doesn't apply to these extern (C) fwd declarations. While declaring them with their real name should work too, I think the explicit pure in the function name makes a lot of sense in this case.

Ok. I keep it as that for now.

This currently fails to compile as

stdlib.d(166,23): Error: pure function 'core.stdc.stdlib.pureMalloc' cannot call impure function 'core.stdc.stdlib.malloc'

I guess I have to use a local pure declaration of malloc as _pureMalloc then?

JackStouffer · 2017-01-18T13:47:31Z

Andrei stated this should go in core/memory.d

nordlow · 2017-01-18T15:42:06Z

So, should I move it to core/memory.d, @andralex?

nordlow · 2017-01-18T15:48:25Z

Should I add similar wrappers for calloc and realloc?

JackStouffer · 2017-01-18T16:20:45Z

Should I add similar wrappers for calloc and realloc?

Yes

So, should I move it to core/memory.d

#1735 (comment)

nordlow · 2017-01-18T22:49:33Z

Done.

kinke · 2017-01-18T23:02:23Z

What about potential race conditions when restoring errno? The docs here don't mention errno at all, instead:

Data races: Only the storage referenced by the returned pointer is modified. No other storage locations are accessed by the call.

And here:

malloc is thread-safe: it behaves as though only accessing the memory locations visible through its argument, and not any static storage.

JackStouffer · 2017-01-18T23:39:09Z

Pretty sure errno is thread local.

nordlow · 2017-01-19T00:09:37Z

http://stackoverflow.com/questions/1694164/is-errno-thread-safe#1694170

andralex · 2017-01-19T07:45:22Z

src/core/memory.d

+{
+    const errno = _pureGetErrno();
+    void* ret = _pureMalloc(size);
+    cast(void)_pureSetErrno(errno);


space after closing paren in cast - cc @wilzbach

andralex · 2017-01-19T07:45:43Z

src/core/memory.d

+/// Pure variant of `malloc` that doesn't affect global variable `errno`.
+void* pureMalloc(size_t size) pure
+{
+    const errno = _pureGetErrno();


Does druntime use this convention of prefixing private symbols with one underscore?

Renamed it.

andralex · 2017-01-19T07:47:36Z

src/core/memory.d

+    pragma(mangle, "malloc") void* _pureMalloc(size_t);
+    pragma(mangle, "calloc") void* _pureCalloc(size_t nmemb, size_t size);
+    pragma(mangle, "realloc") void* _pureRealloc(void* ptr, size_t size);
+}


please add one unittest to cover all, or at best short ddoc unittests showing the use of each inside a pure function

andralex · 2017-01-19T07:48:30Z

OK, this is good progress. Thanks!

andralex · 2017-01-19T07:49:47Z

One idea for documentation: simply group all three together with documentation such as "Pure wrappers for the C stdlib memory allocation primitives malloc, calloc, and realloc." Then you use one ddoc unittest featuring all. That way you get nice compact documentation and one unifying example.

andralex · 2017-01-19T07:50:18Z

I suspect pureCalloc is safe, too. Awesomeness.

JackStouffer · 2017-01-19T11:49:21Z

src/core/memory.d

    }
 }
+
+/// Pure variant of `malloc` that doesn't affect global variable `errno`.


Please replace this with the following:

/** * Pure variants of C's memory allocation functions. Purity is achieved via * resetting the `errno` to it's value prior to being called, removing the * function's global state mutation. * * See_Also: * $(LINK2 https://dlang.org/spec/function.html#pure-functions, D's rules for purity), * which allow for memory allocation under specific circumstances. */

IMO it addresses the intuitiveness of a pure memory allocation function for the reader.

The teleological description is better than the description in terms of implementation details. I.e. if a platform offered access to an errno-less malloc (as e.g. would be easy to hack with jemalloc), pureMalloc would use that.

andralex · 2017-01-24T14:31:41Z

src/core/memory.d

 {
    const errno = fakePureGetErrno();
    void* ret = fakePureMalloc(size);
    cast(void)fakePureSetErrno(errno);


I'd say it's more efficient to guard the call like this:

if (!ret || errno != ERR_OK) cast(void)fakePureSetErrno(errno);

The common case is malloc succeeds and the previous state was ERR_OK so no need to reset errno.

ERR_OK is not defined. I guess you mean 0, right?

I can't find any good docs on {set,get}Errno. Shouldn't we just read and write the errno directly instead of calling {set,get}Errno? Provided that errno is thread-local.

Otherwise, LGTM.

nordlow · 2017-01-28T10:24:24Z

Ready for squashing?

andralex · 2017-01-28T13:48:09Z

lgtm

nordlow · 2017-01-28T13:54:20Z

Squashed.

nordlow · 2017-01-28T13:55:43Z

BTW: Is manual squashing still needed by PR authors?

I heard somewhere that Github had added an automatic squash option upon merge.

andralex · 2017-01-28T15:48:00Z

BTW: Is manual squashing still needed by PR authors?

I forgot. cc @MartinNowak @CyberShadow

andralex · 2017-01-28T15:49:19Z

One more thing - please add a doc unittest featuring the use of all three. Thanks!

wilzbach · 2017-01-28T15:55:35Z

BTW: Is manual squashing still needed by PR authors?
I forgot. cc @MartinNowak @CyberShadow

GitHub squashing is already enabled and the dlang bot supports squashed merges. @CyberShadow had some concerns related to digger and PR number inference, but IIRC they have been resolved as the PR number is part of the commit message?

andralex · 2017-01-29T20:22:21Z

src/core/memory.d

+    {
+        void* p = pureMalloc(n);
+        enforce(n == 0 || p !is null);
+


Vertical space in a five-liner is a net negative (ironically your implementations themselves don't have useless vertical space, and indeed they shouldn't). There is no value added by it.

andralex · 2017-01-29T20:22:54Z

src/core/memory.d

+        void* p = pureMalloc(n);
+        enforce(n == 0 || p !is null);
+
+        scope(failure) free(p);


Use p = pureRealloc(p, 0); to free memory in a pure manner.

andralex · 2017-01-29T20:23:08Z

src/core/memory.d

+    import std.exception : enforce;
+    import core.stdc.stdlib : free;
+
+    ubyte[] fun(size_t n) // TODO pure?


one way or another the example must be pure!

andralex · 2017-01-29T20:23:26Z

src/core/memory.d

+
+        p = pureRealloc(p, n *= 2);
+        enforce(n == 0 || p !is null);
+


space, the final frontier - let's not

andralex · 2017-01-29T20:55:22Z

src/core/memory.d

+{
+    import core.stdc.stdlib : free;
+
+    ubyte[] fun(size_t n) // TODO pure?


nordlow · 2017-01-29T20:55:49Z

Needed to use assert instead. enforce is in Phobos :)

andralex · 2017-01-29T20:55:52Z

src/core/memory.d

+    {
+        void* p = pureMalloc(n);
+        p !is null || n == 0 || assert(0);
+        scope(failure) { p = pureRealloc(p, 0); }


braces are redundant - think of an inlined if, would one put braces?

nordlow · 2017-01-29T21:00:00Z

Ok now, @andralex ?

andralex · 2017-01-29T21:03:32Z

thx!

nordlow · 2017-01-29T21:03:56Z

Great!

nordlow · 2017-01-30T06:23:08Z

Why is pureFree disallowed when pureRealloc can be used to free aswell?

andralex · 2017-01-30T13:46:23Z

Ask @schveiguy :)

nordlow · 2017-01-30T14:13:48Z

@schveiguy ?

schveiguy · 2017-01-30T14:31:12Z

It's a good question. I would question it the other way, why is pureRealloc allowed when pureFree is not.

Technically, realloc(ptr, 0) is essentially the same as free(ptr). However, one is not likely to realloc without wanting to re-assign to the original pointer. That is, if one has a function as has been discussed like:

void foo(immutable int *ptr) pure { ... }

Using realloc in such a function doesn't make a whole lot of sense, since you potentially destroy the original pointer, and have no way to inform the caller of the new location.

So while technically realloc can be used as a crude instrument to achieve pure free, the likelihood that someone does this is quite low. I would say you are misusing realloc if you are doing it that way.

IMO, realloc is ok to mark pure.

rainers · 2017-01-30T22:49:54Z

There are no auto-tester builds for druntime anymore?
This PR broke master because the Digital Mars malloc is able to allocate size_t.max bytes! (In fact it just adds 2 to the size before calling the Windows API functions.)

CyberShadow · 2017-01-30T22:52:38Z

Wow, the autotester did not report any status for the last commit.

I'm going to enable the autotester being green as a requirement for merging Druntime PRs.

nordlow · 2017-01-30T22:53:47Z

Oops, sorry about that. Should we just remove this test case, @andralex?

CyberShadow · 2017-01-30T22:53:59Z

Done. @rainers Can you code up a fix or should we revert?

nordlow · 2017-01-30T22:56:09Z

I can remove the part where size_t.max bytes is allocated if you feel like.

rainers · 2017-01-30T23:00:45Z

Subtracting 2 works, too: #1754

nordlow · 2017-01-30T23:09:26Z

Thanks.

schveiguy · 2017-01-31T13:32:14Z

Wow, the autotester did not report any status for the last commit.

The merge was done without auto-tester requirement (i.e. merge button was pushed, not auto-merge).

I'm going to enable the autotester being green as a requirement for merging Druntime PRs.

This is probably a good requirement, but I'd much rather the auto-merge feature be used, since there is usually a lag between some other merge happening and github deciding that it's no longer green.

CyberShadow · 2017-01-31T14:25:04Z

I see... so the problem is that @andralex did not use auto-merge.

This is probably a good requirement, but I'd much rather the auto-merge feature be used, since there is usually a lag between some other merge happening and github deciding that it's no longer green.

We might be able to fix this by enabling the GitHub-side requirement that PR branches can only be merged if they are rebased off the latest commit of the target branch, and have the autotester do that rebase on top of the target branch commit it tested with before attempting to merge the PR. Not sure it would be worth the effort, though - have we ever had breakages due to this?

andralex · 2017-01-31T16:09:22Z

I saw "All tests passed" so I merged directly. It looks like a race condition.

schveiguy · 2017-01-31T16:13:01Z

It looks like a race condition.

Yes, this is the lag I spoke of. github can take some time to sync with the status of the auto tester, or the auto tester could possibly not report the "invalid" status quickly enough. In any case, using the auto-tester auto-merge feature is the safest path -- if all tests have passed, it will merge immediately, so it's no different.

nordlow mentioned this pull request Jan 17, 2017

Revert "Qualify C-style memory allocations functions as pure" #1735

Merged

PetarKirov suggested changes Jan 18, 2017

View reviewed changes

nordlow force-pushed the pure-c-allocs branch from 9ca974d to c410c29 Compare January 18, 2017 15:31

nordlow changed the title ~~Add pureMalloc~~ Add pure wrappers for C style allocation Jan 18, 2017

nordlow force-pushed the pure-c-allocs branch from 81c7849 to 0ee3f5b Compare January 18, 2017 22:47

nordlow force-pushed the pure-c-allocs branch 2 times, most recently from f20b859 to 3500531 Compare January 18, 2017 22:52

andralex approved these changes Jan 19, 2017

View reviewed changes

JackStouffer reviewed Jan 19, 2017

View reviewed changes

andralex approved these changes Jan 24, 2017

View reviewed changes

PetarKirov approved these changes Jan 28, 2017

View reviewed changes

nordlow force-pushed the pure-c-allocs branch from b588fbd to c1c578c Compare January 28, 2017 13:54

nordlow force-pushed the pure-c-allocs branch from 1b4d9ba to 9c778d4 Compare January 29, 2017 17:47

andralex reviewed Jan 29, 2017

View reviewed changes

nordlow force-pushed the pure-c-allocs branch 2 times, most recently from a07e188 to 67918c3 Compare January 29, 2017 20:54

andralex reviewed Jan 29, 2017

View reviewed changes

Add pureMalloc, pureCalloc and pureRealloc

78bead0

nordlow force-pushed the pure-c-allocs branch from 67918c3 to 78bead0 Compare January 29, 2017 20:56

andralex merged commit 6146508 into dlang:master Jan 29, 2017

wilzbach mentioned this pull request Jul 5, 2017

Add pureFree to core.memory #1836

Merged

nordlow deleted the pure-c-allocs branch October 2, 2018 06:59

Uh oh!

Comments

Conversation

nordlow commented Jan 17, 2017 • 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!

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

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Jan 18, 2017

Uh oh!

nordlow commented Jan 18, 2017

Uh oh!

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

Uh oh!

Uh oh!

JackStouffer commented Jan 18, 2017

Uh oh!

nordlow commented Jan 18, 2017

Uh oh!

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

Uh oh!

Uh oh!

JackStouffer commented Jan 18, 2017

Uh oh!

nordlow commented Jan 19, 2017

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!

andralex commented Jan 19, 2017

Uh oh!

andralex commented Jan 19, 2017

Uh oh!

andralex commented Jan 19, 2017

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 Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow commented Jan 28, 2017

Uh oh!

andralex commented Jan 28, 2017

Uh oh!

nordlow commented Jan 28, 2017

Uh oh!

nordlow commented Jan 17, 2017 •

edited

Loading

nordlow Jan 18, 2017 •

edited

Loading

nordlow Jan 18, 2017 •

edited

Loading

nordlow commented Jan 18, 2017 •

edited

Loading

kinke commented Jan 18, 2017 •

edited

Loading

nordlow Jan 28, 2017 •

edited

Loading

nordlow commented Jan 28, 2017 •

edited

Loading