Patchwork util.h: fix gcc version checking

login
register
mail settings
Submitter Siddharth Agarwal
Date Sept. 19, 2013, 4:57 p.m.
Message ID <a14b158920b990f6e656.1379609868@dev1091.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2538/
State Accepted
Headers show

Comments

Siddharth Agarwal - Sept. 19, 2013, 4:57 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1379609100 25200
#      Thu Sep 19 09:45:00 2013 -0700
# Node ID a14b158920b990f6e65632767b59ed7cccc80858
# Parent  1b592791351d126d8af39f70bbc63a707a11a75f
util.h: fix gcc version checking

gcc doesn't have a predefined GCC_VERSION macro.
Matt Mackall - Sept. 19, 2013, 6:05 p.m.
On Thu, 2013-09-19 at 09:57 -0700, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1379609100 25200
> #      Thu Sep 19 09:45:00 2013 -0700
> # Node ID a14b158920b990f6e65632767b59ed7cccc80858
> # Parent  1b592791351d126d8af39f70bbc63a707a11a75f
> util.h: fix gcc version checking
> 
> gcc doesn't have a predefined GCC_VERSION macro.

Queued for default, thanks.
Pierre-Yves David - Sept. 30, 2013, 1:44 p.m.
On 09/19/2013 06:57 PM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal<sid0@fb.com>
> # Date 1379609100 25200
> #      Thu Sep 19 09:45:00 2013 -0700
> # Node ID a14b158920b990f6e65632767b59ed7cccc80858
> # Parent  1b592791351d126d8af39f70bbc63a707a11a75f
> util.h: fix gcc version checking
>
> gcc doesn't have a predefined GCC_VERSION macro.
>
> diff --git a/mercurial/util.h b/mercurial/util.h
> --- a/mercurial/util.h
> +++ b/mercurial/util.h
> @@ -156,7 +156,7 @@
>   {
>   	return _byteswap_ulong(*(uint32_t *)c);
>   }
> -#elif GCC_VERSION>= 403
> +#elif __GNUC__>  4 || (__GNUC__ == 4&&  __GNUC_MINOR__>= 3)
>   static inline uint32_t getbe32(const char *c)
>   {
>   	return __builtin_bswap32(*(uint32_t *)c);
> @@ -179,7 +179,7 @@
>   	x = _byteswap_ulong(x);
>   	*(uint32_t *)c = x;
>   }
> -#elif GCC_VERSION>= 403
> +#elif __GNUC__>  4 || (__GNUC__ == 4&&  __GNUC_MINOR__>= 3)
>   static inline void putbe32(uint32_t x, char *c)
>   {
>   	x = __builtin_bswap32(x);
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

This changeset turn my Fedora test machine crazy. Any test result in 
corrupted changeset and running hg incoming ends in a segmentation fault.

It's a:
Fedora 18 ppc64
Power7 CPU
gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8)
Linux gcc1-power7.osuosl.org 3.7.2-204.fc18.ppc64 #1 SMP Fri Jan 18 
10:44:51 MST 2013 ppc64 ppc64 ppc64 GNU/Linux
Siddharth Agarwal - Sept. 30, 2013, 4:23 p.m.
On 09/30/2013 06:44 AM, Pierre-Yves David wrote:
> On 09/19/2013 06:57 PM, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal<sid0@fb.com>
>> # Date 1379609100 25200
>> #      Thu Sep 19 09:45:00 2013 -0700
>> # Node ID a14b158920b990f6e65632767b59ed7cccc80858
>> # Parent  1b592791351d126d8af39f70bbc63a707a11a75f
>> util.h: fix gcc version checking
>>
>> gcc doesn't have a predefined GCC_VERSION macro.
>>
>> diff --git a/mercurial/util.h b/mercurial/util.h
>> --- a/mercurial/util.h
>> +++ b/mercurial/util.h
>> @@ -156,7 +156,7 @@
>>   {
>>       return _byteswap_ulong(*(uint32_t *)c);
>>   }
>> -#elif GCC_VERSION>= 403
>> +#elif __GNUC__>  4 || (__GNUC__ == 4&& __GNUC_MINOR__>= 3)
>>   static inline uint32_t getbe32(const char *c)
>>   {
>>       return __builtin_bswap32(*(uint32_t *)c);
>> @@ -179,7 +179,7 @@
>>       x = _byteswap_ulong(x);
>>       *(uint32_t *)c = x;
>>   }
>> -#elif GCC_VERSION>= 403
>> +#elif __GNUC__>  4 || (__GNUC__ == 4&& __GNUC_MINOR__>= 3)
>>   static inline void putbe32(uint32_t x, char *c)
>>   {
>>       x = __builtin_bswap32(x);
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>
> This changeset turn my Fedora test machine crazy. Any test result in 
> corrupted changeset and running hg incoming ends in a segmentation fault.

Oh, doh, __builtin_bswap32 will do the wrong thing on big endian systems.

Two options:
(1) Add more ifdefs for big-endian vs little.
(2) Back this out.

Given that there's no significant performance difference, I'm in favour 
of (2).
Augie Fackler - Sept. 30, 2013, 7:24 p.m.
On Mon, Sep 30, 2013 at 09:23:07AM -0700, Siddharth Agarwal wrote:
> On 09/30/2013 06:44 AM, Pierre-Yves David wrote:
> >On 09/19/2013 06:57 PM, Siddharth Agarwal wrote:
> >># HG changeset patch
> >># User Siddharth Agarwal<sid0@fb.com>
> >># Date 1379609100 25200
> >>#      Thu Sep 19 09:45:00 2013 -0700
> >># Node ID a14b158920b990f6e65632767b59ed7cccc80858
> >># Parent  1b592791351d126d8af39f70bbc63a707a11a75f
> >>util.h: fix gcc version checking
> >>
> >>gcc doesn't have a predefined GCC_VERSION macro.
> >>
> >>diff --git a/mercurial/util.h b/mercurial/util.h
> >>--- a/mercurial/util.h
> >>+++ b/mercurial/util.h
> >>@@ -156,7 +156,7 @@
> >>  {
> >>      return _byteswap_ulong(*(uint32_t *)c);
> >>  }
> >>-#elif GCC_VERSION>= 403
> >>+#elif __GNUC__>  4 || (__GNUC__ == 4&& __GNUC_MINOR__>= 3)
> >>  static inline uint32_t getbe32(const char *c)
> >>  {
> >>      return __builtin_bswap32(*(uint32_t *)c);
> >>@@ -179,7 +179,7 @@
> >>      x = _byteswap_ulong(x);
> >>      *(uint32_t *)c = x;
> >>  }
> >>-#elif GCC_VERSION>= 403
> >>+#elif __GNUC__>  4 || (__GNUC__ == 4&& __GNUC_MINOR__>= 3)
> >>  static inline void putbe32(uint32_t x, char *c)
> >>  {
> >>      x = __builtin_bswap32(x);
> >>_______________________________________________
> >>Mercurial-devel mailing list
> >>Mercurial-devel@selenic.com
> >>http://selenic.com/mailman/listinfo/mercurial-devel
> >
> >This changeset turn my Fedora test machine crazy. Any test result in
> >corrupted changeset and running hg incoming ends in a segmentation fault.
>
> Oh, doh, __builtin_bswap32 will do the wrong thing on big endian systems.
>
> Two options:
> (1) Add more ifdefs for big-endian vs little.
> (2) Back this out.
>
> Given that there's no significant performance difference, I'm in favour of
> (2).

Can one of you two mail the backout with the explanation of why it's wrong?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/util.h b/mercurial/util.h
--- a/mercurial/util.h
+++ b/mercurial/util.h
@@ -156,7 +156,7 @@ 
 {
 	return _byteswap_ulong(*(uint32_t *)c);
 }
-#elif GCC_VERSION >= 403
+#elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
 static inline uint32_t getbe32(const char *c)
 {
 	return __builtin_bswap32(*(uint32_t *)c);
@@ -179,7 +179,7 @@ 
 	x = _byteswap_ulong(x);
 	*(uint32_t *)c = x;
 }
-#elif GCC_VERSION >= 403
+#elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
 static inline void putbe32(uint32_t x, char *c)
 {
 	x = __builtin_bswap32(x);