[PATCH] Add extra logic required to detect endianness on Solaris

[PATCH] Add extra logic required to detect endianness on Solaris

[Charles Bailey]

Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>
---

The endian detection added in 7e3dae494 isn't sufficient for the Solaris
Studio compilers. This adds some fallback logic which works for Solaris
but would also work for AIX and Linux if it were needed.

 compat/bswap.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/compat/bswap.h b/compat/bswap.h
index 120c6c1..5a41311 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -110,6 +110,27 @@ static inline uint64_t git_bswap64(uint64_t x)
 #endif
 
 #if !defined(__BYTE_ORDER)
+/* Known to be needed on Solaris but designed to potentially more portable */
+
+#if !defined(__BIG_ENDIAN)
+#define __BIG_ENDIAN 4321
+#endif
+
+#if !defined(__LITTLE_ENDIAN)
+#define __LITTLE_ENDIAN 1234
+#endif
+
+#if defined(_BIG_ENDIAN)
+#define __BYTE_ORDER __BIG_ENDIAN
+#endif
+
+#if defined(_LITTLE_ENDIAN)
+#define __BYTE_ORDER __LITTLE_ENDIAN
+#endif
+
+#endif /* !defined(__BYTE_ORDER) */
+
+#if !defined(__BYTE_ORDER)
 # error "Cannot determine endianness"
 #endif
 
-- 
1.9.0

Re: [PATCH] Add extra logic required to detect endianness on Solaris

[Junio C Hamano]

Charles Bailey <cbailey32@bloomberg.net> writes:

>  #if !defined(__BYTE_ORDER)
> +/* Known to be needed on Solaris but designed to potentially more portable */
> +
> +#if !defined(__BIG_ENDIAN)
> +#define __BIG_ENDIAN 4321
> +#endif
> +
> +#if !defined(__LITTLE_ENDIAN)
> +#define __LITTLE_ENDIAN 1234
> +#endif
> +
> +#if defined(_BIG_ENDIAN)
> +#define __BYTE_ORDER __BIG_ENDIAN
> +#endif
> +#if defined(_LITTLE_ENDIAN)
> +#define __BYTE_ORDER __LITTLE_ENDIAN
> +#endif

The existing support is only for platforms where all three macros
(BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN) are defined, and the
convention used on such platforms where BYTE_ORDER is set to either
one of the *_ENDIAN macros to tell the code which byte order we
have.  This mimics the convention where __BYTE_ORDER and other two
macros are already defined with two leading underscores, and in such
a case we do not have to do anything.  We make the final decision to
use or bypass bswap64() in our ntohll() implementation based on the
variables with double leading underscores.

