xserver: Cleaning up memory allocation functions and macros
efw at eamonwalsh.com
Sat Apr 28 19:58:36 PDT 2007
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 ), 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.
>>> * 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
More information about the xorg