cmake, `Makefile.mk`: use `-isystem` for dep headers, silence BearSSL issues by vszakats · Pull Request #14763 · curl/curl

vszakats · 2024-09-02T21:50:51Z

Patch started out for working around compiler warnings in BearSSL latest
tarball release v0.6 (2018-08-14) and Apple clang 14 with CMake.

Then turned into patching CMake and Makefile.mk builds to use
-isystem instead -I when adding header directories for
dependencies. This avoids compiler warnings in dependency headers,
syncing behaviour with autotools.

Also:

Makefile.mk: add support for BearSSL.
delete warning suppression for mbedTLS headers. No longer necessary
after this patch.
Follow-up to 434db99 mbedtls: fix -Wnull-dereference and -Wredundant-decls #12720

Silenced BearSSL warnings:

In file included from curl/lib/vtls/bearssl.c:28:
In file included from bearssl/inc/bearssl.h:127:
bearssl/inc/bearssl_hash.h:727:5: warning: 'BR_DOXYGEN_IGNORE' is not defined, evaluates to 0 [-Wundef]
    ^
bearssl/inc/bearssl_hash.h:745:5: warning: 'BR_DOXYGEN_IGNORE' is not defined, evaluates to 0 [-Wundef]
    ^
In file included from curl/lib/vtls/bearssl.c:28:
In file included from bearssl/inc/bearssl.h:136:
bearssl/inc/bearssl_ssl.h:1253:20: warning: implicit conversion loses integer precision: 'unsigned int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion]
        cc->version_min = version_min;
                        ~ ^~~~~~~~~~~
bearssl/inc/bearssl_ssl.h:1254:20: warning: implicit conversion loses integer precision: 'unsigned int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion]
        cc->version_max = version_max;
                        ~ ^~~~~~~~~~~
bearssl/inc/bearssl_ssl.h:1327:28: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion]
        ctx->protocol_names_num = num;
                                ~ ^~~
5 warnings generated.

(These warnings were fixed in BearSSL Git master in 2019 via
2893441f2efd4603ddd6d7f49011bdda096a4a87 and
ecdf89770ee82dfea6186fb4369cff3d06cd852e.)

Also these two cases, which are caused by an unidentified component
(outside curl) cranking up MSVC warnings in external headers to /W4
when ZLIB is deselected:
#14859 (comment)

mbedTLS 3.6.1:

