[MPlayer-cvslog] r33217 - trunk/gui/util/bitmap.c

[Reimar Döffinger]

On Tue, Apr 05, 2011 at 09:53:56PM +0200, Ingo Brückl wrote:
> Reimar Döffinger wrote on Tue, 5 Apr 2011 18:37:51 +0200:
> 
> >> -    unsigned char ext[][6] = { ".png\0", ".PNG\0" };
> >> +    unsigned char *ext[] = { "png", "PNG" };
> 
> > These are not even remotely equivalent.
> 
> The new one isn't equivalent, but adequate.

Long text, so only read if you are interested in some "details".
In this case probably, but it is no improvement.
You change a two-dimensional array to an array of pointers.
This has several effects: first, the actual strings will end
up in .rodata.
This means that "ext[0][0] = 0;" will now crash.
Thus the type is wrong, it should be at least "const unsigned char ...".
Though there really is no point in having the "unsigned" there at all.
There is also the point that pointers cost: here at least additional
memory and more and slower code due to having to do two reads (first
the pointers, them the strings themselves) and some more operations
if MPlayer is compiled as PIE for extra security (not officially supported)
and pointers are also always an additional attach opportunity for exploits.
Now, as to the static: It mean that the data is stored in .data (non-constant)
or .rodata (constant) instead of on the stack.
For initialized data, on the stack means extra code to do the initialization
(and each time the function is called).
Note that compilers will skip copying to the stack if you make it const.
Also note that for the "unsigned char *ext[]" that is the const as in
"unsigned char * const ext[]", the first const is just to use the correct
type so you get an error when you do the kind of assignment that would cause
a crash, it does not affect what the compiler does otherwise.
That const is still a good idea even if you already used "static" to avoid the
copy to the stack.
First, because a static variable that is not const has very similar issues to
global variables and thus it is a good idea to make sure nobody will modify it.
But it also allows the compiler to move it from .data to .rodata.
The advantage of .rodata is that the operating system can do nice tricks:
- It can share the same data between all MPlayer instances, so even if you ran
  500 MPlayers at the same time it would be in memory only once.
  To a degree that would be possible with .data as well, but generally it
  doesn't really work well.
- If it runs out of memory, it can just discard it without writing to swap
  space. When necessary it can just reload it from the binary on disk.
  For some embedded system it can even use it directly from ROM/flash without
  making a copy in RAM at all (relatively new in-place execution feature in Linux).