Re: pack bitmap woes on Windows

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Feb 12, 2014, 14:49 UTC
Message-ID: <CABPQNSZRfnp8aZuKBc=y=iDVdMUo8mKPsLsqg0E+bK1TdNvfkA@mail.gmail.com>
In-Reply-To: <CABPQNSabAcHEO0+ht7te9ScVigGOoCLdtiD10ye8AqqeLMagLw@mail.gmail.com>

On Wed, Feb 12, 2014 at 3:22 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

Show 38 quoted lines

> On Wed, Feb 12, 2014 at 3:09 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, Feb 12, 2014 at 8:23 PM, David Kastrup <dak@gnu.org> wrote:
>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>>
>>>> Am 2/12/2014 13:55, schrieb David Kastrup:
>>>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>>>>
>>>>>> Running test suite of 'next' on Windows fails in t5310-pack-bitmaps with
>>>>>> the following symptoms. I haven't followed the topic. Have there been
>>>>>> patches floating that addressed the problem in one way or another?
>>>>>>
>>>>>> (gdb) run
>>>>>> Starting program: D:\Src\mingw-git\t\trash
>>>>>> directory.t5310-pack-bitmaps/..\..\git.exe rev-list --test-bitmap
>>>>>> HEAD
>>>>>> [New thread 3528.0x8d4]
>>>>>> Bitmap v1 test (20 entries loaded)
>>>>>> Found bitmap for 537ea4d3eb79c95f602873b1167c480006d2ac2d. 64 bits
>>>>>> / 15873b36 checksum
>>>>>
>>>>> Does reverting a201c20b41a2f0725977bcb89a2a66135d776ba2 help?
>>>>
>>>> YES! t5310 passes after reverting this commit.
>>>
>>> Oh.  I just looked through the backtrace until finding a routine
>>> reasonably related with the regtest and checked for the last commit
>>> changing it, then posted my question.
>>>
>>> Then I looked through the diff of the patch and considered it
>>> unconspicuous.  So I commenced reading through earlier commits.
>>>
>>> I actually don't have a good idea what might be wrong here.  The code is
>>> somewhat distasteful as it basically uses eword_t and uint64_t
>>> interchangeably, but then this does match its current definition.
>>
>> Perhaps __BYTE_ORDER or __BIG_ENDIAN is misdefined and the ntohll() is skipped?
>
> That is indeed the case.

Looking a bit at it, the whole byte-order detection mess seems insane to me.

MinGW falls inside the "defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))"-block, but does not define __BYTE_ORDER.

It seems the author of a201c20b41a2f0725977bcb89a2a66135d776ba2 assumes __BYTE_ORDER was guaranteed to always be defined, probably fooled by the following check:

#if !defined(__BYTE_ORDER) # error "Cannot determine endianness" #endif

However, that check is only performed if we're NOT building with GCC on x86/x64 nor MSVC (which I don't think defined __BYTE_ORDER either)

The following would make __BYTE_ORDER a guarantee, and show that MinGW does not define it:

---8<---

diff --git a/compat/bswap.h b/compat/bswap.h
index 120c6c1..c73bf0e 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -109,10 +109,6 @@ static inline uint64_t git_bswap64(uint64_t x)
 # endif
 #endif

-#if !defined(__BYTE_ORDER)
-# error "Cannot determine endianness"
-#endif
-
 #if __BYTE_ORDER == __BIG_ENDIAN
 # define ntohll(n) (n)
 # define htonll(n) (n)
@@ -123,6 +119,10 @@ static inline uint64_t git_bswap64(uint64_t x)

 #endif

+#if !defined(__BYTE_ORDER)
+# error "Cannot determine endianness"
+#endif
+
 /*
  * Performance might be improved if the CPU architecture is OK with
  * unaligned 32-bit loads and a fast ntohl() is available.

---8<---

But another level of insanity (IMO) is defining double-underscore
macros. These symbols are reserved for the implementation. Sure,
knowing we're on a given implementation and defining something *else*
dependent on them is fine. But defining them is just yuckiness, and
not very standard-kosher.

IMO, we should rather do the definition the other way around:

#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
#endif

...and only referred to BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN.


That way we'd follow closer to where opengroup are heading:

http://www.opengroup.org/austin/docs/austin_514.txt