Thank you! Looks generally good to me, but maybe it's better to split the CMake/pthread changes into a separate PR?

You welcome.

Pinging @vapier for review.

Maybe reallocf is not a good name.

It may make sense to use the same terms: gdSetMemoryCallocMethod vs. gdCallocf. Why not gdCallocMethod? Or gdSetMemoryCallocFunc and gdCallocFunc.

Running the test with CMake works just fine but not with Autotools 🤔 .

Very good PR , thank you! :)

It could be a tat bit better using:

common struct to store the f

• function ptr data in a global struct, surrounded by our lock mechanism, Each getter/setter using the lock/unlock
• must be only called once per process (not sure we can ensure it but then doc issue, big mess if it changes accross the same process)

What do you think?

From what was discussed in the issue we can go either ways with the static struct or the vars, but I'll stick with the latter for the moment.
Speaking about locking I find it kinda pointless, the methods are obviously not thread safe and will cause disastrous alterations with the memory managers if handled incorrectly.
Maybe doing checks on another global variable is an option. Or maybe we could not check anything and document that calling twice is invalid usage (kinda like the vkBeginCommandBuffer docs).

I think it would make sense to have a struct, it will also helps CPU cache, especially in loaders where quite a few alloc/free may happen. Or in GD3 with the Paths APIs.

Taste thing, I find it cleaner as well :)

What do you think?

Besides that, I am all for it and sounds like a good addition for 2.4.0 :) Well done!!

Only thing about this PR is that testing gdmem with Autotools and shared libgd leads to the functions not being used in the test. Although it works with an static archive. From there idek what's happening.

I think it would make sense to have a struct, it will also helps CPU cache, especially in loaders where quite a few alloc/free may happen. Or in GD3 with the Paths APIs.

I don't have a strong opinion on this, but sounds reasonable.

Most important for me is that we ship custom allocator support ASAP. I wouldn't want to unbundle libgd from php-src before this isn't available.

Only thing about this PR is that testing gdmem with Autotools and shared libgd leads to the functions not being used in the test. Although it works with an static archive.

Thanks for reporting. I think this needs closer investigation.

Anyhow, if you have some time, could you please resolve the merge conflicts?

No further comments @pierrejoye, @cmb69, @vapier?

@YakoYakoYokuYoku in principle I am good with it. I was waiting @remicollet feedback on the API.

While I still like to provide the atomicity handling on our side tho'. That can be done post merge.

I was waiting @remicollet feedback on the API.

I don't really care about the API ;) (as soon as it is there)

BTW, a single setter may be enough (I don't think changing only some make sense, and a single call with 4 pointers will avoid forgetting one).

Perhaps a safety bell checking those allocators have not be used yet before allowing the switch...

@cmb69, @pierre, @remicollet, @vapier I'm deeply sorry for being late with this PR, was occupied with things IRL and I didn't know how to fix this PR up until now.

So if anyone of you can merge this I'd be glad for it. 🙏

@vapier, since you've approved this can you take another look?

Thank you @YakoYakoYokuYoku for all the work you've put into this! However, a related issue came up recently regarding PHP and libgmp, see php/php-src#16507 (comment). So unless the functions would be stored in TLS (thread-local storage), PHP probably can't use the feature. Note that I'm not talking about true thread-safety (that might not be necessary), but rather that each thread should have its own custom allocators.

Another issue is, that is should somehow forbidden to change/clear the allocators after any memory already been allocated. It might be sufficient to track whether any allocation has happend, and in this case let the calls to change/clear the allocators fail or silently do nothing.

In theory adding thread local storage specifiers should suffice though tell me if I'm wrong. On the other hand, the allocators guard could be implemented using atomic booleans and use gd_error to print messages, but I think that a warning in the documentation is enough, unless there's something elso to take into account.

In theory adding thread local storage specifiers should suffice though tell me if I'm wrong.

I think that is sufficient, if supported on all supported platforms. A bit hard to verify, since most of CI is failing anyway.

On the other hand, the allocators guard could be implemented using atomic booleans and use gd_error to print messages, but I think that a warning in the documentation is enough, unless there's something elso to take into account.

Thinking about that (see also php/php-src#16609 (review)), there actually should be locks when setting/retrieving the custom allocators, since you never know when a thread switch happens. If we only support a single change of the custom allocators (i.e. set them all at once), that lock could be completely implemented in libgd; if we support individual setters, we likely need to provide some API which needs to be called by the client who wants to change the custom allocators.

And to avoid havoc even when using a single-threaded process, we need to make it very clear in the documentation, that clients need to restore the old allocators when done with their own. And even that would not be sufficient, if libgd allows to install callbacks.

It is most unfortunate, that a simple and highly desireable feature may cause so many issues.