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

cmake: really enable warnings with clang #9783

Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 22, 2022

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 Oct 22, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 23, 2022

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 added 3 commits Oct 23, 2022
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
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 Oct 23, 2022
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 24, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 24, 2022

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 Oct 26, 2022
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 26, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 26, 2022

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant