Get rid of autoconf check for endianness

real-or-random commented at 12:09 pm on July 21, 2020: contributor

This does not fix any particular issue but it’s preferable to not rely on autoconf. This avoids endianness mess for users on BE hosts if they use their build without autoconf.

This does not fix any particular issue but it's preferable to not
rely on autoconf. This avoids endianness mess for users on BE hosts
if they use their build without autoconf.

891b9e4f30

real-or-random commented at 12:10 pm on July 21, 2020: contributor

I guess we should merge #696 first.

gmaxwell commented at 0:29 am on July 22, 2020: contributor

9% slowdown in hash_rfc6979_hmac_sha256 on i.MX6 (arm7; gcc 4.9.2). Not needing autotools for target endianness detection sounds good but a slowdown on on a popular LE embedded target does not. endian.h perhaps?

real-or-random commented at 10:33 am on July 22, 2020: contributor

i.MX6 (arm7; gcc 4.9.2).

Hm, I had looked at the ASM for recent compilers: https://godbolt.org/z/ndjvKs Note that there’s a large difference between GCC 4 and newer versions, with and without -mcpu=cortex-a9.

Maybe I’m ignorant but my response would be that I don’t care about the performance with an age-old GCC 4. Is GCC 4.9 common for this target?

real-or-random commented at 12:46 pm on July 22, 2020: contributor

Hashing performance will even less be a concern with a resolution of https://github.com/bitcoin-core/secp256k1/issues/702

gmaxwell commented at 4:47 pm on July 22, 2020: contributor

It’s pretty normal to have older (or even awful) compilers on embedded platforms, esp arm because devices usually depend on a vendor kernel that stops getting upgraded to new major versions and hangs around with old operating systems. On novena, the latest supported OS is debian jessie. The situation for toolchains you might find setup for hardware wallets and whatnot isn’t that different a story.

If there is a choice between being fast on older compilers or new, sure pick new. Or between something much harder to maintain and a small slowdown on older compilers, sure. Bit 9% is pretty big, and a byteswap macro is about as idiomatic as you get.

Small embedded devices don’t usually have hashing hardware so 702 doesn’t really apply, unless you think that it’s okay to just provide objectively worse stuff because the user can fix this libraries mistakes?

Besides, it appears to be a similar similar story elsewhere, if less dramatic. GCC 10.1.1 on PPC64LE:

Before: hash_rfc6979_hmac_sha256: min 5.96us / avg 5.96us / max 5.97us After: hash_rfc6979_hmac_sha256: min 6.10us / avg 6.10us / max 6.10us

real-or-random commented at 12:08 pm on July 23, 2020: contributor

@gmaxwell You mentioned endian.h but this seems to be a portability mess, too. Or which header do you refer to?

Here’s another idea: What about sticking to autoconf but also let it define a macro if it figures out little-endian. Then we can #error out at least (or try __BYTE_ORDER__ as a fallback, which is at least available on GCC/clang, and #error if that fails too). That’s not very ergonomic but at least it errs on the side of caution, whereas we currently simply assume LE if autoconf was not used. Creating wrong hashes could end up terribly, e.g., if you create a schnorr sig with a user-specified nonce function, you could end up signing different hashes with the same nonce.

For enhanced safety, we could even add a cheap check in the context creation that checks at runtime if the endianness matches what it thinks it has been compiled for. Reminds me of my suggestion in #702.

gmaxwell commented at 12:12 pm on July 23, 2020: contributor

Sounds okay to me. Also, erroring on a non-configuration makes sense. Portability is kinda complicated– in the sense that a lot of things are in theory non-portable but in practice are fine (and a few things are in theory portable but in practice are not). I think things like BYTE_ORDER are reasonably widely supported. Anyone building for a really weird platform is going to be able to follow a clearly stated #error.

real-or-random commented at 12:24 pm on July 23, 2020: contributor

Portability is kinda complicated– in the sense that a lot of things are in theory non-portable but in practice are fine (and a few things are in theory portable but in practice are not).

