Re: [PATCH 10/10] sim/cris/m32c/sh: disable use of -Werror

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

On 2022/10/22 0:58, Andrew Burgess wrote:
> Tsukasa OI <research_trasio@irq.a4lg.com> writes:
> 
>> Hi Andrew,
>>
>> It's surprising for me that you are working on that!
> 
> While I was reviewing your previous clang patches I tried to build the
> sim/ tree with clang, but even with your patches I was hitting a bunch
> of warnings/errors.
> 
> I created a set of changes as a fixup so that I could test your patches.
> That is, this started as the set of extra fixes I needed on top of your
> work to get the sim/ tree building with clang (for me).
> 
> Once your work was merged I decided, having done this work, I might as
> well split the fixes into separate patches and get them merged.  I may
> have expanded slightly by looking at any warnings emitted by gcc as well
> as any other non-fatal warnings from clang, and fixing anything that
> looked easy.
> 
>>
>> Based on your branch, I applied one patch (from my upcoming patchset)
>> each time I met an error (until I see no errors) and here's some results
>> (including simple patches to CGEN-generated files):
>>
>> Required additional patches (from my patchset) to Andrew tree:
>>     1.  With PATCH 10/10 - Clang 15.0.3 : 16 (+2 for cpu/cris)
>>     2.  With PATCH 10/10 - Clang 15.0.0 : 19 (+2 for cpu/cris)
>>     3.  W/O  PATCH 10/10 - Clang 15.0.3 : 20 (+2 for cpu/cris)
>>     4.  W/O  PATCH 10/10 - Clang 15.0.0 : 23 (+2 for cpu/cris)
> 
> Yeah, my version of clang is somewhat older than that (9.x !)
> 
>> c.f.
>> 1.
>> <https://github.com/a4lg/binutils-gdb/tree/a4lg/snapshots/2022-10-20/andrew-tree-plus-config-1>
>> 2.
>> <https://github.com/a4lg/binutils-gdb/tree/a4lg/snapshots/2022-10-20/andrew-tree-plus-config-2>
>> 3.
>> <https://github.com/a4lg/binutils-gdb/tree/a4lg/snapshots/2022-10-20/andrew-tree-plus-config-3>
>> 4.
>> <https://github.com/a4lg/binutils-gdb/tree/a4lg/snapshots/2022-10-20/andrew-tree-plus-config-4>
>>
>> Environment:
>> -   Ubuntu 22.04.1 LTS
>> -   LLVM + Clang 15.0.0 (built from git source; primary)
>> -   LLVM + Clang 15.0.3 (built from git source; secondary)
>>
>> And... my tree (alone with my changes) failed on Clang 15.0.0 but
>> succeeded on Clang 15.0.1.  That is because
>> -Wimplicit-function-declaration was default-error on 15.0.0 but
>> downgraded to default-warning on 15.0.1.  All error messages generated
>> by Clang 15.0.0 makes sense so I'll continue using Clang 15.0.0 as a
>> reference.
>>
>> c.f.
>> <https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213>
>>
>> Thanks to your PATCH 09/10, I could build my working tree with Clang
>> except M32R simulator with Clang 15.0.0.  Not only that, I could figure
>> out how to make M32R simulator (sort of) work.  For CRIS and SuperH, I
>> can provide cleaner solution without disabling -Werror (at least on my
>> environment).
> 
> Yes please.  The disable -Werror isn't a final resting place, its more
> just a stopping point so we can have something that builds - though I
> think what you're saying above is that with later versions of clang
> things still don't build, which is a shame.
> 
>>
>> My Current Tree (to be submitted as a RFC PATCH) is available at:
>> <https://github.com/a4lg/binutils-gdb/tree/clang-nowarn-1>
>>
>> My initial plan was to split it to smaller patchsets and submit each
>> ones periodically but... it seems submitting all at once seems syncing /
>> reviewing my and your work easier.  I'll submit a RFC patchset
>> consisting of 40 or 41 patches in total.
> 
> It's not clear to me if any of my patches above conflict significantly
> with patches you have.
> 
> If there are any of the above that are a problem, then just let me know
> and I wont merge them.
> 
> I don't have any plans to do anything more on this in the immediate
> future, like I said, with the tools I have to hand right now the sim
> tree now builds fine.
> 
> I have added revisiting cris/m32c/sh to my todo list, but I doubt that
> will get looked at before 2023, and it sounds like you might have a
> solution before then - which would be nice :)
> 
> Ideally, I'd like to push any of the above patches that don't actively
> make your life harder, I'll drop anything that is a problem for you.
> And then look forward to your incoming fixes.
> 
> Let me know,
> 
> Thanks,
> Andrew

c.f.: my related patchset
<https://sourceware.org/pipermail/gdb-patches/2022-October/192853.html>


Well... I just submitted my draft RFC PATCH (misnamed [PATCH] though) to
sync and discuss with your current/future work.  I knew it will take
some time (**did** take some time) to investigate Clang issues and I
don't want I and you to avoid redoing each other's work.

I almost support your patchset (as I describe below).

