Hi, On 14 July 2014 13:51, Hans de Goede <[email protected]> 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
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
