Re: [PATCH] linux/find: ignore -Wtype-limits to reduce W=2 warnings by 34% tree-wide
On Wed. 27 Apr 2022 at 11:58, Vincent MAILHOL
wrote:
> + Alexander Lobakin
> On Wed. 27 Apr 2022 at 05:42, Yury Norov wrote:
> > + gcc@gcc.gnu.org
> > + Rikard Falkeborn
> >
> > On Wed, Apr 27, 2022 at 01:16:58AM +0900, Vincent Mailhol wrote:
> > > find_first_bit(), find_first_and_bit(), find_first_and_bit() and
> > > find_first_and_bit() all invokes GENMASK(size - 1, 0).
> > >
> > > This triggers below warning when compiled with W=2.
> > >
> > > | ./include/linux/find.h: In function 'find_first_bit':
> > > | ./include/linux/bits.h:25:36: warning: comparison of unsigned
> > > | expression in '< 0' is always false [-Wtype-limits]
> > > |25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > | |^
> > > | ./include/linux/build_bug.h:16:62: note: in definition of macro
> > > | 'BUILD_BUG_ON_ZERO'
> > > |16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct {
> > > int:(-!!(e)); })))
> > > | | ^
> > > | ./include/linux/bits.h:25:17: note: in expansion of macro
> > > '__is_constexpr'
> > > |25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > | | ^~
> > > | ./include/linux/bits.h:38:10: note: in expansion of macro
> > > 'GENMASK_INPUT_CHECK'
> > > |38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > | | ^~~
> > > | ./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
> > > | 119 | unsigned long val = *addr & GENMASK(size - 1,
> > > 0);
> > > | | ^~~
> > >
> > > linux/find.h being a widely used header file, above warning show up in
> > > thousand of files which include this header (either directly on
> > > indirectly).
> > >
> > > Because it is a false positive, we just silence -Wtype-limits flag
> > > locally to remove the spam. clang does not warn about it, so we just
> > > apply the diag_ignore() directive to gcc.
> > >
> > > By doing so, the warnings for a W=2 build are reduced by
> > > 34%. Benchmark was done with gcc 11.2.1 on Linux v5.17 x86_64
> > > allyesconfig (except CONFIG_WERROR). Beforethe patch: 515496 warnings
> > > and after: 340097.
> > >
> > > For reference, past proposal to modify GENMASK_INPUT_CHECK() was
> > > rejected in:
> > > https://lore.kernel.org/all/20220304124416.1181029-1-mailhol.vinc...@wanadoo.fr/
> >
> > So, here is nothing wrong with the kernel code and we have an alternative
> > compiler (clang) that doesn't throw Wtype-limits. It sounds to me like an
> > internal GCC problem, and I don't understand how hiding broken Wtype-limits
> > on kernel side would help people to improve GCC.
> >
> > On the thread you mentioned above:
> >
> > > > > > Have you fixed W=1 warnings?
> > > > > > Without fixing W=1 (which makes much more sense, when used with
> > > > > > WERROR=y && COMPILE_TEST=y) this has no value.
> > > > >
> > > > > How is this connected?
> > > >
> > > > By priorities.
> > > > I don't see much value in fixing W=2 per se if the code doesn't compile
> > > > for W=1.
> > >
> > > *My code* compiles for W=1. For me, fixing this W=2 in the next in line
> > > if speaking of priorities.
> > >
> > > I do not understand why I should be forbidden to fix a W=2 in the
> > > file which I am maintaining on the grounds that some code to which
> > > I do not care still has some W=1.
> >
> > If you are concerned about a particular driver - why don't you silence
> > the warning in there? Or alternatively build it with clang?
>
> Sorry if my previous comments looked selfish. I used the first
> person to illustrate my point but because this W=2 appears in
> thousands of files my real intent is to fix it for everybody, not
> only for myself.
>
> > With all that, I think that the right way to go is to fix the root
> > cause of this churn - broken Wtype-limits in GCC, and after that move
> > Wtype-limits to W=1. Anything else looks hacky to me.
>
> Why is this use of __diag_ignore() hacky compared when compared
> to the other use of __diag_ignore() or the use of -Wno-something
> in local Makefiles?
>
> I did my due diligence and researched GCC’s buzilla before
> sending this patch. There is an opened ticket here:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86647
I would like to withdraw the above statement. After looking
deeper, this is not related to the GCC bug in the above link.
I was misled by the __is_constexpr() in GENMASK_INPUT_CHECK():
| #define GENMASK_INPUT_CHECK(h, l) \
| (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
| __is_constexpr((l) > (h)), (l) > (h), 0)))
and because of that, I was assuming that the parameters were
constant.
But actually, in the expression GENMASK(size - 1, 0), the first
member is not necessarily constant. The thing is that the warning
check occurs before the evaluation of __builtin_choose_expr() and
so, it sees the com
Updating patch regarding bypassing assembler when generating LTO object files
Hi,
I have been fiddling around with the patch regarding "bypassing assmebler
while generating slim lto files" and managed to make it build with the
current trunk. Though the patch seems to be working on Linux machine, it
causes an ICE on macOS (it crashes in langhooks.cc while executing
`lhd_begin_section ()`).
here is the patch ( not fully tested and is still a prototype ), I
basically mimicked the entire previous patch (
https://gcc.gnu.org/legacy-ml/gcc/2014-09/msg00340.html ) but on current
source.
- Ankur
---
From e5d72a73962c34dcd818c17d440eb98ddd94e624 Mon Sep 17 00:00:00 2001
From: Ankur Saini
Date: Sun, 8 May 2022 07:03:51 +0530
Subject: [PATCH] bypass-asm prototype: 1
---
gcc/Makefile.in | 1 +
gcc/common.opt | 3 +++
gcc/langhooks.cc| 29 +++-
gcc/{lto => }/lto-object.cc | 29 +---
gcc/lto-streamer.h | 35 ++
gcc/lto/Make-lang.in| 4 ++--
gcc/lto/lto-common.cc | 3 ++-
gcc/lto/lto-lang.cc | 1 +
gcc/lto/lto.h | 38 -
gcc/toplev.cc | 19 ---
10 files changed, 110 insertions(+), 52 deletions(-)
rename gcc/{lto => }/lto-object.cc (94%)
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 31ff95500c9..3d1241d5819 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1509,6 +1509,7 @@ OBJS = \
lto-section-out.o \
lto-opts.o \
lto-compress.o \
+ lto-object.o \
mcf.o \
mode-switching.o \
modulo-sched.o \
diff --git a/gcc/common.opt b/gcc/common.opt
index 8a0dafc522d..6348197380d 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1158,6 +1158,9 @@ fbtr-bb-exclusive
Common Ignore
Does nothing. Preserved for backward compatibility.
+fbypass-asm=
+Common Joined Var(flag_bypass_asm)
+
fcallgraph-info
Common RejectNegative Var(flag_callgraph_info) Init(NO_CALLGRAPH_INFO);
Output callgraph information on a per-file basis.
diff --git a/gcc/langhooks.cc b/gcc/langhooks.cc
index df970678a08..abfab7a27e0 100644
--- a/gcc/langhooks.cc
+++ b/gcc/langhooks.cc
@@ -38,6 +38,10 @@ along with GCC; see the file COPYING3. If not see
#include "stor-layout.h"
#include "cgraph.h"
#include "debug.h"
+#include "function.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "lto-streamer.h"
/* Do nothing; in many cases the default hook. */
@@ -820,6 +824,19 @@ lhd_begin_section (const char *name)
{
section *section;
+ if (flag_bypass_asm)
+{
+ static int initialized = false;
+ if (!initialized)
+ {
+ gcc_assert (asm_out_file == NULL);
+ lto_set_current_out_file (lto_obj_file_open (asm_file_name, true));
+ initialized = true;
+ }
+ lto_obj_begin_section (name);
+ return;
+}
+
/* Save the old section so we can restore it in lto_end_asm_section. */
gcc_assert (!saved_section);
saved_section = in_section;
@@ -836,8 +853,13 @@ lhd_begin_section (const char *name)
implementation just calls assemble_string. */
void
-lhd_append_data (const void *data, size_t len, void *)
+lhd_append_data (const void *data, size_t len, void *v)
{
+ if (flag_bypass_asm)
+{
+ lto_obj_append_data (data, len, v);
+ return;
+}
if (data)
{
timevar_push (TV_IPA_LTO_OUTPUT);
@@ -854,6 +876,11 @@ lhd_append_data (const void *data, size_t len, void *)
void
lhd_end_section (void)
{
+ if (flag_bypass_asm)
+{
+ lto_obj_end_section ();
+ return;
+}
if (saved_section)
{
switch_to_section (saved_section);
diff --git a/gcc/lto/lto-object.cc b/gcc/lto-object.cc
similarity index 94%
rename from gcc/lto/lto-object.cc
rename to gcc/lto-object.cc
index 329bbc71fd6..8ccc917a4eb 100644
--- a/gcc/lto/lto-object.cc
+++ b/gcc/lto-object.cc
@@ -21,9 +21,17 @@ along with GCC; see the file COPYING3. If not see
#include "config.h"
#include "system.h"
#include "coretypes.h"
-#include "tm.h"
+#include "tree.h"
+#include "basic-block.h"
+#include "tree-ssa-alias.h"
+#include "internal-fn.h"
+#include "gimple-expr.h"
+#include "is-a.h"
+#include "function.h"
+#include "gimple.h"
#include "diagnostic-core.h"
-#include "lto.h"
+#include "tm.h"
+#include "lto-streamer.h"
#include "lto-section-names.h"
#include "simple-object.h"
@@ -133,7 +141,13 @@ lto_obj_file_open (const char *filename, bool writable)
}
else
{
- gcc_assert (saved_attributes != NULL);
+ if (!saved_attributes)
+{
+ lto_file *tmp = lto_obj_file_open (flag_bypass_asm, false);
+ if (!tmp)
+goto fail;
+ lto_obj_file_close (tmp);
+}
lo->sobj_w = simple_object_start_write (saved_attributes,
LTO_SEGMENT_NAME,
&errmsg, &err);
@@ -148,7 +162,8 @@ fail_errmsg:
error ("
Re: Updating patch regarding bypassing assembler when generating LTO object files
Hi Ankur,
> On 8 May 2022, at 15:29, Ankur Saini via Gcc wrote:
>
> I have been fiddling around with the patch regarding "bypassing assmebler
> while generating slim lto files" and managed to make it build with the
> current trunk. Though the patch seems to be working on Linux machine, it
> causes an ICE on macOS (it crashes in langhooks.cc while executing
> `lhd_begin_section ()`).
This is probably because Darwin (macOS) switches to a secondary output
when emitting LTO already, you will probably need to account for that in
the changes.
see gcc/config/darwin.cc: darwin_asm_lto_start() etc.
Iain
>
> here is the patch ( not fully tested and is still a prototype ), I
> basically mimicked the entire previous patch (
> https://gcc.gnu.org/legacy-ml/gcc/2014-09/msg00340.html ) but on current
> source.
>
> - Ankur
>
> ---
>
> From e5d72a73962c34dcd818c17d440eb98ddd94e624 Mon Sep 17 00:00:00 2001
> From: Ankur Saini
> Date: Sun, 8 May 2022 07:03:51 +0530
> Subject: [PATCH] bypass-asm prototype: 1
>
> ---
> gcc/Makefile.in | 1 +
> gcc/common.opt | 3 +++
> gcc/langhooks.cc| 29 +++-
> gcc/{lto => }/lto-object.cc | 29 +---
> gcc/lto-streamer.h | 35 ++
> gcc/lto/Make-lang.in| 4 ++--
> gcc/lto/lto-common.cc | 3 ++-
> gcc/lto/lto-lang.cc | 1 +
> gcc/lto/lto.h | 38 -
> gcc/toplev.cc | 19 ---
> 10 files changed, 110 insertions(+), 52 deletions(-)
> rename gcc/{lto => }/lto-object.cc (94%)
>
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 31ff95500c9..3d1241d5819 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1509,6 +1509,7 @@ OBJS = \
> lto-section-out.o \
> lto-opts.o \
> lto-compress.o \
> + lto-object.o \
> mcf.o \
> mode-switching.o \
> modulo-sched.o \
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 8a0dafc522d..6348197380d 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1158,6 +1158,9 @@ fbtr-bb-exclusive
> Common Ignore
> Does nothing. Preserved for backward compatibility.
>
> +fbypass-asm=
> +Common Joined Var(flag_bypass_asm)
> +
> fcallgraph-info
> Common RejectNegative Var(flag_callgraph_info) Init(NO_CALLGRAPH_INFO);
> Output callgraph information on a per-file basis.
> diff --git a/gcc/langhooks.cc b/gcc/langhooks.cc
> index df970678a08..abfab7a27e0 100644
> --- a/gcc/langhooks.cc
> +++ b/gcc/langhooks.cc
> @@ -38,6 +38,10 @@ along with GCC; see the file COPYING3. If not see
> #include "stor-layout.h"
> #include "cgraph.h"
> #include "debug.h"
> +#include "function.h"
> +#include "basic-block.h"
> +#include "gimple.h"
> +#include "lto-streamer.h"
>
> /* Do nothing; in many cases the default hook. */
>
> @@ -820,6 +824,19 @@ lhd_begin_section (const char *name)
> {
> section *section;
>
> + if (flag_bypass_asm)
> +{
> + static int initialized = false;
> + if (!initialized)
> + {
> + gcc_assert (asm_out_file == NULL);
> + lto_set_current_out_file (lto_obj_file_open (asm_file_name, true));
> + initialized = true;
> + }
> + lto_obj_begin_section (name);
> + return;
> +}
> +
> /* Save the old section so we can restore it in lto_end_asm_section. */
> gcc_assert (!saved_section);
> saved_section = in_section;
> @@ -836,8 +853,13 @@ lhd_begin_section (const char *name)
>implementation just calls assemble_string. */
>
> void
> -lhd_append_data (const void *data, size_t len, void *)
> +lhd_append_data (const void *data, size_t len, void *v)
> {
> + if (flag_bypass_asm)
> +{
> + lto_obj_append_data (data, len, v);
> + return;
> +}
> if (data)
> {
> timevar_push (TV_IPA_LTO_OUTPUT);
> @@ -854,6 +876,11 @@ lhd_append_data (const void *data, size_t len, void *)
> void
> lhd_end_section (void)
> {
> + if (flag_bypass_asm)
> +{
> + lto_obj_end_section ();
> + return;
> +}
> if (saved_section)
> {
> switch_to_section (saved_section);
> diff --git a/gcc/lto/lto-object.cc b/gcc/lto-object.cc
> similarity index 94%
> rename from gcc/lto/lto-object.cc
> rename to gcc/lto-object.cc
> index 329bbc71fd6..8ccc917a4eb 100644
> --- a/gcc/lto/lto-object.cc
> +++ b/gcc/lto-object.cc
> @@ -21,9 +21,17 @@ along with GCC; see the file COPYING3. If not see
> #include "config.h"
> #include "system.h"
> #include "coretypes.h"
> -#include "tm.h"
> +#include "tree.h"
> +#include "basic-block.h"
> +#include "tree-ssa-alias.h"
> +#include "internal-fn.h"
> +#include "gimple-expr.h"
> +#include "is-a.h"
> +#include "function.h"
> +#include "gimple.h"
> #include "diagnostic-core.h"
> -#include "lto.h"
> +#include "tm.h"
> +#include "lto-streamer.h"
> #include "lto-section-names.h"
> #include "simple-object.h"
>
> @@ -133,7 +141,13 @@ lto_obj_file_open (const char *filename, bool writab
gcc-13-20220508 is now available
Snapshot gcc-13-20220508 is now available on https://gcc.gnu.org/pub/gcc/snapshots/13-20220508/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 13 git branch with the following options: git://gcc.gnu.org/git/gcc.git branch master revision a1947c92f7cda5f6cf7b8d8a9a44f6dd45352c03 You'll find: gcc-13-20220508.tar.xz Complete GCC SHA256=3a4bd7bcd147df7d1918fe028614a5c77f79afca46a00b61393509146ddc5360 SHA1=d8d030eca6ad8511c7644aa5acf8b1bfe4f80786 Diffs from 13-20220501 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-13 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
