[PATCH] headers: Fix build errors with latest glibc

[Daniel Stone]

Hi,

On 14 July 2014 13:51, Hans de Goede <hdegoede at redhat.com> wrote:

> On 07/14/2014 02:43 PM, Julien Cristau wrote:
> > On Mon, Jul 14, 2014 at 14:33:00 +0200, Hans de Goede wrote:
> >> Can I / we please get a reply from you on this ?
> >> As explained below this is not about glamor.h, but about
> >> making os.h safe to include which really is an
> >> orthogonal problem, The glamor.h usage of os.h was
> >> just a (bad) example.
> >
> > The drivers need to include xorg-server.h before any other server
> > header.  AFAICT that's true both before and after your patch.
>

Yes, this is basically my position. Including anything from <xorg/*.h>
without having either xorg-server.h (for drivers) or xorg-config.h
(internal) included first results in undefined behaviour. Specifically on
64-bit systems, you lose out horrendously because CARD32 secretly gets
promoted via unsigned long to CARD64, as _XSERVER64 is not defined. The
lack of all the functional definitions in xorg-server.h also means that
your ABI's probably different even without the type differences.

It's a terrible, terrible, idea that results in really hard-to-track-down
bug reports. To be fair, I'm not on the end of these bug reports anymore,
but it's not fun. Anything we can do to discourage this would be better.


> Well in practice they are not doing that, and they are getting
> away with it (iow everything works just fine), except that
> os.h starts re-defining system libc functions.
>



> I really don't want to go and fix every single driver out there.
>

To be honest, I really don't think it would be every single driver.


> There may be some headers where drivers really *must* include
> xorg-server.h first, but os.h is not one of them. So far we've
> been getting away with os.h redefining e.g. strndup, but with
> the latest glibc things have started to conflict.
>
> I know X is really really old, so we have a bunch of legacy,
> like headers which do not work stand-alone (which is considered
> a big no no in modern C development practices). But just because
> we have this legacy we should no strive to do better.
>
> So there are 2 reasons to make these changes to os.h:
>
> 1) It avoids the need to fix every single driver out there
> 2) It is simply the right thing to do
>

os.h isn't safe to necessarily safe to include without xorg-server.h, as it
drags in misc.h, which has: extern _X_EXPORT void SwapLongs(CARD32 *list,
unsigned long count);

Again, CARD32 is unsigned long without xorg-server.h, which makes it 64-bit
on those systems; including xorg-server.h defines _XSERVER64, which makes
CARD32 actually be uint32_t. Not massively harmful, but still not really
great. A lot of the authorization functions also use XID types, which have
the same variance on 64-bit systems without xorg-server.h.

It might not be strictly harmful in some cases, but it's not the sort of
thing I want to encourage. 'Always include xorg-server.h before any X
header ever' is a really simple rule to remember for driver developers;
muddying the water, particularly when not documented, isn't amazingly
helpful IMHO.

Cheers,
Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140714/d61a82c7/attachment.html>