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

build: fix some -Wsign-conversion/-Warith-conversion warnings #12492

Closed
wants to merge 10 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Dec 9, 2023

  • enable -Wsign-conversion warnings, but also setting them to not
    raise errors.
  • fix -Warith-conversion warnings seen in CI.
    These are triggered by -Wsign-converion and causing errors unless
    explicitly silenced. It makes more sense to fix them, there just a few
    of them.
  • fix some -Wsign-conversion warnings.
  • hide -Wsign-conversion warnings with a #pragma.
  • add macro CURL_WARN_SIGN_CONVERSION to unhide them on a per-build
    basis.
  • update a CI job to unhide them with the above macro:
    https://github.com/curl/curl/actions/workflows/linux.yml -> OpenSSL -O3

TODO:

  • re-silence these warnings before merge.
  • fix all -Wsign-conversion warnings. [FUTURE]
  • delete CURL_WARN_SIGN_CONVERSION logic and delete -Wno-error=sign-conversion options. [FUTURE]

@vszakats vszakats marked this pull request as draft December 9, 2023 02:46
@github-actions github-actions bot added the CI Continuous Integration label Dec 9, 2023
@vszakats vszakats force-pushed the sign-conv branch 2 times, most recently from c657225 to f31da30 Compare December 13, 2023 23:38
@vszakats vszakats changed the title see -Wsign-conversion build: fix some -Wsign-conversion warnings Dec 16, 2023
@vszakats vszakats force-pushed the sign-conv branch 2 times, most recently from 6e8616e to eaf8ca0 Compare December 16, 2023 15:40
@vszakats vszakats changed the title build: fix some -Wsign-conversion warnings build: fix some -Wsign-conversion/-Warith-conversion warnings Dec 16, 2023
@vszakats
Copy link
Member Author

vszakats commented Dec 16, 2023

-Wsign-conversion generates a bunch of warnings. These are made visible in this PRs CI runs.

E.g.:

510-520 unique warnings in each, probably with large overlap.

There are a few options to proceed with this:

  • Enable it with a -Wno-error exception, so that the warnings appear, but don't block. This might not play well with PowerShell in appveyor.ci, but it also has the undesired effect of polluting all logs with these warnings all the time, making actual errors difficult to spot and need searching for error: in them.
  • Enable it in cmake / autotools, then silence it globally via #pragma in curl_setup.h, with an option to unsilence. This isn't useful on the get go, but allows working on these warnings locally, or in a dedicated CI job. This allows to move around the #pragma as ideally more and more code would have these warnings fixed, making it unnecessary at one point.
  • Not enable this warning at all.

-Wsign-conversion is one of the few compiler hardening options suggested by OpenSSF:
https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++#-Wsign-conversion

@vszakats vszakats marked this pull request as ready for review December 16, 2023 23:25
@vszakats vszakats added the build label Dec 16, 2023
@vszakats vszakats closed this in 2dbe75b Dec 19, 2023
@vszakats vszakats deleted the sign-conv branch December 19, 2023 12:47
vszakats added a commit that referenced this pull request Dec 20, 2023
Fix remaining warnings in examples and tests which are not suppressed
by the pragma in `lib/curl_setup.h`.

Silence a toolchain issue causing warnings in `FD_SET()` calls with
older Cygwin/MSYS2 builds. Likely fixed on 2020-08-03 by:
https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=5717262b8ecfed0f7fab63e2c09c78991e36f9dd

Follow-up to 2dbe75b #12492

Closes #12557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

1 participant