fix for custom 'realloc' with a context #775

[ConradWood]

implements code and tests as discussed here #775
also patches example 'simple' to compile unconditionally (.PHONY) (because it seems useful for an example to reuse latest and it compiles in less than a second anyways) and with malloc support.

[PetteriAimonen]

The implementation looks mostly good.
I have a few notes:

1. The comments indicate that realloc() would be used for releasing, but I don't see the corresponding modification to pb_release_single_field(). It also looks like the test case does not check for releasing through the custom allocator, it could count the number of new allocations (ptr == NULL) and number of releases (size == 0).
2. I don't see the point of changing examples/simple Makefile. That example is supposed to be basic and simple, and doesn't use dynamic allocation in any case.
3. I would like the PB_ENABLE_MALLOC to remain the main and only switch for dynamic allocation. I.e. if it is not specified, PB_ENABLE_MALLOC_CONTEXT should add the extra field in pb_istream_t, but not enable dynamic allocation. I think in the next major release when breaking changes are made, I might make PB_ENABLE_MALLOC_CONTEXT default on, but PB_ENABLE_MALLOC will remain default off.
4. The alltypes style test cases are a bit of a maintenance burden. I'll probably merge custom_realloc to alltypes_pointer test case at some point. But I guess it is reasonable to have it separate for now when PB_ENABLE_MALLOC_CONTEXT remains a default-off feature.

[ConradWood]

On Mon, 2022-05-09 at 10:47 -0700, Petteri Aimonen wrote: The implementation looks mostly good. I have a few notes:    1. The comments indicate that realloc() would be used for releasing, but I don't see the corresponding modification to pb_release_single_field(). It also looks like the test case does not check for releasing through the custom allocator, it could count the number of new allocations (ptr == NULL) and number of releases (size == 0).

Yes, this is where it gets interesting. As we discussed here[1] we discussed using a separate context, but eventually settled on using istream instead. Whilst implementing this is, I realised that istream is not currently passed to pb_release(). I also realised, that my usecase is not limited to one proto, I am thinking along this: struct context *ctx = pb_createcontext() pb_decode(ctx,stream_a,fields_a,dest_a); pb_decode(ctx,stream_b,fields_b,dest_b); ctx->close() where memory allocations for both proto parsers come from one memory heap. close releases both. pb_release(ctx) is optional in this case. It seems less than ideal to use the stream for these cases. However, there are a number of ways I could do this with the stream: 1. I could add an api pb_release_context(pb_istream_t *stream,const pb_msgdesc_t *fields, void *dest_struct) 2. I could modify "struct pb_msgdesc_s". Specifically I could add a field which could hold a copy of the pointer to realloc from the stream(). (I do not particularly like this option, for various reasons I am happy to elaborate if needed) My conclusion though is, that none of the options are great. I think the original concept of the context being separate from the stream is logically more in-line with what I (and perhaps others) would expect. wdyt? [1] #775

   2. I don't see the point of changing examples/simple Makefile. That example is supposed to be basic and simple, and doesn't use dynamic allocation in any case.

ok

   3. I would like the PB_ENABLE_MALLOC to remain the main and only switch for dynamic allocation. I.e. if it is not specified, PB_ENABLE_MALLOC_CONTEXT should add the extra field in pb_istream_t, but not enable dynamic allocation. I think in the next major release when breaking changes are made, I might make PB_ENABLE_MALLOC_CONTEXT default on, but PB_ENABLE_MALLOC will remain default off.

ok

   4. The alltypes style test cases are a bit of a maintenance burden. I'll probably merge custom_realloc to alltypes_pointer test case at some point. But I guess it is reasonable to have it separate for now when PB_ENABLE_MALLOC_CONTEXT remains a default-off feature.

ok

[PetteriAimonen]

I agree, in your use case where allocator releases all allocations made in context, pb_release() is optional.

For general use, having stream passed to pb_release() makes most sense to me.
I think it could be made an optional parameter, and if the stream is NULL then pb_release() would internally do:

pb_istream_t default = {NULL, NULL, 0, &pb_free_wrapper};
if (!stream) stream = &default;

To provide a reasonable upgrade path to get clean API for 0.5 and also backward compatibility for 0.4 series, I suggest the following on header side:

#ifdef PB_ENABLE_MALLOC_CONTEXT
/* stream is optionally used for error messages and to set realloc function, if NULL then pb_free() is used */
void pb_release(pb_istream_t *stream, const pb_msgdesc_t *fields, void *dest_struct);
#else
void pb_release(const pb_msgdesc_t *fields, void *dest_struct);
#endif

[Et7f3]

meta data might be keeped so it won't completely free ?

[PetteriAimonen]

All definitions of realloc I've seen fully free the object. See e.g. https://pubs.opengroup.org/onlinepubs/009695299/functions/realloc.html "If size is 0 and ptr is not a null pointer, the object pointed to is freed."

[PetteriAimonen]

Looks like C17/C23 deprecate the zero size.