xserver: Cleaning up memory allocation functions and macros

[Egbert Eich]

Magnus Vigerlöf writes:
 > Hi all,
 > 
 > I've been checking the memory allocation functions (as requested) and would 
 > like to have some feedback on what I think should be changed.
 > 
 > Remember, this work will not affect the client side of things, only server and 
 > its drivers. I'll do my very best to retain the link-compability with the 
 > current source.
 > 
 > 
 > My proposal is the following:
 > 
 > * I haven't seen any place where INTERNAL_MALLOC is set/used (except in 
 > os/xalloc.c and os/utils.c) so let's remove every trace of that one. This 
 > also means that os/xalloc.c will be removed.

The internal malloc was introduced when Linux was using glibc and it's
own libc malloc sucked. Today with the glibc malloc we don't have these
problems any more.
It contained a rudimentary memory debugger to catch double free's for 
example which I was using from time to time as it was easy and straight
forward to use.
I think it's probably OK to remove it but while you are at it: a  some 
document on best practices for memory debugging would be useful.


 > 
 > * 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.

It's probably not used - but would it be useful to check for such cases
you describe?
Also it's used to call CheckMemory() which is a function from Keithp's
memory debugger that used to live in extras/ in the mono tree.
I assume this is gone already so you can just as well remove the 
function from xf86Events.c/xf86Init.c which I once added to run the 
leak checker by sending a signal (SIGUSR2) to the server to get the
memory leakage state at any time at runtime.
Again a document on resources for memory debugging using the system
tools would be useful.

 > 
 > * 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...].)

Are you suggesting to call the system provided malloc() from the code
directly?
I'm not sure if this is a great idea: A lot of bigger projects create 
their own allocation calls mostly probably for portability reasons:
as was the case with early Linux sytems one may want to port the code
to a platform whose internal malloc is not really optimal for our purposes.
With own wrappers or just macros it's easy to remedy this situation
without uglu kludges.
X.Org has always also been the sample implementation of the X Window
System. Therefore the goal has been to make it portable enough that
it can be ported to whatever OS there is (*).
Have we abandoned this goal? - To ensure the maximum portability 
either macros or wrappers seem to be advisable.

 > 
 > * I'll go through the individual directories in xserver on-by-one and fix the 
 > code according to the 'standard' below once we agree on how to go forward.
 > 
 > 
 > So, what memory allocation functions should be used from now on?
 > 
 > * 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.

Maybe not on all implementations? If we use a wrapper or macro this
can be handled there. The default wrapper doesn't need to address this.

 > 
 > * xstrdup() will be defined as a macro which can handle being passed NULL. 
 > strdup doesn't.
 > 
 > * 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.

Hm, is this needed? 

 > 
 > * 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).
 > 

We don't want to use it while the server is running.
But they may be handy in situations where a failure is unlikely (so we 
don't want to be bothered with special handling) but if it happens need 
to be handled with a proper error message.
Driver initialization is a situation that comes to my mind here.

---------------
(*) The notion of sample implementation doesn't mean that we have or 
want to support all these OSes. The idea is that anybody can pick up
the code and port it to his operating system. This notion has helped
to proliferate the X Window System.
And no, I probably don't want to see support of all these OSes clutter
up our tree. 

Cheers,
	Egbert.