[Bug libc/14508] -Wformat warnings

["cvs-commit at gcc dot gnu.org" <sourceware-bugzilla at sourceware dot org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=14508

--- Comment #5 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  0d40d0ecba3b1e5b8c3b8da01c708fea3948e193 (commit)
      from  383e87c96b9eee3b4b2d78dbeeaa8e2c0db35feb (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=0d40d0ecba3b1e5b8c3b8da01c708fea3948e193

commit 0d40d0ecba3b1e5b8c3b8da01c708fea3948e193
Author: Joseph Myers <joseph@codesourcery.com>
Date:   Tue Feb 6 21:55:08 2018 +0000

    Unify and simplify bits/byteswap.h, bits/byteswap-16.h headers (bug 14508,
bug 15512, bug 17082, bug 20530).

    We have a general principle of preferring optimizations for library
    facilities to use compiler built-in functions rather than being
    located in library headers, where the compiler can reasonably optimize
    code without needing to know glibc implementation details.

    This patch applies this principle to bits/byteswap.h, eliminating all
    the architecture-specific variants and bits/byteswap-16.h.  The
    __bswap_16, __bswap_32 and __bswap_64 interfaces all become inline
    functions, never macros, using the GCC built-in functions where
    available and otherwise a single architecture-independent definition
    using shifts and masking (which compilers may well be able to detect
    and optimize; GCC has detection of various byte-swapping idioms).

    The __bswap_constant_32 macro needs to stay around because of uses in
    static initializers within glibc and its tests, and so for consistency
    all __bswap_constant_* are kept rather than just being inlined into
    the old-GCC-or-non-GCC parts of the __bswap_* inline function
    definitions.

    Various open bugs are addressed by this cleanup, with caveats about
    exactly what is covered by those bugs and when the bugs applied at
    all.

    Bug 14508 reports -Wformat warnings building glibc because __bswap_*
    sometimes returned the wrong types.  Obviously we already don't have
    such warnings any more or the build would be failing, given -Werror,
    and I suspect that bug was originally for wrong types for x86_64, as
    fixed by commit d394eb742a3565d7fe7a4b02710a60b5f219ee64 (glibc 2.17).
    The only case I saw removed by this patch where the types would still
    have been wrong was the non-__GNUC__ case of __bswap_64 in the s390
    header (using unsigned long long int, but uint64_t would be unsigned
    long int for 64-bit).  In any case, the single header consistently
    uses __uintN_t types after this patch, thereby eliminating all such
    bugs.  The existing string/test-endian-types.c test already suffices
    to verify that the types are correct with the compiler used to build
    glibc and its tests.

    Bug 15512 reports an error from __bswap_constant_16 with -Werror
    -Wsign-conversion.  I am unable to reproduce this with any GCC version
    supporting -Wsign-conversion - all seem to be able to avoid warning
    for ((x) >> 8) & 0xffu, where x is uint16_t, which while it formally
    does involve an implicit conversion from int to unsigned int, is also
    a case where it should be easy for the compiler to see that the value
    converted is never negative.  But in this patch __bswap_constant_16 is
    changed to use signed 0xff so that no such implicit conversion occurs
    at all, and a test with -Werror -Wsign-conversion is added.

    Bug 17082 objects to the use of ({}) statement expressions in these
    macros preventing use at file scope (in C, that's in sizeof etc.; in
    C++, more generally in static initializers).  The particular case of
    these interfaces is fixed by this patch as it changes them to inline
    functions, eliminating all uses of ({}) in bits/byteswap.h, and a
    corresponding testcase is added.  The bug tries to raise a more
    general policy question about use of ({}) in macros in installed
    headers, referring to "many other libc functions" (unspecified which
    functions are being considered).

    Since such policy questions belong on libc-alpha, and since there
    *are* macros in installed headers which can't really avoid using ({})
    (where they are type-generic, so can't use an inline function, but
    need a temporary variable, and a few where the interface involves
    returning memory from alloca so can't use an inline function either),
    I propose to consider that bug fixed with this change.  That is
    without prejudice to any other new bugs anyone wishes to file *for
    precisely defined sets of macros* requesting moving away from ({})
    *where it is clearly possible for those interfaces*.  Where ({}) can
    be avoided, typically by use of an inline function, I think that's a
    good idea - that inline functions are typically to be preferred to
    ({}) for header interfaces where such optimizations are useful but the
    interface is suited to being defined using an inline function.

    Bug 20530 requests use of __builtin_bswap16 when available (GCC 4.8
    and later), which this patch implements.

    Tested for x86_64, and with build-many-glibcs.py.  Also did an x86_64
    test with the __GNUC_PREREQ conditionals changed to "#if 0" to verify
    the old-GCC/non-GCC case in the headers.  (There are already existing
    tests for correctness of results of these interfaces.)

        [BZ #14508]
        [BZ #15512]
        [BZ #17082]
        [BZ #20530]
        * bits/byteswap.h: Update file comment.  Do not include
        <bits/byteswap-16.h>.
        (__bswap_constant_16): Cast result to __uint16_t.  Use signed 0xff
        constant.
        (__bswap_16): Define as inline function.
        (__bswap_constant_32): Reformat definition.
        (__bswap_32): Always define as inline function, not macro, using
        __uint32_t.  Use __builtin_bswap32 if [__GNUC_PREREQ (4, 3)],
        otherwise __bswap_constant_32.
        (__bswap_constant_64): Reformat definition.  Do not use
        __extension__ here.
        (__bswap_64): Always define as inline function, not macro.  Use
        __extension__ on function definition.  Use __builtin_bswap64 if
        [__GNUC_PREREQ (4, 3)], otherwise __bswap_constant_64.
        * string/test-endian-file-scope.c: New file.
        * string/test-endian-sign-conversion.c: Likewise.
        * string/Makefile (headers): Remove bits/byteswap-16.h.
        (tests): Add test-endian-file-scope and
        test-endian-sign-conversion.
        (CFLAGS-test-endian-sign-conversion.c): New variable.
        * bits/byteswap-16.h: Remove file.
        * sysdeps/ia64/bits/byteswap-16.h: Likewise.
        * sysdeps/ia64/bits/byteswap.h: Likewise.
        * sysdeps/m68k/bits/byteswap.h: Likewise.
        * sysdeps/s390/bits/byteswap-16.h: Likewise.
        * sysdeps/s390/bits/byteswap.h: Likewise.
        * sysdeps/tile/bits/byteswap.h: Likewise.
        * sysdeps/x86/bits/byteswap-16.h: Likewise.
        * sysdeps/x86/bits/byteswap.h: Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                          |   34 +++++
 bits/byteswap-16.h                                 |   34 -----
 bits/byteswap.h                                    |  101 +++++---------
 string/Makefile                                    |    6 +-
 .../test-endian-file-scope.c                       |   31 +++--
 .../test-endian-sign-conversion.c                  |   27 +++-
 sysdeps/ia64/bits/byteswap-16.h                    |   42 ------
 sysdeps/ia64/bits/byteswap.h                       |  100 -------------
 sysdeps/m68k/bits/byteswap.h                       |   88 -----------
 sysdeps/s390/bits/byteswap-16.h                    |   65 --------
 sysdeps/s390/bits/byteswap.h                       |  134 -----------------
 sysdeps/tile/bits/byteswap.h                       |   37 -----
 sysdeps/x86/bits/byteswap-16.h                     |   49 ------
 sysdeps/x86/bits/byteswap.h                        |  155 --------------------
 14 files changed, 108 insertions(+), 795 deletions(-)
 delete mode 100644 bits/byteswap-16.h
 copy posix/test-ssize-max.c => string/test-endian-file-scope.c (63%)
 copy inet/test-hnto-types.c => string/test-endian-sign-conversion.c (65%)
 delete mode 100644 sysdeps/ia64/bits/byteswap-16.h
 delete mode 100644 sysdeps/ia64/bits/byteswap.h
 delete mode 100644 sysdeps/m68k/bits/byteswap.h
 delete mode 100644 sysdeps/s390/bits/byteswap-16.h
 delete mode 100644 sysdeps/s390/bits/byteswap.h
 delete mode 100644 sysdeps/tile/bits/byteswap.h
 delete mode 100644 sysdeps/x86/bits/byteswap-16.h
 delete mode 100644 sysdeps/x86/bits/byteswap.h

-- 
You are receiving this mail because:
You are on the CC list for the bug.