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

[Roland Scheidegger]

Am 25.11.2017 um 05:24 schrieb Matt Turner:
> On Fri, Nov 24, 2017 at 2:25 AM, 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
> 
> In response to Andres' report that the Windows build fails with this
> patch, I was going to take your suggestion... but it doesn't seem like
> u_endian.h has any code to handle Windows.
> 
> I see this crap in p_config.h:
> 
> /*
>  * Endian detection.
>  */
> 
> #include "util/u_endian.h"
> #if !defined(PIPE_ARCH_LITTLE_ENDIAN) && !defined(PIPE_ARCH_BIG_ENDIAN)
> 
> #if defined(PIPE_ARCH_X86) || defined(PIPE_ARCH_X86_64) ||
> defined(PIPE_ARCH_ARM) || defined(PIPE_ARCH_AARCH64)
> #define PIPE_ARCH_LITTLE_ENDIAN
> #elif defined(PIPE_ARCH_PPC) || defined(PIPE_ARCH_PPC_64) ||
> defined(PIPE_ARCH_S390)
> #define PIPE_ARCH_BIG_ENDIAN
> #endif
> 
> #endif
> 
> #if !defined(PIPE_ARCH_LITTLE_ENDIAN) && !defined(PIPE_ARCH_BIG_ENDIAN)
> #error Unknown Endianness
> #endif
> 
> I don't think *any* of that would be necessary if we just handled
> Windows (in the appropriate place -- u_endian.h)!
> 
> José, Roland: I think I'm going to commit a patch to u_endian.h to
> define PIPE_ARCH_LITTLE_ENDIAN in the absense of any platform-specific
> handling, so that the rest of my series can land:
> 
> diff --git a/src/util/u_endian.h b/src/util/u_endian.h
> index 7bbd7dc215..3d5c006f35 100644
> --- a/src/util/u_endian.h
> +++ b/src/util/u_endian.h
> @@ -67,4 +67,7 @@
> 
>  #endif
> 
> +#warn Unknown Endianness for this platform. Assuming little endian
> +#define PIPE_ARCH_LITTLE_ENDIAN
> +
>  #endif
> 
> Presumably Windows only runs on little endian platforms, so that
> should be fine for your purposes.
> 

Note that initially u_endian.h was part of p_config.h (right there where
it's now included), so I guess that's basically fallout from separating,
where u_endian.h now can no longer rely on the other PIPE_ARCH_X86 etc.
bits.
I suppose always defining that in u_endian.h makes sense, albeit I'd
guess you should kick out the bits you quoted in p_config.h then because
u_endian.h will obviously always define the pipe arch endian bit (albeit
I can't tell you if that would always really give you the same answer on
all platforms as before - not talking about windows here).

Roland