Bug 19164 – malloc may be considered pure when failure results in program exit (no need to reset errno)

A common pattern is to call `core.memory.pureMalloc` and either `assert(false)` or `onOutOfMemoryError()` if allocation fails. In those places instead using `malloc` directly (privately annotated as `pure`) would avoid a small amount of work: the `core.memory.pureMalloc` wrapper reads errno, calls malloc, then resets errno, which is unnecessary if allocation failure results in the program exiting at once.

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/dcca23edcac64fc8505645dcfb71dcd0e7d5c77e Fix Issue 19164 - malloc may be considered pure when failure results in program exit (no need to reset errno) https://github.com/dlang/phobos/commit/4f3db522d66a018caec69812ed9e19f89dad2679 Merge pull request #6660 from n8sh/issue-19164 Fix Issue 19164 - malloc may be considered pure when failure results in program exit (no need to reset errno)

From http://man7.org/linux/man-pages/man3/malloc.3p.html : Upon successful completion with size not equal to 0, malloc() shall return a pointer to the allocated space. If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned. Otherwise, it shall return a null pointer and set errno to indicate the error. I.e. just checking for 0 result is insufficient to make the failure call.

The pattern is correct when it is known the argument to malloc cannot be zero. For instance, in std.container.array.Array.length(size_t newLength), allocation is only attempted if newLength is greater than the current length.

But functions in that fix do not assert that. Nor is 0 an invalid argument to malloc.

The functions are package-visible only, and they're replacing places where previously malloc was being called and the program was aborting if the result was null.

If it was only to improve already existing code, it should've been done there locally for that code. But these are made for the whole package. For longevity, changing semantics of malloc isn't a good design decision. If anyone in five years time adds something else to Phobos that uses those functions, how are they to know they shouldn't pass 0 to them? Essentially, as implemented, enforceMalloc may terminate the program on a valid input. It's not that passing 0 to malloc is a useful thing to do. But since it allows it, a wrapper shouldn't fail with it either.

The name "enforceMalloc" was chosen to be similar to "enforce" which throws an exception if its argument is false. The idea being "enforceMalloc" is like a shorthand way of writing: "auto ptr = malloc(sz); enforce!OutOfMemoryError(ptr);" >If anyone in five years time adds something else to Phobos that uses >those functions, how are they to know they shouldn't pass 0 to them? They can know by opening std.internal.memory and looking at what `enforceMalloc` does. If they don't know then they should not use it!

>If it was only to improve already existing code, it should've been >done there locally for that code. BTW, the first version of the PR was like that, but Sebastian Wilzbach & Petar Kirov objected convincingly on the grounds that it caused needless code duplication.

It's like we're talking about different things... `malloc` returning null is *insufficient* to warrant program termination, no matter how you want to call the wrapper. As for that: > They can know by opening std.internal.memory and looking at what `enforceMalloc` does. If they don't know then they should not use it! No. Just hard no. From your own documentation: ``` Pure variants of C's memory allocation functions `malloc`, `calloc`, and `realloc` that achieve purity by aborting the program on failure so they never visibly change errno. ``` null result is not a failure when malloc's argument was 0. So either the documentation is wrong, or the implementation is wrong.

You are right, the documentation is incorrect and should be fixed.

>It's like we're talking about different things... `malloc` returning >null is *insufficient* to warrant program termination, no matter how >you want to call the wrapper. I think 99% of the problem is that I copied and pasted the documentation and gave the wrong idea. It's not a general-purpose replacement for malloc, it's supposed to be a purified macro for `enforce!OutOfMemoryError(malloc(sz))`.

Improvement PR: https://github.com/dlang/phobos/pull/6774