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.

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

ci: Bump GCC_SNAPSHOT_MAJOR to 15 #1583

Merged
merged 2 commits into from
Aug 17, 2024
Merged

Conversation

maflcko
Copy link
Contributor

@maflcko maflcko commented Aug 12, 2024

Follow-up to #1313

Clang should silently follow the main devel branch, but GCC needs to be bumped manually.

@real-or-random
Copy link
Contributor

GCC 15 has a new warning, see https://gcc.gnu.org/pipermail/gcc-patches/2024-June/656014.html .

@maflcko
Copy link
Contributor Author

maflcko commented Aug 12, 2024

Yeah, I guess one solution would be to be verbose and write the array:

static const unsigned char PREFIX[19] = {'s', 'e', 'c', 'p', '2', '5', '6', 'k', '1', ' ', 't', 'e', 's', 't', ' ', 'i', 'n', 'i', 't'};

an alternative would be to disable the warning, which seems cleaner and any issues that the warning theoretically could uncover should be detectable by a the existing CI sanitizer builds?

@real-or-random
Copy link
Contributor

real-or-random commented Aug 12, 2024

static const unsigned char PREFIX[19] = {'s', 'e', 'c', 'p', '2', '5', '6', 'k', '1', ' ', 't', 'e', 's', 't', ' ', 'i', 'n', 'i', 't'};

This is ultimately a question of taste, but I suggest this because I think it's the most readable:

static const unsigned char PREFIX[] = {'s', 'e', 'c', 'p', '2', '5', '6', 'k', '1', ' ', 't', 'e', 's', 't', ' ', 'i', 'n', 'i', 't'};

This makes it unambiguously clear that we consider it an array and not a string (i.e., it's not null-terminated), the reader doesn't need to count the chars, and we can use sizeof(PREFIX).

We define such constant arrays in multiple code locations, see
https://github.com/bitcoin-core/secp256k1/actions/runs/10356727938/job/28667982412?pr=1583
Whenever I review such code, I need to count the chars manually to check whether the terminating null byte is included or not.

The previous code is correct and harmless to initialize an array with a
non-terminated character sequence using a string literal.

However, it requires exactly specifying the array size, which can be
cumbersome.

Also, GCC-15 may issue the -Wunterminated-string-initialization warning.
[1]

Fix both issues by using array initialization. This refactoring commit
does not change behavior.

[1] Example warning:

src/modules/schnorrsig/main_impl.h:48:46: error: initializer-string for array of 'unsigned char' is too long [-Werror=unterminated-string-initialization]
   48 | static const unsigned char bip340_algo[13] = "BIP0340/nonce";
      |                                              ^~~~~~~~~~~~~~~
@maflcko
Copy link
Contributor Author

maflcko commented Aug 15, 2024

Whenever I review such code, I need to count the chars manually to check whether the terminating null byte is included or not.

Good point about the review overhead and ambiguity. I pushed a commit with your suggestion.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa67b67, I have reviewed the code and it looks OK.

I can see gcc version 15.0.0 20240811 (experimental) (GCC) in the CI logs.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK fa67b67

@real-or-random real-or-random merged commit b307614 into bitcoin-core:master Aug 17, 2024
116 checks passed
@maflcko maflcko deleted the patch-1 branch August 19, 2024 06:56
hebasto added a commit to hebasto/bitcoin that referenced this pull request Sep 7, 2024
2f2ccc4695 Merge bitcoin-core/secp256k1#1600: cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable
421ed1b46f cmake: Introduce `SECP256K1_APPEND_LDFLAGS` variable
1988855079 Merge bitcoin-core/secp256k1#1586: fix: remove duplicate 'the' from header file comment
b307614401 Merge bitcoin-core/secp256k1#1583: ci: Bump GCC_SNAPSHOT_MAJOR to 15
fa67b6752d refactor: Use array initialization for unterminated strings
9b0f37bff1 fix: remove duplicate 'the' from header file comment
e34b476730 ci: Bump GCC_SNAPSHOT_MAJOR to 15
3fdf146bad Merge bitcoin-core/secp256k1#1578: ci: Silent Homebrew's noisy reinstall warnings
f8c1b0e0e6 Merge bitcoin-core/secp256k1#1577: release cleanup: bump version after 0.5.1
7057d3c9af ci: Silent Homebrew's noisy reinstall warnings
c3e40d75db release cleanup: bump version after 0.5.1

git-subtree-dir: src/secp256k1
git-subtree-split: 2f2ccc469540fde6495959cec061e95aab033148
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Sep 9, 2024
6115628 Squashed 'src/secp256k1/' changes from 642c885b61..2f2ccc4695 (Hennadii Stepanov)

Pull request description:

  This PR updates the libsecp256k1 subtree to bitcoin-core/secp256k1@2f2ccc4, which includes the following changes:
  - bitcoin-core/secp256k1#1577
  - bitcoin-core/secp256k1#1578
  - bitcoin-core/secp256k1#1583
  - bitcoin-core/secp256k1#1586
  - bitcoin-core/secp256k1#1600

  The latter is required for #30791.

ACKs for top commit:
  l0rinc:
    utACK ff54395
  real-or-random:
    utACK ff54395 no blockers from libsecp256k1 side, and these commits touch only build/docs/tests and are thus particularly harmless
  fanquake:
    ACK ff54395

Tree-SHA512: 77cf1ba9aa24efdcf01e99850ea8bed54f847307a3c98c10289c9b7fd9e70c9219f28bee0f00ef7d69979d95a0ddac1e937d3d183ebc9d9b8e6cce0db1d507c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants