Hi, On 07/14/2014 03:41 PM, Daniel Stone wrote: > 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.
Ok, fair enough. I hope someone will get around to fixup drivers where necessary before the next Fedora mass-rebuild though. If not I guess I will get to see how bad the driver situation wrt this really is. Regards, Hans _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
