From: Sedat Dilek <sedat.dilek@gmail.com>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: linux-kbuild@vger.kernel.org,
Nick Desaulniers <ndesaulniers@google.com>,
Nathan Chancellor <natechancellor@gmail.com>,
Michal Marek <michal.lkml@markovi.net>,
Clang-Built-Linux ML <clang-built-linux@googlegroups.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kbuild: refactor scripts/Makefile.extrawarn
Date: Thu, 29 Aug 2019 23:01:39 +0200 [thread overview]
Message-ID: <CA+icZUX0WvZ9Kfvvkq2Xhgv=kEkmHi7bF8VSONKcXnJx28bKGg@mail.gmail.com> (raw)
In-Reply-To: <20190829181231.5920-1-yamada.masahiro@socionext.com>
On Thu, Aug 29, 2019 at 8:13 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Instead of the warning-[123] magic, let's accumulate compiler options
> to KBUILD_CFLAGS directly as the top Makefile does. I think this makes
> easier to understand what is going on in this file.
>
> This commit slightly changes the behavior, I think all of which are OK.
>
> [1] Currently, cc-option calls are needlessly evaluated. For example,
> warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
> needs evaluating only when W=3, but it is actually evaluated for
> W=1, W=2 as well. With this commit, only relevant cc-option calls
> will be evaluated. This is a slight optimization.
>
> [2] Currently, unsupported level like W=4 is checked by:
> $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
> This will no longer be checked, but I do not think it is a big
> deal.
>
> [3] Currently, 4 Clang warnings (Winitializer-overrides, Wformat,
> Wsign-compare, Wformat-zero-length) are shown by any of W=1, W=2,
> and W=3. With this commit, they will be warned only by W=1. I
> think this is a more correct behavior since each warning belongs
> to only one group.
>
> For understanding this commit correctly:
>
> We have 3 warning groups, W=1, W=2, and W=3. You may think W=3 has a
> higher level than W=1, but they are actually independent. If you like,
> you can combine them like W=13. To enable all the warnings, you can
> pass W=123. This is shown by 'make help', but it is often missed
> unfortunately. Since we support W= combination, there should not exist
> intersection among the three groups. If we enable Winitializer-overrides
> for W=1, we do not need to for W=2 or W=3. This is why I believe the
> change [3] makes sense.
>
> The documentation says -Winitializer-overrides is enabled by default.
> (https://clang.llvm.org/docs/DiagnosticsReference.html#winitializer-overrides)
> We negate it by passing -Wno-initializer-overrides for the normal
> build, but we do not do that for W=1. This means, W=1 effectively
> enables -Winitializer-overrides by the clang's default. The same for
> the other three. I wonder if this logic needs detailed commenting,
> but I do not want to be bothered any more. I added comments.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> Changes in v2:
> - Added comments and more commit log
>
> scripts/Makefile.extrawarn | 105 +++++++++++++++++++------------------
> 1 file changed, 54 insertions(+), 51 deletions(-)
>
Thanks for the v2.
I am impressed by the very informative commit-log.
But... I still miss relevant infos in the "kbuild-docs" aka in
<Documentation/kbuild/kbuild.rst file>
I also was not aware I can combine W=... settings like W=123 (W=132
and W=321 does the same I guess).
In my little world W=3 should include W=1 and W=2.
Such informations I would like to have in kbuild-docs.
BTW, I mixed up kbuild-system with kconfig-system as I normally get in
touch with the 2nd.
Renaming the kbuild-variable is up to you.
I am OK when you want to wait for Arnd's rework of extrawarn compiler options.
I did change it in one of my patches treewide.
- Sedat -
next prev parent reply other threads:[~2019-08-29 21:01 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-29 18:12 [PATCH v2] kbuild: refactor scripts/Makefile.extrawarn Masahiro Yamada
2019-08-29 21:01 ` Sedat Dilek [this message]
2019-08-30 6:57 ` Sedat Dilek
2019-08-30 21:02 ` Nick Desaulniers
2019-08-31 7:02 ` Miguel Ojeda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+icZUX0WvZ9Kfvvkq2Xhgv=kEkmHi7bF8VSONKcXnJx28bKGg@mail.gmail.com' \
--to=sedat.dilek@gmail.com \
--cc=clang-built-linux@googlegroups.com \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michal.lkml@markovi.net \
--cc=natechancellor@gmail.com \
--cc=ndesaulniers@google.com \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).