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

[Andrew Burgess <aburgess@redhat.com>]

Tsukasa OI <research_trasio@irq.a4lg.com> writes:

> 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.

We don't generally keep dead code around, it's always possible to pull
things back from git if needed.  Before I push patch #7 I thought I'd
ask why you wanted to keep this, but were OK with patch #6.

Thanks,
Andrew