Warn on sign conversion #1703
Merged
Warn on sign conversion #1703
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch fixes bugs in sign propagation disovered with
-Wsign-conversion compilar flag and related integer overflows in
dnf5::print_transaction_size_stats(), like this one:
dnf5/main.cpp: In function ‘void dnf5::print_transaction_size_stats(Context&)’:
dnf5/main.cpp:785:29: error: conversion to ‘long long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Werror=sign-conversion]
785 | in_pkgs_size += pkg_size;
| ^~~~~~~~
There were more ways of fixing, but I choose this one:
Use unsigned integers because an integer overflow for signed types is
undefined in C++. I choose unsigned long long int to follow what
get_download_size() and get_install_size() returns. Otherwise, we would
have to add another check whether the value fits into e.g. uint64_t.
Unless we chose uintmax_t.
Explicitly type-cast counters, with a prior range check, on passing
them to libdnf5::cli::utils::units::to_size(). Otherwise, we would
have to add to_size() for unsigned integers to the public API.
(Actuallly to_size() should have been for unsigned integeres from the
very beginning. Package sizes cannot be negative.)
Not printing the transaction size statistics if an irrepresentible
value occurs. Adding more branches to the already complicated code for
the very improbably corner cases does not make much sense.
(I tried also a different implemeation with keeping int64_t as the
counters, but the code was longer and uglier.)
Related: rpm-software-management#1701
-Wsign-conversion is by default enabled in Clang with -Wconversion,
but disabled in GCC. Unintented type coercion can misinterpret the
sign as in:
libdnf5-cli/output/transaction_table.cpp:181:97: error: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
181 | P_(" Skipping: {:4} package\n", " Skipping: {:4} packages\n", skips), skips)
| ^~~~~
This would be missed when compiling with GCC, but causing a build
failure with Clang.
This patch brings GCC and Clang compilers in line, both to enable
-Wsign-conversion. However, it disables the warning for SWIG-generated
bindings as the generated code violates the warning many times, like
here for Perl:
bindings/perl5/libdnf5/CMakeFiles/perl5_advisory.dir/advisoryPERL_wrap.cxx:700:36: error: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Werror=sign-conversion]
700 | if (strlen(name) + 1 > (bsz - (r - buff))) return 0;
| ~~~^~~~~~~
Related: rpm-software-management#1701
jrohel
reviewed
Sep 17, 2024
| } | ||
| } | ||
|
|
||
| if (in_pkgs_size != 0) { | ||
| const auto [in_pkgs_size_value, in_pkgs_size_unit] = libdnf5::cli::utils::units::to_size(in_pkgs_size); | ||
| if (in_pkgs_size != 0 && std::in_range<int64_t>(in_pkgs_size) && std::in_range<int64_t>(download_pkgs_size)) { |
There was a problem hiding this comment.
std::in_range<int64_t>(in_pkgs_size) - Maybe a bit unnecessary test. The transaction will not exceed 8 exabytes in the near future. And if it does, I suspect there will be more serious problems on other places. :-)
|
Thank you for the PR. |
Merged
via the queue into
rpm-software-management:main
with commit Sep 17, 2024
76fef26
14 of 20 checks passed
evan-goode
added a commit
to evan-goode/dnf5
that referenced
this pull request
Sep 20, 2024
Fixes i686 build. Needed since rpm-software-management#1703.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 20, 2024
Fixes i686 build. Needed since #1703.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch set adds -Wsign-conversion to compiler flags and fixes the code be compilable with it. It should prevent from surprises like in #1701.