Bug#318218: Thoughts :-)

[David Nusinow <david_nusinow@verizon.net>]

On Fri, Jul 15, 2005 at 02:13:35AM -0400, Nathanael Nerode wrote:
> This can probably be fixed by changing the following stanza from
> xc/programs/Xserver/hw/xfree86/common/compiler.h:
> 
> # else /* !__alpha__ && !__powerpc__ && !__sparc__ */
> 
> #  define MMIO_IN8(base, offset) \
>         *(volatile CARD8 *)(((CARD8*)(base)) + (offset))
> #  define MMIO_IN16(base, offset) \
>         *(volatile CARD16 *)(void *)(((CARD8*)(base)) + (offset))
> #  define MMIO_IN32(base, offset) \
>         *(volatile CARD32 *)(void *)(((CARD8*)(base)) + (offset))
> #  define MMIO_OUT8(base, offset, val) \
>         *(volatile CARD8 *)(((CARD8*)(base)) + (offset)) = (val)
> #  define MMIO_OUT16(base, offset, val) \
>         *(volatile CARD16 *)(void *)(((CARD8*)(base)) + (offset)) = (val)
> #  define MMIO_OUT32(base, offset, val) \
>         *(volatile CARD32 *)(void *)(((CARD8*)(base)) + (offset)) = (val)
> 
> OK.  The problem with this is that the cast to volatile CARD8* (et al) doesn't 
> create a volatile object from the point of view of GCC4.  (GCC4 may be wrong 
> here.)  However, code like the following, if I'm not mistaken, *does* create 
> a volatile object, and GCC4 notices:
> 
> static inline unsigned char
> xorgReadMmio8(volatile void * base, const unsigned long offset)
> {
>   volatile CARD8 * tmp;
>   tmp = (CARD8*)base + offset;
>   return *tmp;
> }
> #define MMIO_IN8(base, offset) xorgReadMmio8(base, offset)
> 
> (Based on a little testing of simplified code, GCC is smart enough to optimize 
> away nearly everything in this, leaving nicely optimized assembly which still
> does the memory read, even when the resulting data is never accessed.)
> 
> So a consistent replacement along these lines should do the trick.
> Shall I whip up a patch?

To be honest, I'd rather not mess with this file unless we have to. The
simple fix in Redhat's bugzilla looks a lot less invasive, and it seems to
fix all the reported problems so far, so I'd rather go with that unless
upstream says otherwise.

I'm making a patch for us right now.

 - David Nusinow