elektraMalloc et al.

[kodebach]

Continuation of elektraMalloc et al. discussion from #3731.

The question is: Should elektraMalloc et al. be public API? Should we even keep them at all?

---

The following is a response to #3731 (comment)

I agree that they provide little benefit. But removing them also provides little benefit and a lot of effort.

Removing the functions that have the same API as their libc counterpart should just be a regex-replace.

These are probably bugs, as our API design says that for every (public) function allocating something there needs to be a function freeing it.

Technically the function allocating is elektraMalloc/elektraCalloc and the function freeing is elektraFree. So no bug here ;)

More seriously, there aren't any functions in libelektra-core or libelektra-kdb AFAIK that return something the caller has to free with elektraFree. But there are some functions in e.g. libelektra-ease that just return a newly allocated string. Do you really want to add something like elektraFreeString, which then just wraps elektraFree?

I don't know of any if you want to change the allocator only for Elektra. There are many libraries providing similar mechanisms, e.g. libltdl or glyphy (IIRC, project seems to be gone now) or even implement their own allocator, e.g. harfbuzz

libltdl's approach is totally different. They use lt_dlmalloc, but that is not a function. It is global function pointer that by default is set to malloc. So calling code can just set the pointer to a different value, which is much simpler than actually replacing a function like elektraFree. However, calling malloc through a function pointer will (probably) cause a small performance hit.

harfbuzz implements it's own allocator. That can make sense, if there is specific knowledge that can be used to improve performance. But that means the allocator would be specific to Elektra. Then you could just recompile Elektra and use compiler/linker options to replace malloc et al.

Personally, I don't think keeping elektraMalloc and elektraCalloc (also for other reasons see #3686) is a good idea. A big problem with keeping elektraMalloc is that it is makes it impossible to call some libc functions that allocate data with malloc (e.g. strndup) or others that expect a pointer that can be passed to realloc. You might even do that by accident and never notice the problem, until someone replaces elektraMalloc and then you could get some pretty bad memory bugs.

---

IMO the only safe way to keep elektraMalloc et al. is to define that they will always call their libc counterpart (malloc et al.) and their purpose is simply to add assertions. Then we can also make them private, because you can just call free to free any pointer returned by Elektra. Ideally, I'd remove the functions, but it seems unlikely this will be accepted.

It would also be much easier for newcomers, if the could just use standard malloc/free and other libc functions.

[markus2330]

But there are some functions in e.g. libelektra-ease that just return a newly allocated string. Do you really want to add something like elektraFreeString, which then just wraps elektraFree?

Do we even need these elektraBooleanToString functions to be public? If we decide yes, yes it would be the correct way to add a elektraFreeString. Do you see a big problem in having such a function?

MO the only safe way to keep elektraMalloc et al. is to define that they will always call their libc counterpart (malloc et al.) and their purpose is simply to add assertions. Then we can also make them private, because you can just call free to free any pointer returned by Elektra.

It is possible that your application is linked to another version of libc than Elektra. Then calling free (with your version of libc) would be unsafe.

It would also be much easier for newcomers, if the could just use standard malloc/free and other libc functions.

Only if we also get rid of elektraRealloc.

[kodebach]

Do we even need these elektraBooleanToString functions to be public?

They are used in the codegen part of the high-level API, so yes these functions have to be public.

If we decide yes, yes it would be the correct way to add a elektraFreeString. Do you see a big problem in having such a function?

Okay, so why not just make elektraFree public then? It's just a different name... The implementation of elektraFreeString would literally be:

void elektraFreeString (const char * string)
{
    return elektraFree (string);
}

It is possible that your application is linked to another version of libc than Elektra.

How would that happen? Wouldn't you have to statically link libc into Elektra while compiling Elektra and then link you app against a different libc or not use a statically link libc for the app? That seems like a very advanced (borderline broken) setup. If somebody tries that, I think they'll be able to find a way to make it work.

I'm pretty sure our CMake setup doesn't even have an option to link libc statically into Elektra.

Only if we also get rid of elektraRealloc.

Not if it is clearly defined that elektraRealloc always calls realloc and is just an alternative (less easy to misuse) API. If you want to use realloc go ahead, but if you like elektraRealloc better than use that.

[markus2330]

Okay, so why not just make elektraFree public then?

core vs. libease. In libease its much easier to argue for more (public) functions.

The implementation of elektraFreeString would literally be:

This is called wrapper and nothing is wrong with it.

How would that happen? Wouldn't you have to statically link libc into Elektra while compiling Elektra and then link you app against a different libc or not use a statically link libc for the app? That seems like a very advanced (borderline broken) setup. If somebody tries that, I think they'll be able to find a way to make it work.