I was specifically referring to endian.h not being available on Mac OS for example, which is a practical issue.

I think things like BYTE_ORDER are reasonably widely supported.

By the way, I found this hell yesterday: :D https://gist.github.com/jtbr/7a43e6281e6cca353b33ee501421860c#file-endianness-h-L39

I guess the combination of __BYTE_ORDER__ (just this none, none of the weird stuff behind the link) and #error is good enough.

elichai commented at 12:25 pm on July 23, 2020: contributor

What do you think about something like this? https://godbolt.org/z/hfYa7K It still has the conditional you were trying to avoid, but it doesn’t rely on autotools.

real-or-random commented at 2:36 pm on July 23, 2020: contributor

What do you think about something like this? https://godbolt.org/z/hfYa7K It still has the conditional you were trying to avoid, but it doesn’t rely on autotools.

Oh letting constant folding optimize the run-time away is really a neat idea. GCC 4.9 seems to do better here.

GCC 4.4.7 x86 and ARM GCC 4.5.4 without -mcpu=cortex-a9 still don’t get this, but this seems good enough. Hey, even MSVC manages to optimize this… Or what do you think @gmaxwell ?

gmaxwell commented at 7:53 pm on July 23, 2020: contributor

That looks like an extremely non-ideomatic way to do a byteswap and I wouldn’t be particularly confident that some random compiler would handle it correctly, or that the optimization wouldn’t randomly break when a butterfly flapped its wings in the amazon.

elichai commented at 10:42 am on July 24, 2020: contributor

That looks like an extremely non-ideomatic way to do a byteswap and I wouldn’t be particularly confident that some random compiler would handle it correctly, or that the optimization wouldn’t randomly break when a butterfly flapped its wings in the amazon.

Yeah it’s quite sketchy hehe, I first saw something like this in NIST submissions, it was way worse and had UB, it was something along these lines:

 0typedef enum {BIG, LITTLE, MIDDLE} Endianess;
 1
 2Endianess Endian() {
 3    unsigned char arr[8];
 4    *(unsigned int*)arr = 0x01020304;
 5    if (arr[0] == 0x04) {
 6        return LITTLE;
 7    } else if (arr[0] == 0x01) {
 8        return BIG;
 9    } else {
10        return MIDDLE;
11    }
12}

sipa commented at 6:13 pm on July 24, 2020: contributor

FWIW, it seems that literally every compiler on godbolt (including the obscure ones outside of clang/gcc/icc/msvc, and ones for less common platforms) supports __BYTE_ORDER (and it equalling either __BIG_ENDIAN or __LITTLE_ENDIAN).

Perhaps it’s reasonable to just use that in non-autotools environments?

real-or-random commented at 6:38 pm on July 24, 2020: contributor

FWIW, it seems that literally every compiler on godbolt (including the obscure ones outside of clang/gcc/icc/msvc, and ones for less common platforms) supports __BYTE_ORDER (and it equalling either __BIG_ENDIAN or __LITTLE_ENDIAN).

Perhaps it’s reasonable to just use that in non-autotools environments?

Nice, then let’s just do this and #error if it’s not defined. I’ll update the PR.

real-or-random commented at 1:00 pm on August 7, 2020: contributor

FWIW, it seems that literally every compiler on godbolt (including the obscure ones outside of clang/gcc/icc/msvc, and ones for less common platforms) supports __BYTE_ORDER (and it equalling either __BIG_ENDIAN or __LITTLE_ENDIAN).

I guess you meant __BYTE_ORDER__, that’s apparently defined almost everywhere. A notable exception is MSVC of course… But we can test for this with _MSC_VER and it then offers _WIN32, which is “Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.” https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=vs-2019

real-or-random cross-referenced this on Aug 7, 2020 from issue Use preprocessor macros instead of autoconf to detect endianness by real-or-random

real-or-random commented at 2:34 pm on August 7, 2020: contributor

Superseded by #787.

real-or-random closed this on Aug 7, 2020

real-or-random referenced this in commit 979961c506 on Aug 13, 2020

Get rid of autoconf check for endianness #770