[PATCH 1/2] include: Add generic HeapAlloc() wrappers

[Michael Stefaniuc]

On 01/22/2018 08:23 PM, Michael Stefaniuc wrote:
> On 01/22/2018 04:05 PM, Jacek Caban wrote:
>> On 01/21/2018 10:05 PM, Michael Stefaniuc wrote:
>>> Signed-off-by: Michael Stefaniuc <mstefani at winehq.org>
>>> ---
>>> I know that Alexandre and Jacek wanted to work on the HeapAlloc
>>> wrappers. But I don't remember if I had promised (or not) at the last
>>> WineConf to dig up my old patch and polish it up and submit it to serve
>>> as a discussion start point. Anyway, here it is with some explanations.
>>
>> Thanks for looking at this. This looks mostly good for me. I have a few
>> suggestions to consider.
> thanks for reviewing the patch.
> 
> Before I reply to your suggestions here are two considerations that were
> at the base of my proposal:
> 
> Principle of the least surprise
> -------------------------------
> - Developers are already used to the current wrappers.
> - Linux developers are used to the malloc, realloc, calloc functions
> while Windows developers are used to HeapAlloc(). The function names
> will confer an expectation of what the wrappers do. If possible we
> should fulfill that expectation.
> Ease the transition
> -------------------
> - There should be no flag date for the move to the new wrappers.
> - Big churn patches should be avoided.
> - The full transition will take years, thus there will be new and old
> wrappers side by side. Those should have the same signature and the
> behavior should not differ wildly.
> - As I'll probably have to do a big part of this work you don't want me
> to run away screaming. And if I do, the code should be still in an
> acceptable consistent state.
> 
> 
> 
>>> The _nofail variants should not "fail" memory allocations and will raise an
>>> exception on out of memory conditions.
>>> heap_alloc_nofail thus will never return NULL.
>>> heap_realloc_nofail can still return NULL but *only* when the alloc size
>>> is 0 and thus functions as heap_free. Checking the returned pointer for
>>> NULL is not needed when the size != 0. And the "assign to temp pointer
>>> to prevent mem leak on failure" workaround is never needed.
>>
>> I think that most realloc use cases will use fallible variant (since its
>> usage usually means that the buffer may grow) and they still need
>> temporary pointer. How about using something like:
>> BOOL heap_realloc(void **mem, SIZE_T size)
>> instead? You could leave passed pointer alone on failure and set it to
>> new memory on success.
> Alexandre's requirement was a terse "sane behavior like realloc".
> 
> I see your proposal has some merit.
> But why stop there? We could just free the old pointer on allocation
> failure while keeping the existing function signature.
> I didn't check but I would assume that is the common case when the temp
Actually I do have a coccinelle script that I used to gather some
heap_realloc data. The script found:

realloc calls:
	heap_realloc: 122
	HeapReAlloc: 401
	heap_realloc_zero: 12
	d3dcompiler_realloc: 8
	msi_realloc_zero: 4
	msi_realloc: 20

realloc leaks: 279
realloc temp variable: 270
	return only: 15
	free old mem: 8

The "leaks" are
    ptr = heap_realloc(ptr, size)

The "temp variable" is code like
    temp = heap_realloc(ptr, size)

Of those calls using a temp variable the "return only" just do
    temp = heap_realloc(ptr, size)
    if (!temp)
	return ...;

The "free old mem" ones free the old memory as their first action as in
    temp = heap_realloc(ptr, size)
    if (!temp) {
	heap_free(ptr);
        ...
    }


So for more than half of the HeapReAlloc / heap_realloc calls an
"automatic free the old mem on failure" approach would work.

That is just a lower boundary as I didn't check all the permutations on
how to check for a failed heap_realloc. E.g. my script doesn't checks in
the "return only" or "free old mem" cases for code like
    if (!(temp = heap_realloc(ptr, size))


bye
	michael

> pointer trick is used. That can be than simplified to just without
> leaking any memory:
> if (!(ptr = heap_realloc(ptr, size))
>     return E_OUTOFMEMORY;
> 
> We can have another wrapper heap_reallocex() that implements your
> suggestion for the cases when a different cleanup is needed. Of course
> with a better name for the wrapper.
> 
> Anyway, both change variants are possible from a churn point of view:
> wine$ git grep heap_realloc | wc -l
> 196
> 
> 
>>> The _nofail variants should be used only for small allocations that are
>>> not application driven.
>>> Preferabely the allocation sizes should be known at compile time.
>>> Careful when allocating memory for COM and other objects: The main
>>> object should not be allocated with the _nofail variants. Subsequent
>>> small allocations in the same object could use the _nofail version.
>>
>> How about having just heap_alloc as infallible and heap_calloc as
>> fallible? If you need fallible heap_alloc, you may just pass extra 1
>> argument to heap_calloc. If you need infallible heap_calloc, then you
>> know there won't be an overflow and may just multiply arguments.
> You want to change
> 
> if (!(obj = heap_alloc(sizeof(*obj)))
>     return E_OUTOFMEMORY;
> 
> to
> 
> if (!(obj = heap_calloc(sizeof(*obj), 1))
>     return E_OUTOFMEMORY;
> 
> ?
> 
> Ugh, NACK.
> Quite a surprising change. Requires a flag date or big churn patches.
> The default heap_alloc() should keep its current behavior.
> That allows for a proper code review and transition to the infallible
> alloc behavior.
> 
> If somebody else has a better name for the "nofail" variants I'm all
> ears. The only other name I could came up that matched the intent was
> heap_alloc_I_reviewed_the_code_and_know_what_I_am_doing(); but that's
> even longer ;)
> 
> 
> bye
> 	michael
> 
>