Skip to content

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

Closed
vszakats wants to merge 10 commits intocurl:masterfrom
vszakats:sign-conv
Closed

build: fix some -Wsign-conversion/-Warith-conversion warnings#12492
vszakats wants to merge 10 commits intocurl:masterfrom
vszakats:sign-conv

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 9, 2023 •
edited
Loading

  • 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 •
edited
Loading

-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
see -Wsign-conversion warnings
d992af0
@vszakats
fix -Werror=arith-conversion
e936673 
and indentation

Ref: https://github.com/curl/curl/actions/runs/7148682295/job/19469980314?pr=12492#step:29:1235

```
inet_pton.c:115:35: error: conversion to 'unsigned int' from 'int' may change the sign of the result [-Werror=arith-conversion]
  115 |       unsigned int val = *tp * 10 + (unsigned int)(pch - digits);
      |                                   ^
```
Ref: https://dev.azure.com/daniel0244/curl/_build/results?buildId=18964&view=logs&jobId=fd8b539a-45be-52ee-427b-3ad3fdec5ebd&j=81bb90a1-709c-5261-a92d-4869708fd1b6&t=5cd3fffe-65e4-58bd-82d3-f0b036b20e6c
@vszakats
add DWORD casts
bb2f087
@vszakats
testutil.c: silence warnings on Windows
8e90c4f
@vszakats
fix type to avoid warning
8f77af5
@vszakats
add cast to avoid warning
62417c1
@vszakats
fix type to avoid warning
5ded400
@vszakats
fix type to avoid warning
1d1826a
@vszakats
hide warnings by default, add CURL_WARN_SIGN_CONVERSION to unhide them
b4c8662
@vszakats
unhide them for single (Linux) CI job
cace2e1
@vszakats vszakats closed this in 2dbe75b Dec 19, 2023
@vszakats vszakats deleted the sign-conv branch December 19, 2023 12:47
@vszakats vszakats mentioned this pull request Dec 19, 2023
Closed
vszakats added a commit that referenced this pull request Dec 20, 2023
@vszakats
build: fix -Wconversion/-Wsign-conversion warnings
95a882d 
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
@vszakats vszakats mentioned this pull request Jun 6, 2024
Closed
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 CI Continuous Integration cmdline tool tests

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vszakats