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

Fixes for building with -pedantic #1608

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Conversation

justsmth
Copy link
Contributor

@justsmth justsmth commented May 28, 2024

Issues:

Relates to: aws/aws-lc-rs#425

Description of changes:

  • Fix compiler errors that occur when building the "crypto" & "ssl" targets with -pedantic.
  • Adds CI jobs gcc-14-pedantic and clang-18-pedantic.

Call-outs:

  • This only verifies that the "crypto" and "ssl" targets can be built. These are the two targets required by downstream packages, such as aws-lc-sys.
  • The -Wno-c99-c11-compat (GCC) and -Wno-c11-extensions (Clang) are to avoid a pedantic warning related to our use of _Generic:
/home/justsmth/repos/aws-lc/crypto/asn1/../internal.h:1099:4: error: ISO C99 does not support ‘_Generic’ [-Werror=pedantic]
 1099 |   (_Generic((x),                                    \
      |    ^~~~~~~~
  • The -Wno-newline-eof is needed b/c Clang complains about a missing newline at the end of the s2n-bignum assembly files:
/home/justsmth/repos/aws-lc/third_party/s2n-bignum/include/_internal_s2n_bignum.h:17:7: warning: no newline at end of file [-Wnewline-eof]
   17 | #endif
      |       ^
  • The -Wno-overlength-strings parameter was needed to avoid a failure due to string literals in err-data.c:
...
/tmp/tmp.SO3vhKEAo4/AWS-LC-BUILD/crypto/err_data.c:1603:5: error: string length ‘16178’ is greater than the length ‘4095’ ISO C99 compilers are required to support [-Werror=overlength-strings]
 1603 |     "";
      |     ^~
...
  • Building our tests with -pedantic will still fail due largely to our tests using binary literals:
...
Files/crypto_test.dir/endian_test.cc.o -MF crypto/CMakeFiles/crypto_test.dir/endian_test.cc.o.d -o crypto/CMakeFiles/crypto_test.dir/endian_test.cc.o -c /home/justsmth/repos/aws-lc/crypto/endian_test.cc
/home/justsmth/repos/aws-lc/crypto/endian_test.cc:69:20: error: binary constants are a C++14 feature or GCC extension [-Werror]
   69 |   uint32_t value = 0b00000010000000000000000000000;
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/justsmth/repos/aws-lc/crypto/endian_test.cc:70:23: error: binary constants are a C++14 feature or GCC extension [-Werror]
   70 |   uint32_t expected = 0b00100000000000000000000000000;
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/justsmth/repos/aws-lc/crypto/endian_test.cc:84:20: error: binary constants are a C++14 feature or GCC extension [-Werror]
   84 |   uint64_t value = 0b0000001000000000000000000000000000010000000000000000000000;
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.05%. Comparing base (8258d73) to head (ea5a923).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1608   +/-   ##
=======================================
  Coverage   78.05%   78.05%           
=======================================
  Files         562      562           
  Lines       94600    94600           
  Branches    13575    13575           
=======================================
+ Hits        73840    73842    +2     
+ Misses      20168    20165    -3     
- Partials      592      593    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justsmth justsmth force-pushed the fix-pedantic branch 2 times, most recently from a849701 to e5504ef Compare May 29, 2024 12:30
@justsmth justsmth changed the title [DRAFT] Fix build with -pedantic Fixes for building with -pedantic May 29, 2024
@justsmth justsmth marked this pull request as ready for review May 29, 2024 12:37
@justsmth justsmth requested a review from a team as a code owner May 29, 2024 12:37
@justsmth justsmth force-pushed the fix-pedantic branch 3 times, most recently from daa3f31 to 70a7c9e Compare May 31, 2024 20:41
- name: Build SSL
run: cmake --build ./build --target ssl

clang-18-pedantic:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need distinct CI jobs for pedantry? can we add these flags to existing GCC 13 / clang 18 jobs?

Copy link
Contributor Author

@justsmth justsmth Jun 4, 2024

Choose a reason for hiding this comment

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

Supporting a "pedantic" build can be very painful/restrictive. With this PR we are able to compile the "crypto" and "ssl" build targets with -pedantic, but we can't build/run the tests against it.

Since our tests are a better indicator for the health of a build than pedantic warnings, I chose to limit the use of -pedantic here for just helping ensure that downstream consumers aren't blocked b/c they have a build environment that requires -pedantic.

I used only the latest versions of GCC and Clang since those would likely produce a superset of the pedantic warnings from their previous versions.

@justsmth justsmth merged commit ff3d99c into aws:main Jun 5, 2024
92 of 94 checks passed
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.

5 participants