[Pixman] [PATCH] Fix pixman build with older GCC releases

[Brad Smith]

On Sat, Sep 28, 2013 at 09:54:10PM +0300, Siarhei Siamashka wrote:
> On Sat, 28 Sep 2013 00:01:27 -0400
> Brad Smith <brad at comstyle.com> wrote:
> 
> > On 16/09/13 4:25 PM, S??ren Sandmann wrote:
> > > Brad Smith <brad at comstyle.com> writes:
> > >
> > >> Discussion seems to have died down. Could this please be commited?
> > >
> > > I didn't see any reply to Siarhei's suggestion of doing it as a
> > > configure check. Also, you'll need to send a patch formatted with "git
> > > format-patch", with a useful conmmit log.
> > 
> > I looked around at the few compilers that implement this GCC-ism other 
> > than GCC and that's only LLVM and PCC
> 
> For example, you forgot about the Intel compiler. Not that it is
> causing any problems here, but this just shows that your knowledge
> about the use of the existing compilers all over the world is a
> little bit incomplete.

Yes, I did not come across any indication of the Intel compiler
supporting this built in function when searching around for any
details regarding this function, compilers supporting it, examples
of how other projects deal with this and so forth.

> And guess what? The other people (me included) also don't have a
> complete understanding about all the possible system configurations
> where pixman is used. That's why you had been politely asked to
> provide a bit more details about why exactly you are interested
> in GCC 3.3 support. It's simple curiosity because your original
> commit message did not try to justify the practical usefulness
> of the patch in a way that is clear enough for everyone. The reply
> http://lists.freedesktop.org/archives/pixman/2013-August/002842.html
> helped really a lot, and IMHO this should be a part of the commit
> message.

Well for me the most important thing is ensuring pixman builds not
to ensure it is utilizing the most efficient code and/or function with
every possible compiler out there.

> > and they were already covered by the code as it is at the moment.
> 
> That's good. And such extra details (the exact GCC version these
> compilers pretend to emulate) would be also a nice addition to
> the commit message.

Ok, I can do that.

> > I don't see any reason to further complicate the diff.
> 
> Are you interested in fixing the problem? Or in having your
> patch applied exactly in the way you see reasonable?

Well my diff is fixing what I consider is the immediate problem
and that is that pixman is not building, not that it isn't using
the most efficient resulting code with all compilers.

We obviously have different perspectives as to what the "problem"
is.

> I would like to get this resolved, just because I'm the one who
> introduced this regression (by relying on the __GNUC__ ifdef and
> incorrectly assuming that __builtin_clz has been always supported
> by GCC). Sorry for this.
> 
> But your solution does not sit well with me because it adds
> even more of the fragile ifdeffery magic. What if somebody comes
> tomorrow, complaining about yet another problem with this ill-fated
> ifdef and suggesting to also add some check, based on something like
> "defined(__FOOBAR_COMPILER__)" to it?

It's not that I was completely ruling out going down the autoconf
test way of fixing this. I just wasn't aware of any other compiler
that implemented this functionality that was not GCC or a compiler
essentially pretending to be GCC. I went looking around and almost
all other projects I could find using this function uses the same
method of detecting and using this function.

Ok before I post an official patch so to speak I'd like some review
of what I have come up with...


diff --git a/configure.ac b/configure.ac
index 263c63e..a269f21 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1036,6 +1036,22 @@ fi
 
 AC_MSG_RESULT($support_for_float128)
 
+dnl =====================================
+dnl __builtin_clz
+
+support_for_builtin_clz=no
+
+AC_MSG_CHECKING(for __builtin_clz)
+AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
+unsigned int x = 11; int main (void) { __builtin_clz(x); }
+]])], support_for_builtin_clz=yes)
+
+if test x$support_for_builtin_clz = xyes; then
+   AC_DEFINE([HAVE_BUILTIN_CLZ], [], [Whether the compiler supports __builtin_clz])
+fi
+
+AC_MSG_RESULT($support_for_builtin_clz)
+
 dnl ==================
 dnl libpng
 
diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
index 89b9682..4032c13 100644
--- a/pixman/pixman-matrix.c
+++ b/pixman/pixman-matrix.c
@@ -37,7 +37,7 @@
 static force_inline int
 count_leading_zeros (uint32_t x)
 {
-#ifdef __GNUC__
+#ifdef HAVE_BUILTIN_CLZ
     return __builtin_clz (x);
 #else
     int n = 0;

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.