Re: [PATCH v3 0/5] sim: Suppress warnings if built with Clang

[Tsukasa OI <research_trasio@irq.a4lg.com>]

On 2022/10/12 1:40, Tom de Vries wrote:
> On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
>> Hello,
>>
>> This is the version 2 patchset to suppress Clang compiler warnings
>> (causes a build failure due to default -Werror).
>>
>>
> 
> Hi,
> 
> AFAICT, this causes breakage with the build bots, see f.i.
> https://builder.sourceware.org/buildbot/#/builders/74/builds/1357 :
> ...
>   CC     pk_disklabel.o
> ../../../binutils-gdb/sim/ppc/corefile.c: In function
> ‘core_map_find_mapping’:
> ../../../binutils-gdb/sim/ppc/corefile.c:295:136: error: format ‘%x’
> expects argument of type ‘unsigned int’, but argument 4 has type ‘cpu *’
> {aka ‘struct _cpu *’} [-Werror=format=]
>   295 |     error("core_find_mapping() - access to unmaped address,
> attach a default map to handle this - addr=0x%x nr_bytes=0x%x
> processor=0x%x cia=0x%x\n",
>       |
>                                                                       ~^
>       |
>                                                                        |
>       |
> unsigned int
>   296 |           addr, nr_bytes, processor, cia);
>       |                           ~~~~~~~~~
>       |                           |
>       |                           cpu * {aka struct _cpu *}
> cc1: all warnings being treated as errors
> make[3]: *** [Makefile:144: corefile.o] Error 1
> ...
> 
> Thanks,
> - Tom

OK, I've figured out why.

First of all, ATTRIBUTE_PRINTF should be placed on the declaration
(rather than definition) whenever possible (if there's no declaration,
no need to create one) because it provides better analysis even if a
printf-like function is in the different object file.  This is opposite
from what I initially thought (note that I remembered those two words
"definition" and "declaration" incorrectly and be careful when you read
my previous e-mails and commit messages).

Second, when Andrew added ATTRIBUTE_PRINTF on the declaration, it
revealed existing broken printf-like calls and generated some errors,
even on GCC.  I'm working on the fix.

Thanks,
Tsukasa

> 
>> [Background]
>>
>> When we build Binutils and GDB with Clang, it causes a build failure
>> due to
>> warnings generated by Clang and the default -Werror configuration.
>>
>> I MOSTLY managed to make ALL ARCHITECTURE ENABLED Binutils and GDB
>> -Werror-free on Clang 15.0.0 (note that this does not necessarily mean
>> warning-free) and this patchset is a part of it (common simulator parts).
>>
>> -   It still requires -Wno-implicit-function-declaration on the
>>      LatticeMicro32 and M32R simulators.  Except them, most of
>> Binutils / GDB
>>      components can be built with Clang by default with my tree.
>> -   I noticed that the assembler is not tested enough (because just with
>>      --enable-targets=all, it builds only host assembler).
>>
>> Full Clang 15.0.0 -Werror-free branch in development is available at:
>> <https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-dev>
>>
>> Tested configuration:
>> -   Ubuntu 22.04.1 LTS (x86_64)
>> -   LLVM / Clang 15.0.0 (built from source)
>> -   Configuration examples:
>>      $srcdir/configure \
>>          --enable-targets=all \
>>          --enable-multilib --enable-ld --enable-gold --enable-nls \
>>          CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>>          CFLAGS='  -O2 -g -Wno-implicit-function-declaration' \
>>          CXXFLAGS='-O2 -g -Wno-implicit-function-declaration'
>>      $srcdir/configure \
>>          --target=riscv64-unknown-linux-gnu \
>>          --enable-multilib --enable-ld --enable-gold --enable-nls \
>>          CC=clang CXX=clang++ CCLD=clang CXXLD=clang++ \
>>          CFLAGS='-O2 -g' CXXFLAGS='-O2 -g'
>>
>>
>> [About this Patchset]
>>
>> This patchset contains four five fixes to the files under sim{,/common}.
>> Each contains minor fixes to suppress Clang / GCC warnings.
>>
>> Each change is detailed in the each patch.
>>
>>
>> [Changes: v2 -> v3]
>>
>> -   PATCH 2/5: Rewritten predicate macros from (!!x) to (x != 0).
>>      (This is based on the feedback by Andrew Burgess).
>> -   PATCH 3/5: Replaced with a patch by Andrew Burgess
>>      (slightly modified by me).
>> -   PATCH 4/5: Fix cached variable from ld_cv_* to sim_cv_*.
>> -   Removed ChangeLog from commit messages
>>      (based on the feedback [to another patchset by me] by Bruno Larsen).
>>
>>
>> [Changes: v1 -> v2]
>>
>> -   Commit messages are improved
>> -   PATCH v1 1/4 is Moved to another patchset
>> -   PATCH v1 2-4/4 -> PATCH v2 1-3/5
>> -   New: Check known getopt definition on sim (PATCH v2 4/5)
>>      This test is already performed on binutils, gas, gprof and ld and
>> this
>>      commit does the same to sim.
>> -   New: Initialize pbb_br_* by default (PATCH v2 5/5)
>>      It did not generate warnings on Clang but GCC creates some warnings
>>      (although those are not reported as errors).
>>      This is due to uninitialized pbb_br_* variables and this commit
>> fixes
>>      the issue by initializing them by a harmless value.
>>
>> Thanks,
>> Tsukasa
>>
>>
>>
>>
>> Tsukasa OI (5):
>>    sim: Remove self-assignments
>>    sim: Make WITH_{TRACE,PROFILE}-based macros bool
>>    sim: Suppress non-literal printf warning
>>    sim: Check known getopt definition existence
>>    sim: Initialize pbb_br_* by default
>>
>>   sim/common/genmloop.sh   |  8 ++++----
>>   sim/common/hw-tree.c     |  2 --
>>   sim/common/sim-hw.c      | 26 ++++++++++++++++----------
>>   sim/common/sim-profile.h | 12 ++++++------
>>   sim/common/sim-trace.h   |  4 ++--
>>   sim/config.h.in          |  3 +++
>>   sim/configure            | 32 ++++++++++++++++++++++++++++++++
>>   sim/configure.ac         | 10 ++++++++++
>>   8 files changed, 73 insertions(+), 24 deletions(-)
>>
>>
>> base-commit: a13886e2198beb78b81c59839043b021ce6df78a
>