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

Jump to bottom

cmake: really enable warnings with clang #9783

Closed
wants to merge 3 commits into from
Closed

cmake: really enable warnings with clang #9783

wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 22, 2022 •
edited

Even though PICKY_COMPILER=ON is the default, warnings were not enabled when using llvm/clang, because CMAKE_COMPILER_IS_CLANG was always false (in my tests at least).

This is the single use of this variable in curl, and in a different place we already use CMAKE_C_COMPILER_ID MATCHES "Clang", which works as expected, so change the condition to use that instead.

Also fix the warnings uncovered by the above:

  • lib: add casts to silence clang warnings

  • schannel: add casts to silence clang warnings in ALPN code

    Assuming the code is correct, solve the warnings with a cast.

    There is a chance the warning is relevant for some platforms,
    perhaps ARM7 (32-bit)?
    (This combo isn't CI tested.)

Closes #xxxx

These ones remain in brotli code (in CI it can be made a non-error via -Wno-error=vla):

In file included from ./curl/lib/content_encoding.c:36:
./brotli/x64-ucrt/usr/include/brotli/decode.h:204:34: warning: variable length array used [-Wvla]
    const uint8_t encoded_buffer[BROTLI_ARRAY_PARAM(encoded_size)],
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./brotli/x64-ucrt/usr/include/brotli/port.h:253:34: note: expanded from macro 'BROTLI_ARRAY_PARAM'
#define BROTLI_ARRAY_PARAM(name) (name)
                                 ^~~~~~
In file included from ./curl/lib/content_encoding.c:36:
./brotli/x64-ucrt/usr/include/brotli/decode.h:206:48: warning: variable length array used [-Wvla]
    uint8_t decoded_buffer[BROTLI_ARRAY_PARAM(*decoded_size)]);
                           ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
./brotli/x64-ucrt/usr/include/brotli/port.h:253:35: note: expanded from macro 'BROTLI_ARRAY_PARAM'
#define BROTLI_ARRAY_PARAM(name) (name)                                   
                                 ~^~~~~                                   
2 warnings generated.

@vszakats vszakats added the cmake label Oct 22, 2022
@vszakats vszakats force-pushed the cmake-clang-really-enable-warnings branch 2 times, most recently from 5bf89c5 to c75fc0c Compare October 22, 2022 23:55
@vszakats
Copy link
Member Author

vszakats commented Oct 23, 2022 •
edited

Any takers for these new Schannel -Wcast-align warnings?:

./curl/lib/vtls/schannel.c:1203:21: warning: cast from 'unsigned char *' to 'unsigned int *' increases required alignment from 1 to 4 [-Wcast-align]
    extension_len = (unsigned int *)(&alpn_buffer[cur]);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./curl/lib/vtls/schannel.c:1208:6: warning: cast from 'unsigned char *' to 'unsigned int *' increases required alignment from 1 to 4 [-Wcast-align]
    *(unsigned int *)&alpn_buffer[cur] =
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./curl/lib/vtls/schannel.c:1214:16: warning: cast from 'unsigned char *' to 'unsigned short *' increases required alignment from 1 to 2 [-Wcast-align]
    list_len = (unsigned short*)(&alpn_buffer[cur]);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.

UPDATE: Silenced them. There is chance these warnings are relevant for certain platforms (ARM7?) with particular alignment requirements, though.

@vszakats vszakats added the build label Oct 23, 2022
@vszakats
cmake: really enable warnings with clang
939ea1e 
Even though `PICKY_COMPILER=ON` is the default, warnings were not
enabled when using llvm/clang, because `CMAKE_COMPILER_IS_CLANG` was
always false (in my tests at least).

This is the single use of this variable in curl, and in a different
place we already use `CMAKE_C_COMPILER_ID MATCHES "Clang"`, which
works as expected, so change the condition to use that instead.

Also fix the warnings uncovered by the above:

- lib: add casts to silence clang warnings

- schannel: add casts to silence clang warnings in ALPN code

  Assuming the code is correct, solve the warnings with a cast.

  There is a chance the warning is relevant for some platforms,
  perhaps ARM7 (32-bit)?
  (This combo isn't CI tested.)

Closes #xxxx
@vszakats
lib: add casts to silence clang warnings
f2881a2
@vszakats
schannel: add casts to silence clang warnings in ALPN code
cb9e66e 
Assuming the code is correct, solve the warnings with a cast.

There is a chance the warning is relevant for some platforms,
perhaps ARM7 (32-bit)?

(This combo isn't CI tested.)
@vszakats vszakats force-pushed the cmake-clang-really-enable-warnings branch from 68c78af to cb9e66e Compare October 23, 2022 12:51
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 24, 2022
@vszakats
curl-cmake.sh: add TODO pending an upstream PR [ci skip]
c822988 
Ref: curl/curl#9783
@vszakats
Copy link
Member Author

vszakats commented Oct 24, 2022 •
edited

The casts seems fine, though I'd be happy if someone could take a hard look at them.

One possible issue the warnings may cause is for those who build with llvm/clang and -DCURL_WERROR=ON (our CI for example), and some so far hidden warning comes up. One such case is when building with Brotli, where the warning is in Brotli headers.

But, overall this patch only matches up CMake+clang builds with CMake+gcc and autotools ones, where warnings were enabled all along.

@vszakats vszakats closed this in 811c799 Oct 26, 2022
@vszakats vszakats deleted the cmake-clang-really-enable-warnings branch October 26, 2022 09:58
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 26, 2022
@vszakats
curl-cmake.sh: stop manually enabling warnings after upstream fix [ci…
fcda795 
… skip]

Ref: curl/curl#9783
@vszakats
Copy link
Member Author

vszakats commented Oct 26, 2022 •
edited

Small update: The schannel.c warnings were already visible prior to this patch in autotools with --enable-warnings --with-schannel. (Not in CI runs though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build cmake
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@vszakats