C:\vcpkg\installed\x64-windows\include\psa\crypto_struct.h(254,13): error C2220: the following warning is treated as an error [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
C:\vcpkg\installed\x64-windows\include\psa\crypto_struct.h(254,13): warning C4200: nonstandard extension used: zero-sized array in struct/union [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')

Ref: https://github.com/curl/curl/actions/runs/10842694205/job/30107466989?pr=14859#step:10:29

nghttp3 1.5.0:

C:\vcpkg\installed\x64-windows\include\nghttp3\nghttp3.h(2678,1): error C2220: the following warning is treated as an error [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
  C:\vcpkg\installed\x64-windows\include\nghttp3\nghttp3.h(2678,1): warning C4324: 'nghttp3_pri': structure was padded due to alignment specifier [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')

Ref: https://github.com/curl/curl/actions/runs/10871875297/job/30166233862?pr=14859#step:10:28

TODO:

swap C-level fix with CMake's SYSTEM keyword.
swap -I to -isystem in Makefile.mk for mbedTLS.
delete BearSSL and mbedTLS C-level hacks for this.
prepend SYSTEM to all include_directories() arg list.
swap -I to -isystem in Makefile.mk for the rest of dependencies.

bagder · 2024-09-03T08:08:09Z

I'm curious: when do you see these warnings? Why do we get those warnings for bearssl headers? If we -isystem them doesn't that fix this?

vszakats · 2024-09-03T09:00:17Z

It's on macOS with CommandLineTools and CMake. (mbedTLS also needed a similar hack earlier.)

Yes, -isystem also silences it.

autotools has a separate manual step for that: convert -I options to -isystem.

CMake has a SYSTEM keyword for this, but it's not used in any include_directories() or target_include_directories() calls, yet:

https://cmake.org/cmake/help/latest/command/include_directories.html
https://cmake.org/cmake/help/latest/command/target_include_directories.html
https://stackoverflow.com/questions/3371127/use-isystem-instead-of-i-with-cmake

It seems like a case where the CMake-level fix would be better. But then it would be nice to apply it universally for all dependency header dirs, as done in autotools?

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -596,7 +596,7 @@ if(CURL_USE_BEARSSL)
   set(_ssl_enabled ON)
   set(USE_BEARSSL ON)
   list(APPEND CURL_LIBS ${BEARSSL_LIBRARIES})
-  include_directories(${BEARSSL_INCLUDE_DIRS})
+  include_directories(SYSTEM ${BEARSSL_INCLUDE_DIRS})
 
   if(CURL_DEFAULT_SSL_BACKEND AND CURL_DEFAULT_SSL_BACKEND STREQUAL "bearssl")
     set(_valid_default_ssl_backend TRUE)

bagder · 2024-09-03T09:07:38Z

then it would be nice to apply it universally for all dependency header dirs, as done in autotools?

I think so. Because compiler warnings in dependencies' header files are always just a nuisance so we are better off ignoring problems for those.

vszakats · 2024-09-03T09:22:17Z

I'm thinking to apply this for all dependencies.

Is there a risk applying this to all include_directories() and -I options in lib/Makefile.mk?

bagder · 2024-09-03T11:34:40Z

Is there a risk applying this to all include_directories() and -I options in lib/Makefile.mk?

The risk is that you modify the few -I instances we use to point out include dirs when building curl itself.

vszakats · 2024-09-03T12:34:14Z

Is there a risk applying this to all include_directories() and -I options in lib/Makefile.mk?

The risk is that you modify the few -I instances we use to point out include dirs when building curl itself.

This seems well-separated in CMake. I can't see how it could happen. (Though reviews are welcome! :)

(Unless a dependency header dir coincides with one of curl's own source dirs, e.g. by installing one straight into the curl sandbox, or mis-detected as installed there. But that's messed up, and likely risks autotools the same way.)

Seen with BearSSL 0.6 (2018-08-14) (latest tarball) and Apple clang 14. It was fixed in BearSSL Git master in 2019 via 2893441f2efd4603ddd6d7f49011bdda096a4a87 and ecdf89770ee82dfea6186fb4369cff3d06cd852e. ``` In file included from .../curl/lib/vtls/bearssl.c:28: In file included from .../curl/bearssl/inc/bearssl.h:127: .../curl/bearssl/inc/bearssl_hash.h:727:5: warning: 'BR_DOXYGEN_IGNORE' is not defined, evaluates to 0 [-Wundef] ^ .../curl/bearssl/inc/bearssl_hash.h:745:5: warning: 'BR_DOXYGEN_IGNORE' is not defined, evaluates to 0 [-Wundef] ^ In file included from .../curl/lib/vtls/bearssl.c:28: In file included from .../curl/bearssl/inc/bearssl.h:136: .../curl/bearssl/inc/bearssl_ssl.h:1253:20: warning: implicit conversion loses integer precision: 'unsigned int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] cc->version_min = version_min; ~ ^~~~~~~~~~~ .../curl/bearssl/inc/bearssl_ssl.h:1254:20: warning: implicit conversion loses integer precision: 'unsigned int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] cc->version_max = version_max; ~ ^~~~~~~~~~~ .../curl/bearssl/inc/bearssl_ssl.h:1327:28: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] ctx->protocol_names_num = num; ~ ^~~ 5 warnings generated. ```

We set this macro to silence a warning inside `openldap.h`. With this warning now silenced by using `-isystem`, we can drop it. Also it never had to be set to `1`. Also enable OpenLDAP in a CMake GHA/macos job. Follow-up to 445fb81 #14763 Follow-up to 751e168 #12024 Closes #16146

… issues Patch started out for working around compiler warnings in BearSSL latest tarball release v0.6 (2018-08-14) and Apple clang 14 with CMake. Then turned into patching CMake and `Makefile.mk` builds to use `-isystem` instead `-I` when adding header directories for dependencies. This avoids compiler warnings in dependency headers, syncing behaviour with autotools. Also: - `Makefile.mk`: add support for BearSSL. - delete warning suppression for mbedTLS headers. No longer necessary after this patch. Follow-up to 434db99 curl#12720 Silenced BearSSL warnings: ``` In file included from curl/lib/vtls/bearssl.c:28: In file included from bearssl/inc/bearssl.h:127: bearssl/inc/bearssl_hash.h:727:5: warning: 'BR_DOXYGEN_IGNORE' is not defined, evaluates to 0 [-Wundef] ^ bearssl/inc/bearssl_hash.h:745:5: warning: 'BR_DOXYGEN_IGNORE' is not defined, evaluates to 0 [-Wundef] ^ In file included from curl/lib/vtls/bearssl.c:28: In file included from bearssl/inc/bearssl.h:136: bearssl/inc/bearssl_ssl.h:1253:20: warning: implicit conversion loses integer precision: 'unsigned int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] cc->version_min = version_min; ~ ^~~~~~~~~~~ bearssl/inc/bearssl_ssl.h:1254:20: warning: implicit conversion loses integer precision: 'unsigned int' to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] cc->version_max = version_max; ~ ^~~~~~~~~~~ bearssl/inc/bearssl_ssl.h:1327:28: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'uint16_t' (aka 'unsigned short') [-Wimplicit-int-conversion] ctx->protocol_names_num = num; ~ ^~~ 5 warnings generated. ``` (These warnings were fixed in BearSSL Git master in 2019 via 2893441f2efd4603ddd6d7f49011bdda096a4a87 and ecdf89770ee82dfea6186fb4369cff3d06cd852e.) Also these two cases, which are caused by an unidentified component (outside curl) cranking up MSVC warnings in external headers to `/W4` when ZLIB is deselected: curl#14859 (comment) mbedTLS 3.6.1: ``` C:\vcpkg\installed\x64-windows\include\psa\crypto_struct.h(254,13): error C2220: the following warning is treated as an error [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj] (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c') C:\vcpkg\installed\x64-windows\include\psa\crypto_struct.h(254,13): warning C4200: nonstandard extension used: zero-sized array in struct/union [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj] (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c') ``` Ref: https://github.com/curl/curl/actions/runs/10842694205/job/30107466989?pr=14859#step:10:29 nghttp3 1.5.0: ``` C:\vcpkg\installed\x64-windows\include\nghttp3\nghttp3.h(2678,1): error C2220: the following warning is treated as an error [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj] (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c') C:\vcpkg\installed\x64-windows\include\nghttp3\nghttp3.h(2678,1): warning C4324: 'nghttp3_pri': structure was padded due to alignment specifier [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj] (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c') ``` Ref: https://github.com/curl/curl/actions/runs/10871875297/job/30166233862?pr=14859#step:10:28 Closes curl#14763

We set this macro to silence a warning inside `openldap.h`. With this warning now silenced by using `-isystem`, we can drop it. Also it never had to be set to `1`. Also enable OpenLDAP in a CMake GHA/macos job. Follow-up to 445fb81 curl#14763 Follow-up to 751e168 curl#12024 Closes curl#16146

vszakats added build TLS labels Sep 2, 2024

vszakats changed the title ~~bearssl: silence compiler warnings within external headers~~ bearssl: silence compiler warnings in external headers Sep 2, 2024

vszakats changed the title ~~bearssl: silence compiler warnings in external headers~~ bearssl: silence compiler warnings in dependency headers Sep 2, 2024

vszakats changed the title ~~bearssl: silence compiler warnings in dependency headers~~ bearssl: silence compiler warnings in bearssl headers Sep 2, 2024

vszakats changed the title ~~bearssl: silence compiler warnings in bearssl headers~~ vtls: silence compiler warnings in bearssl headers Sep 2, 2024

vszakats changed the title ~~vtls: silence compiler warnings in bearssl headers~~ vtls: silence compiler warnings in BearSSL headers Sep 2, 2024

vszakats changed the title ~~vtls: silence compiler warnings in BearSSL headers~~ bearssl: silence compiler warnings in external BearSSL headers Sep 2, 2024

vszakats added cmake and removed cmake labels Sep 3, 2024

vszakats changed the title ~~bearssl: silence compiler warnings in external BearSSL headers~~ cmake, Makefile.mk: use -isystem for dependency headers, and silence BearSSL issues Sep 3, 2024

vszakats changed the title ~~cmake, Makefile.mk: use -isystem for dependency headers, and silence BearSSL issues~~ cmake, Makefile.mk: use -isystem for dep headers, silence BearSSL issues Sep 3, 2024

vszakats mentioned this pull request Sep 13, 2024

CI: move Azure jobs to GHA, fix fallouts, sshserver, runtests tweaks #14859

Closed

13 tasks

vszakats force-pushed the bearssl-warn branch from 3bf8598 to 6243467 Compare September 19, 2024 14:17

vszakats added 6 commits September 19, 2024 17:50

drop C-level hacks

f958a94

Makefile.mk: use -isystem for mbedtls

414bd5c

cmake: use -isystem for bearssl and mbedtls

476651e

Makefile.mk: add support for bearssl

ed609a5

cmake: use SYSTEM for all deps header dirs

442326e

Makefile.mk: use -isystem for all deps header dirs

e8f1d08

vszakats force-pushed the bearssl-warn branch from 6243467 to e8f1d08 Compare September 19, 2024 15:50

vszakats closed this in 445fb81 Sep 19, 2024

vszakats deleted the bearssl-warn branch September 19, 2024 17:29

vszakats mentioned this pull request Feb 1, 2025

cmake: drop LDAP_DEPRECATED=1 macro, to sync with autotools #16146

Closed

vszakats mentioned this pull request Apr 23, 2025

-Wcast-qual with BoringSSL/AWS-LC emits warnings #17126

Closed

vszakats mentioned this pull request Sep 23, 2025

libssh2, libuv warnings on master #18683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmake, `Makefile.mk`: use `-isystem` for dep headers, silence BearSSL issues#14763

cmake, `Makefile.mk`: use `-isystem` for dep headers, silence BearSSL issues#14763
vszakats wants to merge 7 commits intocurl:masterfrom
vszakats:bearssl-warn

vszakats commented Sep 2, 2024 •

edited

Loading

Uh oh!

bagder commented Sep 3, 2024 •

edited

Loading

Uh oh!

vszakats commented Sep 3, 2024 •

edited

Loading

Uh oh!

bagder commented Sep 3, 2024 •

edited

Loading

Uh oh!

vszakats commented Sep 3, 2024

Uh oh!

bagder commented Sep 3, 2024

Uh oh!

vszakats commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vszakats commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagder commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagder commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented Sep 3, 2024

Uh oh!

bagder commented Sep 3, 2024

Uh oh!

vszakats commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

vszakats commented Sep 2, 2024 •

edited

Loading

bagder commented Sep 3, 2024 •

edited

Loading

vszakats commented Sep 3, 2024 •

edited

Loading

bagder commented Sep 3, 2024 •

edited

Loading