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

[JackStouffer]

This reverts commit 65d9a07.

See #1718 (comment)

[schveiguy]

This is going to break code. Not sure how we need to proceed on this. Can one have an overload of a function based on being able to call in pure code?

[schveiguy]

BTW, I'm not sure malloc needs to be impure. It's generally accepted that memory allocation is an impure operation that can be considered "logically" pure. The errno argument is valid, but that again seems like a reach as errno is only as good as the last call that set it. Due to malloc generating mutable data, it can't be elided per the rules of "weak" purity, so there isn't a danger of malloc returning the same data for identical calls.

[JackStouffer]

I can stomach the memory allocation argument for the GC being pure because logically you know that programs that deal with no memory available and no swap correctly only come out of NASA. Plus you know that GC allocated memory's visibility to the programmer is basically null.

However, it's much less sound argument when there is a global variable being written to that's accessible from anywhere in the program. A pure function having such an obvious side effect without a pointer to it is not something that can be defended outside of some convenience. The pure attribute is really just a lie in this case.

And really, what do we have to gain by marking malloc as pure? There are no performance benefits, and the safety benefit goes out the window. All it stands to do is confuse the definition of pure. I said it before and I'll say it again: if we keep going down this road with pure, you will have serious DIPs from people asking for a strongpure attribute to get what they had before.

[schveiguy]

errno is generally thread-local, so there isn't a danger of cross-thread issues. And almost always, errno is not a global variable that simply can be accessed whenever you want, as it's meaning is worthless if you didn't just call a function that sets it. In reality, it should have been a second parameter to all those system calls, but in the name of convenience, it was just made a global variable so you didn't have all these calls preceeded by:

int possibleError; /* oh, and this needs to go at the start of the function in C */
void *mem = malloc(size, &possibleError);

There are no performance benefits

Fully disagree. malloc and free are much higher performance than GC.malloc and GC.free.

I can live with malloc being impure, as it is always possible to work around this via local prototypes. The bigger question is, how do we transition away from pure malloc, since there is undoubtedly code that expects it to be pure since it's release in October last year?

[schveiguy]

Note that calloc which you want to make impure, is called by GC.malloc (potentially), which then could set errno.

https://github.com/dlang/druntime/blob/master/src/gc/impl/conservative/gc.d#L1849

[JackStouffer]

Fully disagree. malloc and free are much higher performance than GC.malloc and GC.free.

That's not what I meant. I mean that adding pure doesn't increase the performance of malloc.

[schveiguy]

Right, but adding pure to ANY weak-pure function doesn't increase the performance. What it does is allow that function to be called from a strong-pure function.

Considering any strong-pure function that needs to temporarily allocate memory (like for instance, schwartzSort), switching from GC.malloc to C malloc is going to be an option for increasing that performance. Of course, you need to free it, which I still think is bad to mark as pure for the reasons I've stated elsewhere. But with proper usage of malloc and free (or alternatively some mechanism to ensure the memory doesn't escape), you can safely use them inside the strong-pure function.

[Geod24]

This is going to break code

AFAICS 65d9a07 is only part of v2.073.0-b1, so not part of any release, so this is fine.

And almost always, errno is not a global variable that simply can be accessed whenever you want, as it's meaning is worthless if you didn't just call a function that sets it.

Well it can be accessed whenever you want. It just doesn't make sense.
Also, what about:

.errno = 0;
auto x = malloc(512);
assert(.errno == 0);

A slightly more clever compiler could decide that the assert check will always pass because errno is TLS and malloc is pure.

[schveiguy]

AFAICS 65d9a07 is only part of v2.073.0-b1, so not part of any release, so this is fine.

You are right, I have my dates messed up. This was merged just after the release of 2.072. So this is OK to revert IMO.

It may break code, but only if that code is depending on building from master. ping @MartinNowak I think we should revert this before releasing 2.073, to give some more time for discussion.

[schveiguy]

@Geod24 what about this?

void *m(size_t bytes) pure { return (new void[bytes).ptr; }

.errno = 0;
auto x = m(512); // can set errno, as I alluded to earlier
assert(.errno == 0);

Note that errno is not the definitive return value, the return value of the function is. errno just says why the function failed. The semantics of it make this a trivial error (maybe the wrong thing is printed as an error message), as the actual error already occurred and should have been detected.

Besides, sane code will not do as you say, as it doesn't make sense to pre-fill errno.

In any case, I think we probably should merge this, and then we can examine the ramifications before the 2.074 release.

[schveiguy]

Note, if we want to merge this before 2.073 is released, this needs to go into stable branch.

[mathias-lang-sociomantic]

Besides, sane code will not do as you say, as it doesn't make sense to pre-fill errno.

Sane code must do it, actually.
Some functions return values are in-band, meaning there is no special value to indicate an error. In this case you must set errno to 0 before calling the function, and then check it if a pre-defined value is defined (e.g. LONG_MIN). Look for In-Band Error Indicatore here.

EDIT: Sorry, different account, but same person.

[schveiguy]

Not for malloc though. null is out-of-band.

Edit: it can return null on success if size is 0, but that is well-defined. That is, the error condition can be written as: result == null && size != 0

[andralex]

OK. Next step, please define a function pureMalloc that sets errno back to where it was for the current thread.

[andralex]

Does this also set errno?

[andralex]

(apparently not: http://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.basetrf1/malloc.htm)

[andralex]

pureCalloc

[andralex]

pureRealloc

[andralex]

probably these don't need to be wrapped

[schveiguy]

Again, please target stable or else the pure tags will get into 2.073!

[andralex]

cc @MartinNowak

[JackStouffer]

Uh, I changed the base branch in git using git rebase --onto stable master pure but the base branch in github didn't change

[JackStouffer]

Never mind, figured it out

[JackStouffer]

Question for people who know the runtime code better than me: where should these wrapper functions be defined?

[andralex]

import/core/memory.d

[andralex]

Auto-merge toggled on

[andralex]

@MartinNowak merge into stable as well please?

[JackStouffer]

@andralex This is stable :)

[schveiguy]

Awesome, thanks everyone. Let's work on getting RefCounted pure and see how that goes.

[nordlow]

@JackStouffer who adds pureMalloc, pureCalloc and pureRealloc? And where should we put them?

I added pureMalloc here #1746