[RFC] Add pure fail-fast assertMalloc, assertCalloc, assertRealloc #2276

n8sh · 2018-08-16T09:17:01Z

These functions achieve purity by aborting the program on failure so errno is never observed to change. This PR also changes pureMalloc etc. to templates so they work in betterC.

The idea was suggested by @ZombineDev at dlang/phobos#6660 (comment) and #2268 (comment). The sole difference is that instead of adding a boolean template parameter to pureMalloc I instead named the new variant differently. I felt that assertMalloc(100) has a more obvious meaning than pureMalloc!true(100) which gives no hint as to whether it's the one that aborts on failure or the one that saves and restores errno.

dlang-bot · 2018-08-16T09:17:02Z

Thanks for your pull request, @n8sh!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2276"

JinShil · 2018-08-16T09:22:39Z

src/core/memory.d

+    void* ret = fakePureMalloc(size);
+    if (!ret && size)
+    {
+        version (D_BetterC) assert(0, "Memory allocation failed");


@WalterBright has made the decision that we should be using assert(0) (preferably assert(false)) for both -betterC and non-betterC builds.

Also it would be better to use version (D_Exceptions) to check whether exceptions can be thrown as LDC/GDC allow to disable them selectively (and e.g. WebAssembly might not defined -betterC, but still not support exceptions.)

JinShil · 2018-08-16T09:46:35Z

src/core/memory.d

+
+@nogc nothrow pure @system unittest
+{
+    const int errno = fakePureErrno();


fakePureErrno is mangled to fakePureErrnoImpl which is not a template. Therefore this won't work until fakePureErrnoImpl is made into a template, or some other solution is devised.

Edit: I mean it won't work in -betterC until fakePureErrnoImpl is made into a template, or some other solution is devised

Should be fixed now.

JinShil · 2018-08-16T09:56:01Z

src/core/memory.d

+ * The functions may terminate the program using `onOutOfMemoryError` or
+ * `assert(0)`. These functions' purity guarantees no longer hold if
+ * the program continues execution after catching AssertError or
+ * OutOfMemoryError.


I think this comment needs updating.

I kept the mention of the possibility so if we later change to using onOutOfMemoryError no one will be blindsided. Do you think it's better to remove it because that's too unlikely?

No need to change anything immediately, IMO. We can wait to see how the review goes. It just eventually needs to reflect the final implementation.

JinShil · 2018-08-17T04:50:20Z

src/core/memory.d


+version (D_BetterC) @nogc nothrow private @system


Will this implementation work in non-betterC builds? Can't we use this implementation for both -betterC and non-betterC builds?

It would work. The purpose of the version branch is so outside code that assumes fakePureErroImpl exists and tries to link to it will not break.

PetarKirov · 2018-08-17T07:41:28Z

I would argue that OOM is not a logical error on the programmer side, rather it is an exception from the program environment, albeit a non-recoverable from one. IMO, we should continue calling onOutOfMemoryError as it is provides for a bit more systematic way of handling this type of exception, just make sure it has a fallback for version (D_Exceptions) { } { /* ... */ } .

P.S. Thanks a lot for working on this guys, I would prefer to do the work myself instead of just talking, but sadly I have very limited free time these days.

n8sh · 2018-08-17T10:35:43Z

@ZombineDev That makes sense.

Also change pureMalloc etc. to templates so they work in betterC.

JinShil · 2018-08-17T10:47:31Z

I would argue that OOM is not a logical error on the programmer side, rather it is an exception from the program environment, albeit a non-recoverable from one. IMO, we should continue calling onOutOfMemoryError as it is provides for a bit more systematic way of handling this type of exception

I'm not terribly partial to either one, but @WalterBright has spoken. We need to come to some consensus about this and apply it consistently.

Edit: I will add that it'd be kind of nice if both -betterC and non-betterC behaved the same whenever possible.

JinShil · 2018-08-17T10:47:50Z

Sorry! Wrong button.

n8sh · 2018-08-17T11:02:17Z

I'm not terribly partial to either one, but @WalterBright has spoken.

I took your summary to mean that he was okay with using assert but not that he was mandating it. Did I get the wrong idea?

@WalterBright has already affirmed to me (twice actually) in private e-mail exchange that he is OK replacing Errors with asserts even in non-betterC code.

JinShil · 2018-08-17T11:11:58Z

I took your summary to mean that he was okay with using assert but not that he was mandating it. Did I get the wrong idea?

@WalterBright will have to speak for himself, but my interpretation is that he preferred us to use asserts. It's very unlike him to mandate anything, from my experience.

JinShil

I'm OK with this, but the DDoc comments still need updating.

JinShil · 2018-08-19T07:43:44Z

We still need to come to some consensus about Errors vs asserts. Once that decision is made, this should be updated to reflect it.

WalterBright · 2018-08-24T23:06:39Z

src/core/memory.d

+    return ret;
+}
+/// ditto
+void* assertRealloc()(void* ptr, size_t size) @nogc nothrow pure @system


Since realloc() free's memory, it cannot ever be considered pure.

Since realloc() free's memory, it cannot ever be considered pure.

We have a pureFree, and GC.free is also marked as pure.

pure functions are allowed to modify their parameters. Freeing is never @safe, but in what way is it less pure than other forms of mutation?

There is also already a pureRealloc.

druntime/src/core/memory.d

Lines 840 to 847 in db2910e

/// ditto

void* pureRealloc(void* ptr, size_t size) @system pure @nogc nothrow

{

const errnosave = fakePureErrno();

void* ret = fakePureRealloc(ptr, size);

fakePureErrno() = errnosave;

return ret;

}

n8sh requested review from andralex and wilzbach as code owners August 16, 2018 09:17

n8sh force-pushed the failfast-malloc branch 2 times, most recently from 9ee729c to fc9c777 Compare August 16, 2018 09:19

JinShil suggested changes Aug 16, 2018

View reviewed changes

n8sh mentioned this pull request Aug 16, 2018

Fix Issue 19164 - malloc may be considered pure when failure results in program exit (no need to reset errno) dlang/phobos#6660

Merged

n8sh force-pushed the failfast-malloc branch from fc9c777 to 384d093 Compare August 16, 2018 09:40

JinShil reviewed Aug 16, 2018

View reviewed changes

JinShil suggested changes Aug 16, 2018

View reviewed changes

n8sh force-pushed the failfast-malloc branch 9 times, most recently from 45fa62e to fe1119f Compare August 16, 2018 12:47

JinShil reviewed Aug 17, 2018

View reviewed changes

n8sh force-pushed the failfast-malloc branch from fe1119f to 10230b0 Compare August 17, 2018 10:33

Add pure fail-fast assertMalloc, assertCalloc, assertRealloc

a279b3c

Also change pureMalloc etc. to templates so they work in betterC.

n8sh force-pushed the failfast-malloc branch from 10230b0 to a279b3c Compare August 17, 2018 10:37

JinShil closed this Aug 17, 2018

JinShil reopened this Aug 17, 2018

JinShil approved these changes Aug 19, 2018

View reviewed changes

WalterBright reviewed Aug 24, 2018

View reviewed changes

n8sh closed this Nov 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add pure fail-fast assertMalloc, assertCalloc, assertRealloc #2276

[RFC] Add pure fail-fast assertMalloc, assertCalloc, assertRealloc #2276

n8sh commented Aug 16, 2018

dlang-bot commented Aug 16, 2018

JinShil Aug 16, 2018

n8sh Aug 16, 2018

wilzbach Aug 16, 2018

JinShil Aug 16, 2018 •

edited

Loading

n8sh Aug 16, 2018

JinShil Aug 16, 2018

n8sh Aug 16, 2018

JinShil Aug 16, 2018

JinShil Aug 17, 2018

n8sh Aug 17, 2018

PetarKirov commented Aug 17, 2018 •

edited

Loading

n8sh commented Aug 17, 2018

JinShil commented Aug 17, 2018 •

edited

Loading

JinShil commented Aug 17, 2018

n8sh commented Aug 17, 2018

JinShil commented Aug 17, 2018 •

edited

Loading

JinShil left a comment

JinShil commented Aug 19, 2018

WalterBright Aug 24, 2018

aG0aep6G Aug 25, 2018

n8sh Aug 27, 2018

	/// ditto
	void* pureRealloc(void* ptr, size_t size) @system pure @nogc nothrow
	{
	const errnosave = fakePureErrno();
	void* ret = fakePureRealloc(ptr, size);
	fakePureErrno() = errnosave;
	return ret;
	}

[RFC] Add pure fail-fast assertMalloc, assertCalloc, assertRealloc #2276

[RFC] Add pure fail-fast assertMalloc, assertCalloc, assertRealloc #2276

Conversation

n8sh commented Aug 16, 2018

dlang-bot commented Aug 16, 2018

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov commented Aug 17, 2018 • edited Loading

n8sh commented Aug 17, 2018

JinShil commented Aug 17, 2018 • edited Loading

JinShil commented Aug 17, 2018

n8sh commented Aug 17, 2018

JinShil commented Aug 17, 2018 • edited Loading

JinShil left a comment

Choose a reason for hiding this comment

JinShil commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Aug 16, 2018 •

edited

Loading

PetarKirov commented Aug 17, 2018 •

edited

Loading

JinShil commented Aug 17, 2018 •

edited

Loading

JinShil commented Aug 17, 2018 •

edited

Loading