Hi Daniel, 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.
Regards, Hans On 07/03/2014 10:50 PM, Hans de Goede wrote: > Hi Daniel, > > On 07/03/2014 12:07 AM, Daniel Stone wrote: >> Hi, >> >> On 2 July 2014 20:14, Hans de Goede <[email protected]> wrote: >> >>> include/os.h defines protyptes for various non ansi-c str functions, such >>> as str[n]casecmp and strndup. The definition of the prototypes is guarded >>> by #ifndef HAVE_STRFOO, but HAVE_STRFOO is defined by xorg-server.h which >>> is not included by all users of os.h. E.g. glamor.h does not and should not >>> include xorg-server.h >>> >>> This is a problem since the these prototype declarations clash with the >>> libc >>> declarations when using the latest glibc: >>> >>> In file included from /usr/include/xorg/misc.h:115:0, >>> from /usr/include/xorg/screenint.h:50, >>> from /usr/include/xorg/scrnintstr.h:50, >>> from /usr/include/xorg/glamor.h:32, >>> from conftest.c:61: >>> /usr/include/xorg/os.h:579:2: error: expected identifier or '(' before >>> '__extension__' >>> strndup(const char *str, size_t n); >>> >>> To fix this, this commit moves the HAVE_STRFOO defines to their own >>> (generated) header called os-strfeatures.h, and unconditionally includes >>> this >>> header from os.h . >>> >> >> NAK. > > I don't understand your NAK, it seems to be purely based on glamor.h doing > (somewhat) unrelated stuff wrong. But this fix is not for glamor.h, it is for > os.h. The glamor.h compile error was just an example. > > Currently os.h cannot be included safely (without it creating duplicate > prototypes for system functions) without first including one of: > dix-config.h > xorg-server.h > xorg-config.h > > Which is IMHO quite bad for a _public_ header, esp since 2 of the 3 > headers above are not public, so the reasoning of my fix goes like > this: > > 1) This is bad, it is breaking driver builds left and right, after > first hitting this when building the intel driver and fixing it > at the driver level: > http://cgit.freedesktop.org/xorg/driver/xf86-video-intel/commit/?id=40bd45fb802366aa013b9089848b67a28bf3b1ee > I then hit the same issue a couple days later with the ati driver, > so I decided this needs a proper fix at the server level, rather > then fixing each and every driver out there. > > 2) Since inside the server we sometime want dix-config.h and sometimes > xorg-config.h, os.h cannot simply include xorg-server.h itself > > 3) Combining 1) and 2) has let me to the conclusion that the > relevant HAVE_STRFOO defines should be in there own header, > which can be safely included by os.h unconditionally and installed > as a public header. > >> If glamor.h includes scrnintstr.h, it must include xorg-server.h. >> xorg-server.h (as distinct from xorg-config.h) is the specially-generated >> header designed explicitly to be included by all exported/SDK modules which >> include internal headers, since it carries defines such as _XSERVER64 (and >> many, many more) which thoroughly smash the server ABI. >> >> If glamor.h can't have xorg-server.h, then it can't have any other server >> headers either. > > Right, so glamor.h needs some loving, no reason to nack a patch which does > not even touch it ... > > Regards, > > Hans > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