Proprietary apps are sometimes delivered together with a libc (either static or a startup script makes sure their own libs are found). If they would use Elektra, and the person wants the app to use the global Elektra it is imho a valid use case.

Of course it is definitively not "important to have" nor a 1.0 goal. But why break something that is already working.

[kodebach]

core vs. libease. In libease its much easier to argue for more (public) functions.

This only works, if libease creates public wrappers for all the allocation/free functions. Otherwise if another library wants to return a pointer that it got from elektraMalloc, it would need to provide its own wrapper for elektraFree. And then end up with lots of really unnecessary wrappers.

This is called wrapper and nothing is wrong with it.

A wrapper is fine, if it fulfils some purpose (e.g. the keyDup wrapper for keyCopy). But this literally just passes it's arguments through to another function. There is also no chance that there will ever be a need to change the implementation of elektraFreeString. It will always just call elektraFree.

Proprietary apps are sometimes delivered together with a libc (either static or a startup script makes sure their own libs are found). If they would use Elektra, and the person wants the app to use the global Elektra it is imho a valid use case.

See, what I don't really get is: You sometimes come up with these insanely exotic use cases for Elektra to make a point about why we shouldn't change X. But other times you argue that we should have less CMake options, because Elektra is too complex.

If somebody really has such a specific use case that they need their app to use a special libc, but don't want their libraries to use this libc (mixing libc's is a very bad idea, but whatever), then they'll probably be able to find a way to make it work no matter want we do. In all likelihood such a special app uses a forked version of Elektra anyway.

---

My main goal with removing elektraMalloc et al. is to improve developer friendliness, especially for newcomers. It is still annoying that I using e.g. strndup may be problematic, because it doesn't use elektraMalloc for allocations. But when I was new to Elektra (and I suspect many other felt the same), it was downright frustrating that you have to remember to use Elektra's special sauce instead of the standard stuff.

IMHO the benefits of having elektraMalloc et al. just don't outweigh the costs. If it is unacceptable for you to remove them, then we'll just keep things as is. But adding utterly useless wrappers like elektraFreeString, just so that elektraFree is not public API is the last thing I want to do.

[markus2330]

This only works, if libease creates public wrappers for all the allocation/free functions.

Ok by me.

A wrapper is fine, if it fulfils some purpose

Making a private function of one library public is a purpose.

You sometimes come up with these insanely exotic use cases for Elektra to make a point about why we shouldn't change X.

I didn't suddenly come up with this use case, it was one of the original reason why these functions exist. Except of elektraRealloc which was introduced by Avi to improve realloc.

That is why it is very important to document the decisions. Otherwise insane reasons dictate a lot of the code base (especially if nobody is there which remembers the decisions). I personally think that these allocators are over-the-top but I do not think them insane. Other projects do much more for memory management, e.g. sqlite actually allows malloc to fail without making sqlite crash. And yes, they have a sqliteMalloc for memory debugging, something we could also introduce easily as long as we have something like elektraMalloc.

My main goal with removing elektraMalloc et al. is to improve developer friendliness, especially for newcomers.

If this is really the case that it annoys so much, I agree to remove them. Imho it is a very minor thing. I think we lost by far more time discussing them than what it takes to use them.

We probably need another decisions meeting.

[kodebach]

I didn't suddenly come up with this use case, it was one of the original reason why these functions exist.

What I still don't understand is how exactly elektraMalloc is supposed to improve anything. Yes, you have a central point through which all of Elektra's allocations go. But to change anything you still need to change the code and recompile. At that point you could just as well use #define malloc mymalloc.

Other projects do much more for memory management, e.g. sqlite actually allows malloc to fail without making sqlite crash.

Yes, other projects like SQLite do much more and at that point having a wrapper for malloc would be acceptable to me. My problem is really that we pretend there is some important logic in elektraMalloc, when in fact it is only a simple wrapper. Basically, take on this burden of using our own wrapper, because of the small possibility that someday we might do something about memory management. All that, when in fact "doing something about memory management" would be such a huge task, that replacing all calls to libc functions that allocate memory would be a minor inconvenience in comparison.

Imho it is a very minor thing.

Yes, it is a minor thing. But since you brought up the idea of removing elektraMalloc et al. from the public API, it was obviously the correct moment to discuss them in general.

We probably need another decisions meeting.

I think meeting could be helpful. Not necessarily for this or the other changes from the API reviews, but for the changes to the backend and global plugin stuff. It would probably be much quicker to talk through everything than to write long proposals and reviews.

[stale]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

[stale]

I closed this issue now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