This patch seems to address two unrelated issues in that.

 (1) The existing support does not help a platform where the
     convention is to define either _BIG_ENDIAN (with one leading
     underscore) or _LITTLE_ENDIAN and not both, which is Solaris
     but there may be others.

 (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already
     defined on the platform.  Or these may not have been defined at
     all.  You avoid unconditionally redefing these.

I find the latter iffy.

What is the reason for avoiding redefinition?  Is it because you
know the original values they have are precious?  And if so in what
way they are precious?  If the reason of avoiding redefinition is
because you do not even know what their values are (so that you are
trying to be safe by preserving), what other things can you say
about their values you are preserving?

Specifically, do you know that these two are defined differently, so
that defining __BYTE_ORDER to one of them and comparing it to
__BIG_ENDIAN is a good way to tell if the platform is big endian?

I would understand it if (2) were "we undefine if these are defined
and then define them as 4321 and 1234 respectively, in order to
avoid a compiler warning against redefinition of a macro", but that
is not what I am seeing, so I am not sure what you meant to achieve
by that "if !defined()" constructs.

Thanks.

Re: [PATCH] Add extra logic required to detect endianness on Solaris

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Charles Bailey <cbailey32@bloomberg.net> writes:
>
>>  #if !defined(__BYTE_ORDER)
>> +/* Known to be needed on Solaris but designed to potentially more portable */
>> +
>> +#if !defined(__BIG_ENDIAN)
>> +#define __BIG_ENDIAN 4321
>> +#endif
>> +
>> +#if !defined(__LITTLE_ENDIAN)
>> +#define __LITTLE_ENDIAN 1234
>> +#endif
>> +
>> +#if defined(_BIG_ENDIAN)
>> +#define __BYTE_ORDER __BIG_ENDIAN
>> +#endif
>> +#if defined(_LITTLE_ENDIAN)
>> +#define __BYTE_ORDER __LITTLE_ENDIAN
>> +#endif
>
> The existing support is only for platforms where all three macros
> (BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN) are defined, and the
> convention used on such platforms where BYTE_ORDER is set to either
> one of the *_ENDIAN macros to tell the code which byte order we
> have.  This mimics the convention where __BYTE_ORDER and other two
> macros are already defined with two leading underscores, and in such
> a case we do not have to do anything.  We make the final decision to
> use or bypass bswap64() in our ntohll() implementation based on the
> variables with double leading underscores.
>
> This patch seems to address two unrelated issues in that.
>
>  (1) The existing support does not help a platform where the
>      convention is to define either _BIG_ENDIAN (with one leading
>      underscore) or _LITTLE_ENDIAN and not both, which is Solaris
>      but there may be others.
>
>  (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already
>      defined on the platform.  Or these may not have been defined at
>      all.  You avoid unconditionally redefing these.
>
> I find the latter iffy.
>
> What is the reason for avoiding redefinition?  Is it because you
> know the original values they have are precious?  And if so in what
> way they are precious?  If the reason of avoiding redefinition is
> because you do not even know what their values are (so that you are
> trying to be safe by preserving), what other things can you say
> about their values you are preserving?
>
> Specifically, do you know that these two are defined differently, so
> that defining __BYTE_ORDER to one of them and comparing it to
> __BIG_ENDIAN is a good way to tell if the platform is big endian?
>
> I would understand it if (2) were "we undefine if these are defined
> and then define them as 4321 and 1234 respectively, in order to
> avoid a compiler warning against redefinition of a macro", but that
> is not what I am seeing, so I am not sure what you meant to achieve
> by that "if !defined()" constructs.
>
> Thanks.

Just a thought.

I am wondering if you may want to go the other way around.  That is,
instead of using "we have byte-order, big and little and the way to
determine endianness is to see byte-order matches which of the
latter two", use "there may be either big or little but not both
defined, and that is how you learn the byte-order".

And make these two macros match what Solaris happens to use.

I am not sure which variant I like better myself, though.

 compat/bswap.h | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index 120c6c1..e87998e 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -101,19 +101,24 @@ static inline uint64_t git_bswap64(uint64_t x)
 #undef ntohll
 #undef htonll
 
-#if !defined(__BYTE_ORDER)
-# if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
-#  define __BYTE_ORDER BYTE_ORDER
-#  define __LITTLE_ENDIAN LITTLE_ENDIAN
-#  define __BIG_ENDIAN BIG_ENDIAN
-# endif
+#if !defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
+
+#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
+# if BYTE_ORDER == BIG_ENDIAN
+#  define _BIG_ENDIAN
+# else
+#  define _LITTLE_ENDIAN
+#endif
+
 #endif
 
-#if !defined(__BYTE_ORDER)
+#if !defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
 # error "Cannot determine endianness"
+#elif defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN)
+# error "Your endianness is screwed up"
 #endif
 
-#if __BYTE_ORDER == __BIG_ENDIAN
+#if defined (_BIG_ENDIAN)
 # define ntohll(n) (n)
 # define htonll(n) (n)
 #else

Re: [PATCH] Add extra logic required to detect endianness on Solaris

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Just a thought.
>
> I am wondering if you may want to go the other way around.  That is,
> instead of using "we have byte-order, big and little and the way to
> determine endianness is to see byte-order matches which of the
> latter two", use "there may be either big or little but not both
> defined, and that is how you learn the byte-order".
>
> And make these two macros match what Solaris happens to use.
>
> I am not sure which variant I like better myself, though.
> ...

The "how-about-this" patch in the pregvious message forgets the
default case where byte-order, little and big are defined with
leading double underscore, which must also be handled, if we want to
take this aproach.

Re: [PATCH] Add extra logic required to detect endianness on Solaris

[Charles Bailey]

On Thu, May 01, 2014 at 11:58:26AM -0700, Junio C Hamano wrote:
> 
> This patch seems to address two unrelated issues in that.
> 
>  (1) The existing support does not help a platform where the
>      convention is to define either _BIG_ENDIAN (with one leading
>      underscore) or _LITTLE_ENDIAN and not both, which is Solaris
>      but there may be others.
> 
>  (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already
>      defined on the platform.  Or these may not have been defined at
>      all.  You avoid unconditionally redefing these.
> 
> I find the latter iffy.

Yes, you are right. I think I was uncomfortable defining macros with
names reserved for the implementation even if the implementation didn't
seem to be using them. I think I made my patch less correct as a result.
Looking at the rest of the git source code we don't seem to use any of
these macros anywhere else so perhaps we could use macros specific to
GIT?

Let me follow up with an alternative patch.

Charles.

[PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Charles Bailey]

---
 compat/bswap.h | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index 120c6c1..f08a9fe 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -101,19 +101,34 @@ static inline uint64_t git_bswap64(uint64_t x)
 #undef ntohll
 #undef htonll
 
-#if !defined(__BYTE_ORDER)
-# if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
-#  define __BYTE_ORDER BYTE_ORDER
-#  define __LITTLE_ENDIAN LITTLE_ENDIAN
-#  define __BIG_ENDIAN BIG_ENDIAN
+#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
+
+# define GIT_BYTE_ORDER BYTE_ORDER
+# define GIT_LITTLE_ENDIAN LITTLE_ENDIAN
+# define GIT_BIG_ENDIAN BIG_ENDIAN
+
+#elif defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
+
+# define GIT_BYTE_ORDER __BYTE_ORDER
+# define GIT_LITTLE_ENDIAN __LITTLE_ENDIAN
+# define GIT_BIG_ENDIAN __BIG_ENDIAN
+
+#else
+
+# define GIT_BIG_ENDIAN 4321
+# define GIT_LITTLE_ENDIAN 1234
+
+# if defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
+#  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
+# elif defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
+#  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
+# else
+#  error "Cannot determine endianness"
 # endif
-#endif
 
-#if !defined(__BYTE_ORDER)
-# error "Cannot determine endianness"
 #endif
 
-#if __BYTE_ORDER == __BIG_ENDIAN
+#if GIT_BYTE_ORDER == GIT_BIG_ENDIAN
 # define ntohll(n) (n)
 # define htonll(n) (n)
 #else
-- 
1.9.0

Re: [PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Junio C Hamano]

Charles Bailey <cbailey32@bloomberg.net> writes:

> ---

Please sign-off your patches ;-)

This swaps the precedence of BYTE_ORDER and __BYTE_ORDER from the
original, which we may not want to.  It is easy for me to swap the
order of if/elif to restore it, so it is not a big deal, though.

Thanks.

>  compat/bswap.h | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/compat/bswap.h b/compat/bswap.h
> index 120c6c1..f08a9fe 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -101,19 +101,34 @@ static inline uint64_t git_bswap64(uint64_t x)
>  #undef ntohll
>  #undef htonll
>  
> -#if !defined(__BYTE_ORDER)
> -# if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
> -#  define __BYTE_ORDER BYTE_ORDER
> -#  define __LITTLE_ENDIAN LITTLE_ENDIAN
> -#  define __BIG_ENDIAN BIG_ENDIAN
> +#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
> +
> +# define GIT_BYTE_ORDER BYTE_ORDER
> +# define GIT_LITTLE_ENDIAN LITTLE_ENDIAN
> +# define GIT_BIG_ENDIAN BIG_ENDIAN
> +
> +#elif defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
> +
> +# define GIT_BYTE_ORDER __BYTE_ORDER
> +# define GIT_LITTLE_ENDIAN __LITTLE_ENDIAN
> +# define GIT_BIG_ENDIAN __BIG_ENDIAN
> +
> +#else
> +
> +# define GIT_BIG_ENDIAN 4321
> +# define GIT_LITTLE_ENDIAN 1234
> +
> +# if defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
> +#  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
> +# elif defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
> +#  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
> +# else
> +#  error "Cannot determine endianness"
>  # endif
> -#endif
>  
> -#if !defined(__BYTE_ORDER)
> -# error "Cannot determine endianness"
>  #endif
>  
> -#if __BYTE_ORDER == __BIG_ENDIAN
> +#if GIT_BYTE_ORDER == GIT_BIG_ENDIAN
>  # define ntohll(n) (n)
>  # define htonll(n) (n)
>  #else

Re: [PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Charles Bailey]

On Fri, May 02, 2014 at 09:48:58AM -0700, Junio C Hamano wrote:
> Charles Bailey <cbailey32@bloomberg.net> writes:
> 
> > ---
> 
> Please sign-off your patches ;-)

Oops! Please consider this patch...

Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>

> This swaps the precedence of BYTE_ORDER and __BYTE_ORDER from the
> original, which we may not want to.  It is easy for me to swap the
> order of if/elif to restore it, so it is not a big deal, though.

I think I swapped the precedence (semi-deliberately) because I found a
proposal to standardize the BYTE_ORDER variant. I claim that any
platform which provides both but with differing senses is somewhat
broken so I cannot see the precedence mattering much. I don't mind
either way.

Charles.

Re: [PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Junio C Hamano]

Charles Bailey <cbailey32@bloomberg.net> writes:

> I claim that any
> platform which provides both but with differing senses is somewhat
> broken so I cannot see the precedence mattering much.

I agree with that, and that is the reason why we shouldn't change
the order all of a sudden.  If it shouldn't matter, then there is
only downside of a possiblity to break such an insane set-up that
has been happily working by accident, without helping anybody if we
change it, no?

Re: [PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Charles Bailey <cbailey32@bloomberg.net> writes:
>
>> I claim that any
>> platform which provides both but with differing senses is somewhat
>> broken so I cannot see the precedence mattering much.
>
> I agree with that, and that is the reason why we shouldn't change
> the order all of a sudden.  If it shouldn't matter, then there is
> only downside of a possiblity to break such an insane set-up that
> has been happily working by accident, without helping anybody if we
> change it, no?

So,... I am inclined to queue this on top of your patch at least for
now, before I go into incommunicado-mode to finish preparing -rc2.

-- >8 --
Subject: [PATCH] compat/bswap.h: restore preference __BIG_ENDIAN over BIG_ENDIAN

The previous commit swaps the order we check the macros defined by
the compiler and the system headers from the original.  Since the
order of check should not matter (i.e. it is insane to define both
__BIG_ENDIAN and friends and BIG_ENDIAN and friends and in a
conflicting way), it is the most conservative thing to do not to
change it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/bswap.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index f08a9fe..c4293db 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -101,18 +101,18 @@ static inline uint64_t git_bswap64(uint64_t x)
 #undef ntohll
 #undef htonll
 
-#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
-
-# define GIT_BYTE_ORDER BYTE_ORDER
-# define GIT_LITTLE_ENDIAN LITTLE_ENDIAN
-# define GIT_BIG_ENDIAN BIG_ENDIAN
-
-#elif defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
+#if defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
 
 # define GIT_BYTE_ORDER __BYTE_ORDER
 # define GIT_LITTLE_ENDIAN __LITTLE_ENDIAN
 # define GIT_BIG_ENDIAN __BIG_ENDIAN
 
+#elif defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
+
+# define GIT_BYTE_ORDER BYTE_ORDER
+# define GIT_LITTLE_ENDIAN LITTLE_ENDIAN
+# define GIT_BIG_ENDIAN BIG_ENDIAN
+
 #else
 
 # define GIT_BIG_ENDIAN 4321
-- 
2.0.0-rc1-355-gd6d6511

Re: [PATCH] Detect endianness on more platforms that don't use BYTE_ORDER

[Charles Bailey]

On Fri, May 02, 2014 at 12:43:32PM -0700, Junio C Hamano wrote:
> So,... I am inclined to queue this on top of your patch at least for
> now, before I go into incommunicado-mode to finish preparing -rc2.

Yes, I'd agree with that.