Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use preprocessor macros instead of autoconf to detect endianness #787

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Use preprocessor macros instead of autoconf to detect endianness #787

merged 1 commit into from
Aug 13, 2020

Conversation

real-or-random
Copy link
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.

The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.

Supersedes #770 .

@real-or-random real-or-random force-pushed the 202008-endian-preprocessor branch 3 times, most recently from 5f1ee37 to ac43320 Compare August 7, 2020 14:33
@real-or-random real-or-random mentioned this pull request Aug 7, 2020
Closed
@gmaxwell
Copy link
Contributor

gmaxwell commented Aug 7, 2020

I like this approach.

@gmaxwell
Copy link
Contributor

gmaxwell commented Aug 8, 2020

Travis has a zero log apparently unexplained failure on s390 ... the only test that really mattered for this pr. :)

I tested it by hand on s390 and it works. sooo ?!

@sipa
Copy link
Contributor

sipa commented Aug 8, 2020

I checked this PR a few hours ago, and all tests had finished, except the s390x one, which hadn't started.

So I think there may be some provisioning issue that prevents travis from assigning an s390x worker, and it somehow timed out?

@JeremyRubin
Copy link

This is also the case on Bitcoin Core right now too :/

@real-or-random
Copy link
Contributor Author

Yeah, first I felt brave when I squashed #696 and then merged it immediately. And then I got a notification from Travis that I broke master... :/ But yes, it's an issue on their side...

@sipa sipa mentioned this pull request Aug 10, 2020
Closed
@sipa
Copy link
Contributor

sipa commented Aug 10, 2020

Opened a PR for Bitcoin Core bitcoin/bitcoin#19695 to test if this may impact any of the systems tested by its CI (which includes an MSVC based one).

@sipa sipa mentioned this pull request Aug 10, 2020
Open
@gmaxwell
Copy link
Contributor

It's _MSC_VER not _MSVC_VER (thanks to sipa abusing bitcoin's CI setup to test this.)

@real-or-random
Use preprocessor macros instead of autoconf to detect endianness
0dccf98 
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.

The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.
@real-or-random real-or-random force-pushed the 202008-endian-preprocessor branch from ac43320 to 0dccf98 Compare August 11, 2020 09:25
@real-or-random
Copy link
Contributor Author

Fixed. @sipa can you retest on Core?

@sipa
Copy link
Contributor

sipa commented Aug 11, 2020

@real-or-random Updated it.

@real-or-random
Copy link
Contributor Author

@real-or-random Updated it.

It works. 🎉

sipa
sipa reviewed Aug 12, 2020
View reviewed changes
src/util.h
# define SECP256K1_LITTLE_ENDIAN
#endif
#if defined(SECP256K1_LITTLE_ENDIAN) == defined(SECP256K1_BIG_ENDIAN)
# error Please make sure that either SECP256K1_LITTLE_ENDIAN or SECP256K1_BIG_ENDIAN is set, see src/util.h.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never knew that you could put an error message after #error without quotes around it.

In retrospect, not sure what I'd expect it to do. Give an error?

@sipa
Copy link
Contributor

sipa commented Aug 12, 2020

ACK 0dccf98

1 similar comment
@gmaxwell
Copy link
Contributor

ACK 0dccf98

@real-or-random real-or-random merged commit 979961c into bitcoin-core:master Aug 13, 2020
@gmaxwell
Copy link
Contributor

This broke the build on GCC 2.95 on Debian i386 for me and on Haiku (BeOS).

@sipa
Copy link
Contributor

sipa commented Aug 13, 2020

@gmaxwell Can you give the list of preprocessor defines on those?

@gmaxwell
Copy link
Contributor

gmaxwell commented Aug 13, 2020 •
edited
Loading 

    $ gcc -dM -E -x c -
    #define __linux__ 1 
    #define linux 1 
    #define __i386__ 1 
    #define __i386 1 
    #define __i686 1 
    #define __GNUC_MINOR__ 95 
    #define pentiumpro 1 
    #define __pentiumpro 1 
    #define i386 1 
    #define i686 1 
    #define __pentiumpro__ 1 
    #define __unix 1 
    #define __unix__ 1 
    #define __i686__ 1 
    #define __GNUC__ 2 
    #define __linux 1 
    #define __ELF__ 1 
    #define unix 1

From the haiku I can't easily copy and paste, but it includes,

__i586 __i386 _X86_ __i386__ __i586__ __pentium __pentium__

among other less relevant ones.

@sipa sipa mentioned this pull request Aug 14, 2020
Merged
@sipa
Copy link
Contributor

sipa commented Aug 14, 2020

@gmaxwell See #799

real-or-random added a commit that referenced this pull request Sep 9, 2020
@real-or-random
Merge #799: Add fallback LE/BE for architectures with known endiannes…
54caf2e 
…s + SHA256 selftest

8bc6aef Add SHA256 selftest (Pieter Wuille)
5e5fb28 Use additional system macros to figure out endianness (Pieter Wuille)

Pull request description:

  These are all the architecture macros I could find with known endianness. Use those as a fallback when __BYTE_ORDER__ isn't available.

  See #787 (comment)

  It also adds a SHA256 selftest, so that improperly overriding the endianness detection will be detected at runtime.

ACKs for top commit:
  real-or-random:
    ACK 8bc6aef I read the diff, and tested that the self-test passes/fails with/without the correct endianness setting
  gmaxwell:
    ACK 8bc6aef  looks good and I also ran the tests on MIPS-BE and verified that forcing it to LE makes the runtime test  fail.

Tree-SHA512: aca4cebcd0715dcf5b58f5763cb4283af238987f43bd83a650e38e127f348131692b2eed7ae5b2ae96046d9b971fc77c6ab44467689399fe470a605c3458ecc5
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 27, 2020
@real-or-random @deadalnix
Use preprocessor macros instead of autoconf to detect endianness
59ff264 
Summary:
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.

The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.

This is a backport of secp256k1 [[bitcoin-core/secp256k1#787 | PR787]]

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D7600
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Sep 28, 2020
@real-or-random @deadalnix
Use preprocessor macros instead of autoconf to detect endianness
1bf8cda 
Summary:
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.

The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.

This is a backport of secp256k1 [[bitcoin-core/secp256k1#787 | PR787]]

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D7600
@apoelstra apoelstra mentioned this pull request Oct 9, 2020
Merged
@elichai elichai mentioned this pull request Dec 29, 2020
Merged
@real-or-random real-or-random mentioned this pull request Mar 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sipa sipa sipa left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@real-or-random @gmaxwell @sipa @JeremyRubin