[Mesa-dev] [PATCH 1/3] util: Fix SHA1 implementation on big endian

[Matt Turner]

On Fri, Nov 24, 2017 at 8:51 AM, Andres Gomez <agomez at igalia.com> wrote:
> On Fri, 2017-11-24 at 13:32 +0000, Emil Velikov wrote:
>> On 24 November 2017 at 10:25, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
>> > On Thursday, 2017-11-23 11:08:04 -0800, Matt Turner wrote:
>> > > The code defines a macro blk0(i) based on the preprocessor condition
>> > > BYTE_ORDER == LITTLE_ENDIAN. If true, blk0(i) is defined as a byte swap
>> > > operation. Unfortunately, if the preprocessor macros used in the test
>> > > are no defined, then the comparison becomes 0 == 0 and it evaluates as
>> > > true.
>> > > ---
>> > >  src/util/sha1/sha1.c | 9 +++++++++
>> > >  1 file changed, 9 insertions(+)
>> > >
>> > > diff --git a/src/util/sha1/sha1.c b/src/util/sha1/sha1.c
>> > > index ef59ea1dfc..2c629520c3 100644
>> > > --- a/src/util/sha1/sha1.c
>> > > +++ b/src/util/sha1/sha1.c
>> > > @@ -16,8 +16,17 @@
>> > >
>> > >  #include <stdint.h>
>> > >  #include <string.h>
>> > > +#include "u_endian.h"
>> >
>> > If you're including this header, why not use
>> > `#ifdef PIPE_ARCH_LITTLE_ENDIAN` instead of
>> > `#if BYTE_ORDER == LITTLE_ENDIAN`?
>> >
>> > `#error` becomes unnecessary
>> >
>>
>> I won't bother with that - we do want to address all the cases where
>> undefined macro is used in #if statement.
>> This is handled by -Wundef which seemingly is not part of Wall and we
>> don't use - patch incoming in a second.
>>
>> We want to make it a Werror=undef after we fix the ~60 issues. More
>> than half of those are coming from gtest :-\
>>
>> Thanks for fixing these Matt. As-is the series is
>> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
> Matt, this is breaking Windows compilation.
>
> You can check the Appveyor output here:
> https://ci.appveyor.com/project/AndresGomez/mesa-dwep2/build/488
>
> I've also seen a similar error with the MinGW crosscompilation to
> Windows:
>
> --
>
> src/util/hash_table.c: In function ‘_mesa_hash_table_u64_insert’:
> src/util/hash_table.c:591:42: warning: cast to pointer from integer of
> different size [-Wint-to-pointer-cast]
>        _mesa_hash_table_insert(ht->table, (void *)key, data);
>                                           ^
> src/util/hash_table.c: In function ‘hash_table_u64_search’:
> src/util/hash_table.c:607:49: warning: cast to pointer from integer of
> different size [-Wint-to-pointer-cast]
>        return _mesa_hash_table_search(ht->table, (void *)key);
>                                                  ^
> src/util/sha1/sha1.c:23:2: error: #error BYTE_ORDER not defined
>  #error BYTE_ORDER not defined
>   ^
> src/util/sha1/sha1.c:27:2: error: #error LITTLE_ENDIAN no defined
>  #error LITTLE_ENDIAN no defined
>   ^
> scons: *** [build/windows-x86-debug/util/sha1/sha1.o] Error 1
> scons: building terminated because of errors.
>
> --
> Br,
>
> Andres

Thank you for testing. That helps a lot. In that case, I think I'll
take Eric's suggestion to use PIPE_ARCH_* which I know will be
defined.