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

Fixing an issue with CNIOSHA1 missing an #include for the BYTE_ORDER define. #2584

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

ashgti
Copy link
Contributor

@ashgti ashgti commented Nov 2, 2023

When building swift-nio with a system with explicit modules (bazel build rules) I was getting incorrect sha1 results. It turns out the root cause is the BYTE_ORDER macro was not defined in my build context.

Using -Wundef in clang I was seeing:

Sources/CNIOSHA1/c_nio_sha1.c:56:7: error: '__linux__' is not defined, evaluates to 0 [-Werror,-Wundef]
      ^
Sources/CNIOSHA1/c_nio_sha1.c:63:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:63:19: error: 'BIG_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
                  ^
Sources/CNIOSHA1/c_nio_sha1.c:113:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:113:19: error: 'LITTLE_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
                  ^
Sources/CNIOSHA1/c_nio_sha1.c:222:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:222:19: error: 'BIG_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
                  ^
Sources/CNIOSHA1/c_nio_sha1.c:267:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:267:19: error: 'BIG_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]

The soundness check was not actually signaling an error because it was implicitly comparing 0 to 0.

This change includes an update to the #include's to ensure BIG_ENDIAN is defined on macOS and updates the soundness check to have an explicit error if BYTE_ORDER is not defined.

@FranzBusch
Copy link
Member

@swift-ci please test

@Lukasa
Copy link
Contributor

Lukasa commented Nov 6, 2023

@swift-server-bot test this please

@Lukasa Lukasa added the semver/patch No public API change. label Nov 6, 2023
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

nice cleanup, thanks.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 6, 2023

Nightly compiler crash expected, fixed by swiftlang/swift#69518

…define.

When building swift-nio with a system with explicit modules (bazel build rules) I was getting incorrect sha1 results. It turns out the root cause is the `BYTE_ORDER` macro was not defined in my build context.

Using `-Wundef` in clang I was seeing:

```
Sources/CNIOSHA1/c_nio_sha1.c:56:7: error: '__linux__' is not defined, evaluates to 0 [-Werror,-Wundef]
      ^
Sources/CNIOSHA1/c_nio_sha1.c:63:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:63:19: error: 'BIG_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
                  ^
Sources/CNIOSHA1/c_nio_sha1.c:113:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:113:19: error: 'LITTLE_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
                  ^
Sources/CNIOSHA1/c_nio_sha1.c:222:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:222:19: error: 'BIG_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
                  ^
Sources/CNIOSHA1/c_nio_sha1.c:267:5: error: 'BYTE_ORDER' is not defined, evaluates to 0 [-Werror,-Wundef]
    ^
Sources/CNIOSHA1/c_nio_sha1.c:267:19: error: 'BIG_ENDIAN' is not defined, evaluates to 0 [-Werror,-Wundef]
```

The soundness check was not actually signaling an error because it was implicitly comparing 0 to 0.

This change includes an update to the `#include`'s to ensure `BIG_ENDIAN` is defined on macOS and updates the soundness check to have an explicit error if `BYTE_ORDER` is not defined.
@Lukasa
Copy link
Contributor

Lukasa commented Nov 9, 2023

@swift-server-bot test this please

@Lukasa Lukasa enabled auto-merge (squash) November 9, 2023 18:09
@Lukasa Lukasa merged commit fa977dc into apple:main Nov 9, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants