Created attachment 15350 [details] [review] 0001-Compile-warning-fixes.patch o Make sure all functions have a prototype defined in a header file, before definition, and all callers include this header. Only Atombios/hwserv_drv.c:AllocateMemory() did have a prototype not matching definition, and was fixed changing first argument from VOID * to DEVICE_DATA *. o Added an if !defined(__GNUC__) around the usage of pragma warning. o Changed type RADEONInfoRec's FB field from void * to unsigned char *, and VBIOS field from CARD8 * to unsigned char *. This fixes warnings about usage of void * in arithmetic. Also, compiled the driver in both cases, with pciaccess support enabled and disabled, as gcc will also not allow math involving void * and unsigned char *, and fixed the few cases where it did happen. o Added missing prototypes for atombios related functions (src/atombios_output.c and src/atombios_crtc.c) to radeon.h, so that no prototypes are declared in C files now, only in headers. o Changed some atombios functions to static as they are used only inside the defining file, or their pointers are set to structure fields. o _X_EXPORT'ed src/theatre200.c/DumpRageTheatreRegsByName() as it was missed in previous patch and would cause problems if compiled with hidden symbols. Actually, this code should be reviewed, as some of these symbols are also called from the X Server. Probably they should their pointers should be members of some data structure. This would be mandatory if theatre200_drv.so and theatre_drv.so would be both loaded. This is also a reasonable large patch, but it fixes all compiler warnings. Please review. The compiler options used were: -Wbad-function-cast -Wdeclaration-after-statement -Wmissing-prototypes -Wmissing-declarations -Wnested-externs -fno-strict-aliasing -Wold-style-definition -Wpointer-arith -Wstrict-prototypes
Be careful, IIRC, some of the void * were to support the libpciaccess vs. non-libpciaccess cases.
(In reply to comment #1) > Be careful, IIRC, some of the void * were to support the libpciaccess vs. > non-libpciaccess cases. I guessed it could cause problems :-), and tested compilation with libpciaccess enabled and disabled, and fixed 2 cases, by casting to unsigned char *, where it would not compile due to mixing void * and unsigned char * in pointer arithmetic. Both cases, when libpciaccess was disabled. There should not exist any other problems as math with void * is the same as math with signed or unsiged char pointers.
(In reply to comment #2) > There should not exist any other problems as math with void * is the > same as math with signed or unsiged char pointers. So I thought, but it turned out that void* arithmetics are a GCC extension and not defined in any C standard. It's probably better not to use it.
(In reply to comment #3) > > There should not exist any other problems as math with void * is the > > same as math with signed or unsiged char pointers. > > So I thought, but it turned out that void* arithmetics are a GCC extension and > not defined in any C standard. It's probably better not to use it. Either I am confused, or you misunderstood me :-) The patch changed the field from "void*" to "unsigned char*" to avoid warnings about void* math. The cases I casted to unsigned char* were because gcc doesn't compile void* and char* arithmetic: - if (info->tilingEnabled && ((pPix->devPrivate.ptr - info->FB) == 0)) + if (info->tilingEnabled && (((unsigned char *)pPix->devPrivate.ptr - info->FB) == 0)) this could just be "pPix->devPrivate.ptr == info->FB" and: - dst_offset = (pPixmap->devPrivate.ptr - info->FB) + + dst_offset = ((unsigned char *)pPixmap->devPrivate.ptr - info->FB) + otherwise, if keeping RADEONInfoRec.FB type as void*, it would be required a quite larger patch, casting it to a char pointer, to avoid the warnings. The comment about the math being the same is because ((void*)<addr> + integer) should result the same pointer as ((unsigned char*)<addr> + integer).
(In reply to comment #4) > The comment about the math being the same is because > ((void*)<addr> + integer) should result the same pointer as > ((unsigned char*)<addr> + integer). But the point of my comment is that ((void*)<addr> + integer) is only valid with GCC (and probably other compilers trying to be compatible with it), not according to any C standard. I haven't checked if this affects the patch, just wanted to point it out because I used to be unaware of it as well.
This patch should not apply anymore, but I will keep it opened because there are some issues that should really be addressed: o AtomBios should be a shared module for radeon and radeonhd drivers. This patch corrected all compiler warning fixes and also made most of AtomBios static functions, so it could be help in starting point for such shared module. o It should be possible to load variants of the theathre*_drv.so modules, but what happens is that they have symbols with the same name, and due to a lot of "LoaderSymbol" magic, it is required to make most of them visible (the corrections for the "LoaderSymbol" magic are also avaiable in my patches to allow compiling the XServer and drivers with -fvisibility=hidden). I think there may existed some misunderstanding that prevented this patch from being applied, when I said that math of void* and char* should give the same pointer. But the fact is that the patch changes the driver to use char* math, and so, that change should not cause unwanted side effects.
Mass closure: This bug has been untouched for more than six years, and is not obviously still valid. Please reopen this bug or file a new report if you continue to experience issues with current releases.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.