TL;DR:
I support merging your ten patches except PATCH 04,07,10/10.  Although I
personally don't like to write a patch like PATCH 10/10, it might be a
good short-term solution for now (I don't object like PATCH 04,07/10).

Each review to your patchset is as follows:
    [+] : Positive
    [-] : Negative

PATCH 01/10: [+]
    Corresponds to my PATCH 36/40.  I'll withdraw my one.
PATCH 02/10: [+]
    Corresponds to my PATCH 30/40.  I'll withdraw my one.
PATCH 03/10: [+]
    Corresponds to my PATCH 28/40.  Although my PATCH 28/40 is a bit
    simpler, your patch is easy to understand and I feel this is OK
    to me.
PATCH 04/10: [-]
    Corresponds to my PATCH 31/40.  I prefer my solution (to
    initialize help with {0}).
PATCH 05/10: [+]
    Corresponds (but very different) to my PATCH 27/40.
    I chose to keep the semantics as in the original source code but
    it seems it keeps the bug in this place.  Andrew's fixes that bug
    (I think) and I prefer Andrew's one.
PATCH 06/10: [+]
    Corresponds to my PATCH 03/40.  I'll withdraw my one.
PATCH 07/10: [-]
    Corresponds to my PATCH 34/40.  Although this function is currently
    unused, I prefer to keep this function (with ATTRIBUTE_UNUSED)
    for now.
PATCH 08/10: [+]
    Corresponds to my PATCH 15/40.  I chose to add dummy addition
    (+ 0x0) and you chose just remove the self-assignments.
    Both looks equally good and it's okay to withdraw my one.
PATCH 09/10: [+]
    Copied to my working branch (as PATCH 16/40).
    Needless to say, this PATCH 16/40 is yours.
PATCH 10/10: [neutral]
    Personally I don't like this kind of resolution but there's no
    strong reason to object to this.

I received quite a lot of review for bigger batch 1 and will submit v2
after resolving some of this (and removing changes corresponding to your
patches except PATCH 04,07/10).

Thanks,
Tsukasa

> 
>>
>> Note that:
>>
>> 1.  There are three cpu/* changes (must be reviewed on Binutils side)
>> 2.  There are some optional changes (suppresses non-error warnings)
>> 3.  PATCH 01 is a duplicate of another patchset (1 patch in total)
>> 4.  PATCH 16 is entirely authored by Andrew (PATCH 09/10) and
>> 5.  There are other patches that are pretty much the same as Andrew's.
>>     They are coincidence because I and Andrew are trying
>>     to fix the same issue.
>>
>> Thanks,
>> Tsukasa
>>
>> On 2022/10/20 0:24, Andrew Burgess via Gdb-patches wrote:
>>> When building the cris, m32c, and sh simulators with Clang I am seeing
>>> build warnings from a few objects.  These three simulators currently
>>> build with -Werror, and so these warnings cause the build to fail.
>>>
>>> When built with gcc I don't see any warnings from these targets, so
>>> the -Werror is fine.
>>>
>>> As the warnings are not new, in this commit, I propose that we disable
>>> the use of -Werror for these three simulators.  With this done it is
>>> now possible to build the complete simulator tree using clang.
>>> ---
>>>  sim/cris/Makefile.in | 3 +++
>>>  sim/m32c/Makefile.in | 3 +++
>>>  sim/sh/Makefile.in   | 3 +++
>>>  3 files changed, 9 insertions(+)
>>>
>>> diff --git a/sim/cris/Makefile.in b/sim/cris/Makefile.in
>>> index d58aeee9363..53e485dca02 100644
>>> --- a/sim/cris/Makefile.in
>>> +++ b/sim/cris/Makefile.in
>>> @@ -41,6 +41,9 @@ SIM_EXTRA_DEPS = \
>>>  
>>>  SIM_EXTRA_CLEAN = cris-clean
>>>  
>>> +# Some modules don't build cleanly yet.
>>> +mloopv10f.o mloopv32f.o: SIM_WERROR_CFLAGS =
>>> +
>>>  ## COMMON_POST_CONFIG_FRAG
>>>  
>>>  arch = cris
>>> diff --git a/sim/m32c/Makefile.in b/sim/m32c/Makefile.in
>>> index 2436eb940f4..dd9b3aaf175 100644
>>> --- a/sim/m32c/Makefile.in
>>> +++ b/sim/m32c/Makefile.in
>>> @@ -40,4 +40,7 @@ SIM_OBJS = \
>>>  	trace.o \
>>>  	$(ENDLIST)
>>>  
>>> +# Some modules don't build cleanly yet.
>>> +mem.o: SIM_WERROR_CFLAGS =
>>> +
>>>  ## COMMON_POST_CONFIG_FRAG
>>> diff --git a/sim/sh/Makefile.in b/sim/sh/Makefile.in
>>> index fc794f30687..a496095e767 100644
>>> --- a/sim/sh/Makefile.in
>>> +++ b/sim/sh/Makefile.in
>>> @@ -24,4 +24,7 @@ SIM_OBJS = \
>>>  SIM_EXTRA_LIBS = -lm
>>>  SIM_EXTRA_DEPS = table.c code.c ppi.c
>>>  
>>> +# Some modules don't build cleanly yet.
>>> +interp.o: SIM_WERROR_CFLAGS =
>>> +
>>>  ## COMMON_POST_CONFIG_FRAG
>