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

Fixes for building with -pedantic #1608

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Fixes for building with -pedantic #1608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 45 additions & 9 deletions .github/workflows/actions-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,13 +259,49 @@ jobs:
- name: Run tests
run: cmake --build ./build --target run_tests

gcc-13-pedantic:
if: github.repository_owner == 'aws'
needs: [ sanity-test-run ]
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v3
- name: Setup CMake
uses: threeal/cmake-action@v1.3.0
with:
generator: Ninja
c-compiler: gcc-13
cxx-compiler: g++-13
options: CMAKE_BUILD_TYPE=Release CMAKE_C_FLAGS=-pedantic CMAKE_CXX_FLAGS=-pedantic
- name: Build Crypto
run: cmake --build ./build --target crypto
- 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 •
edited
Loading

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.

if: github.repository_owner == 'aws'
needs: [ sanity-test-run ]
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v3
- name: Setup CMake
uses: threeal/cmake-action@v1.3.0
with:
generator: Ninja
c-compiler: clang-18
cxx-compiler: clang++-18
options: CMAKE_BUILD_TYPE=Release CMAKE_C_FLAGS=-pedantic CMAKE_CXX_FLAGS=-pedantic
- name: Build Crypto
run: cmake --build ./build --target crypto
- name: Build SSL
run: cmake --build ./build --target ssl

clang-ubuntu-2004-sanity:
if: github.repository_owner == 'aws'
needs: [sanity-test-run]
strategy:
fail-fast: false
matrix:
gccversion:
clangversion:
- "10"
- "11"
- "12"
Expand All @@ -282,8 +318,8 @@ jobs:
uses: threeal/cmake-action@v1.3.0
with:
generator: Ninja
c-compiler: clang-${{ matrix.gccversion }}
cxx-compiler: clang++-${{ matrix.gccversion }}
c-compiler: clang-${{ matrix.clangversion }}
cxx-compiler: clang++-${{ matrix.clangversion }}
options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release
- name: Build Project
run: cmake --build ./build --target all
Expand All @@ -296,7 +332,7 @@ jobs:
strategy:
fail-fast: false
matrix:
gccversion:
clangversion:
- "13"
- "14"
- "15"
Expand All @@ -313,8 +349,8 @@ jobs:
uses: threeal/cmake-action@v1.3.0
with:
generator: Ninja
c-compiler: clang-${{ matrix.gccversion }}
cxx-compiler: clang++-${{ matrix.gccversion }}
c-compiler: clang-${{ matrix.clangversion }}
cxx-compiler: clang++-${{ matrix.clangversion }}
options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release
- name: Build Project
run: cmake --build ./build --target all
Expand All @@ -327,7 +363,7 @@ jobs:
strategy:
fail-fast: false
matrix:
gccversion:
clangversion:
- "16"
- "17"
- "18"
Expand All @@ -344,8 +380,8 @@ jobs:
uses: threeal/cmake-action@v1.3.0
with:
generator: Ninja
c-compiler: clang-${{ matrix.gccversion }}
cxx-compiler: clang++-${{ matrix.gccversion }}
c-compiler: clang-${{ matrix.clangversion }}
cxx-compiler: clang++-${{ matrix.clangversion }}
options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release
- name: Build Project
run: cmake --build ./build --target all
Expand Down
5 changes: 3 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,9 @@ if(GCC OR CLANG)
set(C_CXX_FLAGS "${C_CXX_FLAGS} -Wall -fvisibility=hidden -fno-common")
endif()
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wwrite-strings -Wformat-security -Wunused-result")
set(C_CXX_FLAGS "${C_CXX_FLAGS} -Wvla -Wtype-limits -Wno-unused-parameter")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wwrite-strings -Wformat-security -Wunused-result -Wno-overlength-strings")
set(CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} -Wno-newline-eof")
set(C_CXX_FLAGS "${C_CXX_FLAGS} -Wno-c11-extensions -Wvla -Wtype-limits -Wno-unused-parameter")
endif()
set(C_CXX_FLAGS "${C_CXX_FLAGS} -Werror -Wformat=2 -Wsign-compare -Wmissing-field-initializers -Wwrite-strings")

Expand Down
16 changes: 8 additions & 8 deletions crypto/fipsmodule/hmac/hmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,14 @@ struct hmac_methods_st {
// The maximum number of HMAC implementations
#define HMAC_METHOD_MAX 8

MD_TRAMPOLINES_EXPLICIT(MD5, MD5_CTX, MD5_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA1, SHA_CTX, SHA_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA224, SHA256_CTX, SHA256_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA256, SHA256_CTX, SHA256_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA384, SHA512_CTX, SHA512_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA512, SHA512_CTX, SHA512_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA512_224, SHA512_CTX, SHA512_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(SHA512_256, SHA512_CTX, SHA512_CBLOCK);
MD_TRAMPOLINES_EXPLICIT(MD5, MD5_CTX, MD5_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA1, SHA_CTX, SHA_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA224, SHA256_CTX, SHA256_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA256, SHA256_CTX, SHA256_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA384, SHA512_CTX, SHA512_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA512, SHA512_CTX, SHA512_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA512_224, SHA512_CTX, SHA512_CBLOCK)
MD_TRAMPOLINES_EXPLICIT(SHA512_256, SHA512_CTX, SHA512_CBLOCK)

struct hmac_method_array_st {
HmacMethods methods[HMAC_METHOD_MAX];
Expand Down
2 changes: 1 addition & 1 deletion crypto/mem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ static void __asan_unpoison_memory_region(const void *addr, size_t size) {}
// implementation is statically linked with BoringSSL. So, if |sdallocx| is
// provided in, say, libc.so, we still won't use it because that's dynamically
// linked. This isn't an ideal result, but its helps in some cases.
WEAK_SYMBOL_FUNC(void, sdallocx, (void *ptr, size_t size, int flags));
WEAK_SYMBOL_FUNC(void, sdallocx, (void *ptr, size_t size, int flags))

// The following four functions can be defined to override default heap
// allocation and freeing. If defined, it is the responsibility of
Expand Down
Loading