xserver: Cleaning up memory allocation functions and macros

[Eamon Walsh]

Magnus Vigerlöf wrote:
> On lördag 28 april 2007, Daniel Stone wrote:
> [...]
>>> * Neither have I seen MEMBUG being defined anywhere. Will anyone miss
>>> this feature if I remove it? There are code that ignore the case where
>>> the memory allocation functions return NULL (the option functions
>>> (addNewOption2 [1]), I don't know how to make it better without breaking
>>> the interface though), so my gut feeling is that it's not used very
>>> much/at all today.
>> Not really.  I guess the best way of testing this would be to get a libc
>> modification in which did the same.
> 
> I've used the memory allocation tracing features in glibc when tracking down 
> the memory leaks in the hotplug path, maybe the hook-concept included in 
> glibc can be used for this purpose when something like this is needed? Not 
> usable for those system based on something else than glibc, I know..

IMHO, it really should be the job of libc to provide these features.

Also, what about ALLOCATE_LOCAL and DEALLOCATE_LOCAL?  Are there any 
target systems out there that still don't have a working alloca?


> 
>>> * Is there a reason for the existence of both xmalloc (macro) and Xmalloc
>>> (wrapper function)? (apply this question on all memory functions) I'll go
>>> for the libc-variants wherever I can, but those functions that depends on
>>> a slightly different semantic than the libc ones, which name standard
>>> should be used? (I prefer the lower case prefix [xstrdup, xnf...].)
>> Probably to do with ABI concerns from 1993.
> 
> Ok. So stick with the current and phase all these calls out as we go then. 
> Good.
> 
>>> * Use standard libc-functions
>>>   - For every call to alloc/calloc/realloc/... there *must* be a check if
>>> it succeded to allocate the memory.
>>>   - free() can take NULL as argument, so it's not needed to test for NULL
>>> before calling it.
>> Huzzah!  Thankyou.
> 
> :)
> 
>>> * xstrdup() will be defined as a macro which can handle being passed
>>> NULL. strdup doesn't.
>> Sounds sensible, but you could also flag a warning if --enable-debug is
>> enabled, and we could fix all these cases and gradually transition to
>> using normal strdup()?
> 
> Hmmm... If this is as easy as enabling a #warning if DEBUG is set, I think I 
> need to make this a wrapper function instead. Otoh, that would make it a 
> candidate to mark it with _X_DEPRECATED. I think I'll go for that instead if 
> there's no objection.
> 

+1 on all of the above.


>>> * xfreeZ() will be defined as a macro that set the pointer to NULL after
>>> freeing the memory with free(). Use *only* if the pointer must be set to
>>> NULL for some reason.
>> Is there any reason to do this?  It just encourages the x*() wrappers
>> and makes the code less obvious to the casual observer.
> 
> The code in some places will look nicer. However, I'm not sure I want this 
> either (I just had to toss the idea up and see if anyone would shoot it 
> down). I'll remove this one. Thanks.

I agree that this macro isn't necessary.


> 
>>> * xnf* will be implemented as wrapper functions that will call FatalError
>>> if the libc-functions returned NULL when they shouldn't (i.e. more or
>>> less kept as-is). NOTE: Avoid these, use proper error-propagation instead
>>> (calling these should be considered a BUG).
>> Indeed.  You can just mark these as _X_DEPRECATED and the compiler will
>> complain every time.
> 
> And it's not that many really (200-400 places, thought it would be more..). 
> But the tricky part will be to refactor the code so it'll handle the 
> situation instead without breaking the interface. I'll change the call to one 
> of these if proper checking is not made (and I can't fix it), so there will 
> be some more, but we'll know where they are at least.

Calls which would break the interface could be changed to FatalError 
explicitly as a temporary measure, with an XXX or TODO to fix the 
interface later.


--Eamon W.