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.

Sign up for GitHub

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

Jump to bottom

Warn on sign conversion #1703

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Warn on sign conversion #1703

merged 2 commits into from
Sep 17, 2024

Commits on Sep 17, 2024

  1. Fix a sign propagation in calculating transaction size statistics 

    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
    @ppisar
    ppisar committed Sep 17, 2024
    Configuration menu
    Copy the full SHA
    1361f69 View commit details
    Browse the repository at this point in the history

  2. build: Add -Wsign-conversion to CXXFLAGS 

    -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
    @ppisar
    ppisar committed Sep 17, 2024
    Configuration menu
    Copy the full SHA
    a6d9b7f View commit details
    Browse the repository at this point in the history