[PATCH 0/6] Silence some instances of -Wtautological-compare and enable globally

[PATCH 0/6] Silence some instances of -Wtautological-compare and enable globally

[Nathan Chancellor]

Hi everyone,

This patch series aims to silence some instances of clang's
-Wtautological-compare that are not problematic and enable it globally
for the kernel because it has a bunch of subwarnings that can find real
bugs in the kernel such as
https://lore.kernel.org/lkml/20200116222658.5285-1-natechancellor@gmail.com/
and https://bugs.llvm.org/show_bug.cgi?id=42666, which was specifically
requested by Dmitry.

The first patch adds a macro that casts the section variables to
unsigned long (uintptr_t), which silences the warning and adds
documentation.

Patches two through four silence the warning in the places I have
noticed it across all of my builds with -Werror, including arm, arm64,
and x86_64 defconfig/allmodconfig/allyesconfig. There might still be
more lurking but those will have to be teased out over time.

Patch six finally enables the warning, while leaving one of the
subwarnings disabled because it is rather noisy and somewhat pointless
for the kernel, where core kernel code is expected to build and run with
many different configurations where variable types can be different
sizes.

A slight meta comment: This is the first treewide patchset that I have
sent. I pray I did everything right but please let me know if I did not.
I assume someone like Andrew will pick this up with acks from everyone
but let me know if there is someone better.

Cheers,
Nathan

Nathan Chancellor (6):
  asm/sections: Add COMPARE_SECTIONS macro
  kernel/extable: Wrap section comparison in sort_main_extable with
    COMPARE_SECTIONS
  tracing: Wrap section comparison in tracer_alloc_buffers with
    COMPARE_SECTIONS
  dynamic_debug: Wrap section comparison in dynamic_debug_init with
    COMPARE_SECTIONS
  mm: kmemleak: Wrap section comparison in kmemleak_init with
    COMPARE_SECTIONS
  kbuild: Enable -Wtautological-compare

 Makefile                       | 3 +--
 include/asm-generic/sections.h | 7 +++++++
 kernel/extable.c               | 3 ++-
 kernel/trace/trace.c           | 2 +-
 lib/dynamic_debug.c            | 2 +-
 mm/kmemleak.c                  | 3 ++-
 6 files changed, 14 insertions(+), 6 deletions(-)


base-commit: 02815e777db630e3c183718cab73752b48a5053e
-- 
2.25.1

[PATCH 1/6] asm/sections: Add COMPARE_SECTIONS macro

[Nathan Chancellor]

When building with clang's -Wtautological-compare, there are a few
warnings around the comparison of section boundaries, which are linker
defined symbols and just contain an address. Clang says that these
comparisons always evaluate to a constant because it thinks they are
regular arrays. This result is expected and reasonable since we just
care about its boolean value. The kernel does this to figure out how
exactly it was laid out during link time so that it can make certain
run time decisions without hard coding them via preprocessor macros.

These comparisons always evaluate the way that the kernel wants (done by
comparing a Clang built kernel to a GCC built kernel). As a result, this
warning should be silenced in these particular instances so that
-Wtautological-compare can be enabled for the kernel globally since it
brings several useful warnings within its group. In other words, by
disabling -Wtautological-compare, the kernel misses out on several
useful subwarnings that are found with existing static checkers;
catching things with the compiler at build time will make it easier to
catch issues, especially as clang starts to be integrated into CI
systems.

The warnings can be silenced by casting the linked defined symbols to
unsigned long (normally uintptr_t but the kernel typedef's uintptr_t to
unsigned long and some kernel developers prefer unsigned long) to make
them purely numeric comparisons, which will be converted to a boolean
without any warning from Clang. The casting is done within a macro so
that it can be documented why this casting happens, rather than
sprinkling random casts in the few places that this happens within the
kernel.

Link: https://github.com/ClangBuiltLinux/linux/issues/765
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 include/asm-generic/sections.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index d1779d442aa5..e1f3095a50c1 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -169,4 +169,11 @@ static inline bool is_kernel_rodata(unsigned long addr)
 	       addr < (unsigned long)__end_rodata;
 }
 
+/*
+ * Comparing section boundaries trips clang's -Wtautological-compare
+ * This silences that warning by making the comparisons purely numeric
+ */
+#define COMPARE_SECTIONS(section_one, op, section_two) \
+	((unsigned long)(section_one) op (unsigned long)(section_two))
+
 #endif /* _ASM_GENERIC_SECTIONS_H_ */
-- 
2.25.1

[PATCH 2/6] kernel/extable: Wrap section comparison in sort_main_extable with COMPARE_SECTIONS

[Nathan Chancellor]

Clang warns:

../kernel/extable.c:37:52: warning: array comparison always evaluates to
a constant [-Wtautological-compare]
        if (main_extable_sort_needed && __stop___ex_table > __start___ex_table) {
                                                          ^
1 warning generated.

These are not true arrays, they are linker defined symbols, which are
just addresses so there is not a real issue here. Use the
COMPARE_SECTIONS macro to silence this warning by casting the linker
defined symbols to unsigned long, which keeps the logic the same.

Link: https://github.com/ClangBuiltLinux/linux/issues/765
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 kernel/extable.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/extable.c b/kernel/extable.c
index a0024f27d3a1..17bf4ccb9de9 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -34,7 +34,8 @@ u32 __initdata __visible main_extable_sort_needed = 1;
 /* Sort the kernel's built-in exception table */
 void __init sort_main_extable(void)
 {
-	if (main_extable_sort_needed && __stop___ex_table > __start___ex_table) {
+	if (main_extable_sort_needed &&
+	    COMPARE_SECTIONS(__stop___ex_table, >, __start___ex_table)) {
 		pr_notice("Sorting __ex_table...\n");
 		sort_extable(__start___ex_table, __stop___ex_table);
 	}
-- 
2.25.1

[PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Nathan Chancellor]

Clang warns:

../kernel/trace/trace.c:9335:33: warning: array comparison always
evaluates to true [-Wtautological-compare]
        if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
                                       ^
1 warning generated.

These are not true arrays, they are linker defined symbols, which are
just addresses so there is not a real issue here. Use the
COMPARE_SECTIONS macro to silence this warning by casting the linker
defined symbols to unsigned long, which keeps the logic the same.

Link: https://github.com/ClangBuiltLinux/linux/issues/765
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c797a15a1fc7..e1f3b16e457b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9332,7 +9332,7 @@ __init static int tracer_alloc_buffers(void)
 		goto out_free_buffer_mask;
 
 	/* Only allocate trace_printk buffers if a trace_printk exists */
-	if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
+	if (COMPARE_SECTIONS(__stop___trace_bprintk_fmt, !=, __start___trace_bprintk_fmt))
 		/* Must be called before global_trace.buffer is allocated */
 		trace_printk_init_buffers();
 
-- 
2.25.1

[PATCH 4/6] dynamic_debug: Wrap section comparison in dynamic_debug_init with COMPARE_SECTIONS

[Nathan Chancellor]

Clang warns:

../lib/dynamic_debug.c:1016:24: warning: array comparison always
evaluates to false [-Wtautological-compare]
        if (__start___verbose == __stop___verbose) {
                              ^
1 warning generated.

These are not true arrays, they are linker defined symbols, which are
just addresses so there is not a real issue here. Use the
COMPARE_SECTIONS macro to silence this warning by casting the linker
defined symbols to unsigned long, which keeps the logic the same.

Link: https://github.com/ClangBuiltLinux/linux/issues/765
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 lib/dynamic_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index aae17d9522e5..c7350aa6e853 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1031,7 +1031,7 @@ static int __init dynamic_debug_init(void)
 	int n = 0, entries = 0, modct = 0;
 	int verbose_bytes = 0;
 
-	if (__start___verbose == __stop___verbose) {
+	if (COMPARE_SECTIONS(__start___verbose, ==, __stop___verbose)) {
 		pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
 		return 1;
 	}
-- 
2.25.1

[PATCH 5/6] mm: kmemleak: Wrap section comparison in kmemleak_init with COMPARE_SECTIONS

[Nathan Chancellor]

Clang warns:

../mm/kmemleak.c:1950:28: warning: array comparison always evaluates to
a constant [-Wtautological-compare]
        if (__start_ro_after_init < _sdata || __end_ro_after_init > _edata)
                                  ^
../mm/kmemleak.c:1950:60: warning: array comparison always evaluates to
a constant [-Wtautological-compare]
        if (__start_ro_after_init < _sdata || __end_ro_after_init > _edata)
                                                                  ^
2 warnings generated.

These are not true arrays, they are linker defined symbols, which are
just addresses so there is not a real issue here. Use the
COMPARE_SECTIONS macro to silence this warning by casting the linker
defined symbols to unsigned long, which keeps the logic the same.

Link: https://github.com/ClangBuiltLinux/linux/issues/765
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 mm/kmemleak.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index aa6832432d6a..e27655526ba7 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1952,7 +1952,8 @@ void __init kmemleak_init(void)
 	create_object((unsigned long)__bss_start, __bss_stop - __bss_start,
 		      KMEMLEAK_GREY, GFP_ATOMIC);
 	/* only register .data..ro_after_init if not within .data */
-	if (__start_ro_after_init < _sdata || __end_ro_after_init > _edata)
+	if (COMPARE_SECTIONS(__start_ro_after_init, <, _sdata) ||
+	    COMPARE_SECTIONS(__end_ro_after_init, >, _edata))
 		create_object((unsigned long)__start_ro_after_init,
 			      __end_ro_after_init - __start_ro_after_init,
 			      KMEMLEAK_GREY, GFP_ATOMIC);
-- 
2.25.1

[PATCH 6/6] kbuild: Enable -Wtautological-compare

[Nathan Chancellor]

Currently, we disable -Wtautological-compare, which in turn disables a
bunch of more specific tautological comparison warnings that are useful
for the kernel (see clang's documentation below). Now that all of the
major/noisy warnings have been fixed, enable -Wtautological-compare so
that more issues can be caught at build time.

-Wtautological-constant-out-of-range-compare is kept disabled because
there are places in the kernel where a constant or variable size can
change based on the kernel configuration; these are not fixed in a
clean/concise way and they are almost always harmless so this one
subwarning is kept disabled.

Link: https://github.com/ClangBuiltLinux/linux/issues/488
Link: http://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html#wtautological-compare
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index b954c304c479..99080c57a1cb 100644
--- a/Makefile
+++ b/Makefile
@@ -742,8 +742,7 @@ ifdef CONFIG_CC_IS_CLANG
 KBUILD_CPPFLAGS += -Qunused-arguments
 KBUILD_CFLAGS += -Wno-format-invalid-specifier
 KBUILD_CFLAGS += -Wno-gnu
-# Quiet clang warning: comparison of unsigned expression < 0 is always false
-KBUILD_CFLAGS += -Wno-tautological-compare
+KBUILD_CFLAGS += -Wno-tautological-constant-out-of-range-compare
 # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
 # source of a reference will be _MergedGlobals and not on of the whitelisted names.
 # See modpost pattern 2
-- 
2.25.1

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Steven Rostedt]

On Tue, 18 Feb 2020 21:54:20 -0700
Nathan Chancellor <natechancellor@gmail.com> wrote:

> Clang warns:
> 
> ../kernel/trace/trace.c:9335:33: warning: array comparison always
> evaluates to true [-Wtautological-compare]
>         if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
>                                        ^
> 1 warning generated.
> 
> These are not true arrays, they are linker defined symbols, which are
> just addresses so there is not a real issue here. Use the
> COMPARE_SECTIONS macro to silence this warning by casting the linker
> defined symbols to unsigned long, which keeps the logic the same.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/765
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index c797a15a1fc7..e1f3b16e457b 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -9332,7 +9332,7 @@ __init static int tracer_alloc_buffers(void)
>  		goto out_free_buffer_mask;
>  
>  	/* Only allocate trace_printk buffers if a trace_printk exists */
> -	if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> +	if (COMPARE_SECTIONS(__stop___trace_bprintk_fmt, !=, __start___trace_bprintk_fmt))

Sorry, but this is really ugly and unreadable. Please find some other
solution to fix this.

NAK-by: Steven Rostedt

-- Steve

>  		/* Must be called before global_trace.buffer is allocated */
>  		trace_printk_init_buffers();
>  

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Nick Desaulniers]

On Wed, Feb 19, 2020 at 6:34 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 18 Feb 2020 21:54:20 -0700
> Nathan Chancellor <natechancellor@gmail.com> wrote:
>
> > Clang warns:
> >
> > ../kernel/trace/trace.c:9335:33: warning: array comparison always
> > evaluates to true [-Wtautological-compare]
> >         if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> >                                        ^
> > 1 warning generated.
> >
> > These are not true arrays, they are linker defined symbols, which are
> > just addresses so there is not a real issue here. Use the
> > COMPARE_SECTIONS macro to silence this warning by casting the linker
> > defined symbols to unsigned long, which keeps the logic the same.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/765
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  kernel/trace/trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index c797a15a1fc7..e1f3b16e457b 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -9332,7 +9332,7 @@ __init static int tracer_alloc_buffers(void)
> >               goto out_free_buffer_mask;
> >
> >       /* Only allocate trace_printk buffers if a trace_printk exists */
> > -     if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> > +     if (COMPARE_SECTIONS(__stop___trace_bprintk_fmt, !=, __start___trace_bprintk_fmt))
>
> Sorry, but this is really ugly and unreadable. Please find some other
> solution to fix this.
>
> NAK-by: Steven Rostedt
>

Hey Nathan,
Thanks for the series; enabling the warning will help us find more
bugs.  Revisiting what the warning is about, I checked on this
"referring to symbols defined in linker scripts from C" pattern.  This
document [0] (by no means definitive, but I think it has a good idea)
says:

Linker symbols that represent a data address: In C code, declare the
variable as an extern variable. Then, refer to the value of the linker
symbol using the & operator. Because the variable is at a valid data
address, we know that a data pointer can represent the value.
Linker symbols for an arbitrary address: In C code, declare this as an
extern symbol. The type does not matter. If you are using GCC
extensions, declare it as "extern void".

Indeed, it seems that Clang is happier with that pattern:
https://godbolt.org/z/sW3t5W

Looking at __stop___trace_bprintk_fmt in particular:

kernel/trace/trace.h
1923:extern const char *__stop___trace_bprintk_fmt[];

(Should be `extern const void __stop___trace_bprintk_fmt;` void since
we don't access any data or function from that symbol, just compare
its address)

kernel/trace/trace_printk.c
260: start_index = __stop___trace_bprintk_fmt - __start___trace_bprintk_fmt;

(Should be `start_index = (uintptr_t)__stop___trace_bprintk_fmt -
(uintptr_t)__start___trace_bprintk_fmt;`) (storing the result in an
int worries me a little, but maybe that refactoring can be saved for
another day)

kernel/trace/trace.c
9335: if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)

(Should be `if (&__stop___trace_bprintk_fmt -
&__start___trace_bprintk_fmt)`.  That's not a significant change to
the existing code, and is quite legible IMO)

Steven, thoughts?

[0] http://downloads.ti.com/docs/esd/SPRUI03/using-linker-symbols-in-c-c-applications-slau1318080.html
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Jason Gunthorpe]

On Wed, Feb 19, 2020 at 09:44:31AM -0800, Nick Desaulniers wrote:
> On Wed, Feb 19, 2020 at 6:34 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue, 18 Feb 2020 21:54:20 -0700
> > Nathan Chancellor <natechancellor@gmail.com> wrote:
> >
> > > Clang warns:
> > >
> > > ../kernel/trace/trace.c:9335:33: warning: array comparison always
> > > evaluates to true [-Wtautological-compare]
> > >         if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> > >                                        ^
> > > 1 warning generated.
> > >
> > > These are not true arrays, they are linker defined symbols, which are
> > > just addresses so there is not a real issue here. Use the
> > > COMPARE_SECTIONS macro to silence this warning by casting the linker
> > > defined symbols to unsigned long, which keeps the logic the same.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/765
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > >  kernel/trace/trace.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > > index c797a15a1fc7..e1f3b16e457b 100644
> > > +++ b/kernel/trace/trace.c
> > > @@ -9332,7 +9332,7 @@ __init static int tracer_alloc_buffers(void)
> > >               goto out_free_buffer_mask;
> > >
> > >       /* Only allocate trace_printk buffers if a trace_printk exists */
> > > -     if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> > > +     if (COMPARE_SECTIONS(__stop___trace_bprintk_fmt, !=, __start___trace_bprintk_fmt))
> >
> > Sorry, but this is really ugly and unreadable. Please find some other
> > solution to fix this.
> >
> > NAK-by: Steven Rostedt
> >
> 
> Hey Nathan,
> Thanks for the series; enabling the warning will help us find more
> bugs.  Revisiting what the warning is about, I checked on this
> "referring to symbols defined in linker scripts from C" pattern.  This
> document [0] (by no means definitive, but I think it has a good idea)
> says:
> 
> Linker symbols that represent a data address: In C code, declare the
> variable as an extern variable. Then, refer to the value of the linker
> symbol using the & operator. Because the variable is at a valid data
> address, we know that a data pointer can represent the value.
> Linker symbols for an arbitrary address: In C code, declare this as an
> extern symbol. The type does not matter. If you are using GCC
> extensions, declare it as "extern void".
> 
> Indeed, it seems that Clang is happier with that pattern:
> https://godbolt.org/z/sW3t5W
> 
> Looking at __stop___trace_bprintk_fmt in particular:
> 
> kernel/trace/trace.h
> 1923:extern const char *__stop___trace_bprintk_fmt[];

Godbolt says clang is happy if it is written as:

  if (&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0])

Which is probably the best compromise. The type here is const char
*[], so it would be a shame to see it go.

I think this warning is specific to arrays and is designed to detect
programmer errors like:
  int a[1];
  int b[1];
  return a < b;

Where the author intended to use memcmp()

Jason

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Steven Rostedt]

On Wed, 19 Feb 2020 14:16:19 -0400
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > kernel/trace/trace.h
> > 1923:extern const char *__stop___trace_bprintk_fmt[];  
> 
> Godbolt says clang is happy if it is written as:
> 
>   if (&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0])
> 
> Which is probably the best compromise. The type here is const char
> *[], so it would be a shame to see it go.

If the above works, I'd be happy with that. As it is still readable.

-- Steve

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Nick Desaulniers]

On Wed, Feb 19, 2020 at 10:16 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Feb 19, 2020 at 09:44:31AM -0800, Nick Desaulniers wrote:
> > Hey Nathan,
> > Thanks for the series; enabling the warning will help us find more
> > bugs.  Revisiting what the warning is about, I checked on this
> > "referring to symbols defined in linker scripts from C" pattern.  This
> > document [0] (by no means definitive, but I think it has a good idea)
> > says:
> >
> > Linker symbols that represent a data address: In C code, declare the
> > variable as an extern variable. Then, refer to the value of the linker
> > symbol using the & operator. Because the variable is at a valid data
> > address, we know that a data pointer can represent the value.
> > Linker symbols for an arbitrary address: In C code, declare this as an
> > extern symbol. The type does not matter. If you are using GCC
> > extensions, declare it as "extern void".
> >
> > Indeed, it seems that Clang is happier with that pattern:
> > https://godbolt.org/z/sW3t5W
> >
> > Looking at __stop___trace_bprintk_fmt in particular:
> >
> > kernel/trace/trace.h
> > 1923:extern const char *__stop___trace_bprintk_fmt[];
>
> Godbolt says clang is happy if it is written as:
>
>   if (&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0])
>
> Which is probably the best compromise. The type here is const char
> *[], so it would be a shame to see it go.

If the "address" is never dereferenced, but only used for arithmetic
(in a way that the the pointed to type is irrelevant), does the
pointed to type matter?  I don't feel strongly either way, but we seem
to have found two additional possible solutions for these warnings,
which is my ultimate goal. Nathan, hopefully those are some ideas you
can work with to address the additional cases throughout the kernel?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Nathan Chancellor]

On Wed, Feb 19, 2020 at 11:11:19AM -0800, Nick Desaulniers wrote:
> On Wed, Feb 19, 2020 at 10:16 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Wed, Feb 19, 2020 at 09:44:31AM -0800, Nick Desaulniers wrote:
> > > Hey Nathan,
> > > Thanks for the series; enabling the warning will help us find more
> > > bugs.  Revisiting what the warning is about, I checked on this
> > > "referring to symbols defined in linker scripts from C" pattern.  This
> > > document [0] (by no means definitive, but I think it has a good idea)
> > > says:
> > >
> > > Linker symbols that represent a data address: In C code, declare the
> > > variable as an extern variable. Then, refer to the value of the linker
> > > symbol using the & operator. Because the variable is at a valid data
> > > address, we know that a data pointer can represent the value.
> > > Linker symbols for an arbitrary address: In C code, declare this as an
> > > extern symbol. The type does not matter. If you are using GCC
> > > extensions, declare it as "extern void".
> > >
> > > Indeed, it seems that Clang is happier with that pattern:
> > > https://godbolt.org/z/sW3t5W
> > >
> > > Looking at __stop___trace_bprintk_fmt in particular:
> > >
> > > kernel/trace/trace.h
> > > 1923:extern const char *__stop___trace_bprintk_fmt[];
> >
> > Godbolt says clang is happy if it is written as:
> >
> >   if (&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0])
> >
> > Which is probably the best compromise. The type here is const char
> > *[], so it would be a shame to see it go.
> 
> If the "address" is never dereferenced, but only used for arithmetic
> (in a way that the the pointed to type is irrelevant), does the
> pointed to type matter?  I don't feel strongly either way, but we seem
> to have found two additional possible solutions for these warnings,
> which is my ultimate goal. Nathan, hopefully those are some ideas you
> can work with to address the additional cases throughout the kernel?
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Yes, thank you for the analysis and further discussion! I have done some
rudimentary printk debugging in QEMU and it looks like these are produce
the same value:

__stop___trace_bprintk_fmt
&__stop___trace_bprintk_fmt
&__start___trace_bprintk_fmt[0]

as well as

__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt
&__stop___trace_bprintk_fmt != &__start___trace_bprintk_fmt
&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0]

I'll use the second one once I confirm this is true in all callspots
with both Clang and GCC, since it looks cleaner. Let me know if there
are any objections to that.

Cheers,
Nathan

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Steven Rostedt]

On Wed, 19 Feb 2020 12:22:49 -0700
Nathan Chancellor <natechancellor@gmail.com> wrote:

> Yes, thank you for the analysis and further discussion! I have done some
> rudimentary printk debugging in QEMU and it looks like these are produce
> the same value:
> 
> __stop___trace_bprintk_fmt
> &__stop___trace_bprintk_fmt
> &__start___trace_bprintk_fmt[0]
> 
> as well as
> 
> __stop___trace_bprintk_fmt != __start___trace_bprintk_fmt
> &__stop___trace_bprintk_fmt != &__start___trace_bprintk_fmt
> &__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0]
> 
> I'll use the second one once I confirm this is true in all callspots
> with both Clang and GCC, since it looks cleaner. Let me know if there
> are any objections to that.

Myself and I'm sure others would be fine with this approach as it is
still readable. I was just against the encapsulating the logic in a
strange macro that killed readability.

I haven't looked at the resulting assembly from these, and will
currently take your word for it ;-)  Of course, I will thoroughly test
any patches to this code to make sure it does not hurt functionality.

-- Steve

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Jason Gunthorpe]

On Wed, Feb 19, 2020 at 11:11:19AM -0800, Nick Desaulniers wrote:
> > Godbolt says clang is happy if it is written as:
> >
> >   if (&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0])
> >
> > Which is probably the best compromise. The type here is const char
> > *[], so it would be a shame to see it go.
> 
> If the "address" is never dereferenced, but only used for arithmetic
> (in a way that the the pointed to type is irrelevant), does the
> pointed to type matter? 

The type is used here:

        if (*pos < start_index)
                return __start___trace_bprintk_fmt + *pos;

The return expression should be a const char **

Presumably the caller of find_next derferences it.

Jason

Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE_SECTIONS

[Nick Desaulniers]

On Wed, Feb 19, 2020 at 11:54 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Feb 19, 2020 at 11:11:19AM -0800, Nick Desaulniers wrote:
> > > Godbolt says clang is happy if it is written as:
> > >
> > >   if (&__stop___trace_bprintk_fmt[0] != &__start___trace_bprintk_fmt[0])
> > >
> > > Which is probably the best compromise. The type here is const char
> > > *[], so it would be a shame to see it go.
> >
> > If the "address" is never dereferenced, but only used for arithmetic
> > (in a way that the the pointed to type is irrelevant), does the
> > pointed to type matter?
>
> The type is used here:
>
>         if (*pos < start_index)
>                 return __start___trace_bprintk_fmt + *pos;
>
> The return expression should be a const char **
>
> Presumably the caller of find_next derferences it.
>
> Jason

And the callers of find_next just return the return value from
find_next, but suddenly as `void*` (find_next()'s return type is
`char**`).  So it doesn't seem like the pointed to type matters, hence
the recommendation of `void` and then address-of (&) used in
comparison+arithmetic.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/6] Silence some instances of -Wtautological-compare and enable globally

[Andrew Morton]

On Tue, 18 Feb 2020 21:54:17 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:

> Hi everyone,
> 
> This patch series aims to silence some instances of clang's
> -Wtautological-compare that are not problematic and enable it globally
> for the kernel because it has a bunch of subwarnings that can find real
> bugs in the kernel such as
> https://lore.kernel.org/lkml/20200116222658.5285-1-natechancellor@gmail.com/
> and https://bugs.llvm.org/show_bug.cgi?id=42666, which was specifically
> requested by Dmitry.
> 
> The first patch adds a macro that casts the section variables to
> unsigned long (uintptr_t), which silences the warning and adds
> documentation.
> 
> Patches two through four silence the warning in the places I have
> noticed it across all of my builds with -Werror, including arm, arm64,
> and x86_64 defconfig/allmodconfig/allyesconfig. There might still be
> more lurking but those will have to be teased out over time.
> 
> Patch six finally enables the warning, while leaving one of the
> subwarnings disabled because it is rather noisy and somewhat pointless
> for the kernel, where core kernel code is expected to build and run with
> many different configurations where variable types can be different
> sizes.
> 

For some reason none of these patches apply.  Not sure why - prehaps
something in the diff headers.

Anyway, the kmemleak.c code has recently changed in ways which impact
these patches.  Please take a look at that, redo, retest and resend?

Re: [PATCH 0/6] Silence some instances of -Wtautological-compare and enable globally

[Nathan Chancellor]

Hi Andrew,

On Mon, Apr 20, 2020 at 09:03:32PM -0700, Andrew Morton wrote:
> On Tue, 18 Feb 2020 21:54:17 -0700 Nathan Chancellor <natechancellor@gmail.com> wrote:
> 
> > Hi everyone,
> > 
> > This patch series aims to silence some instances of clang's
> > -Wtautological-compare that are not problematic and enable it globally
> > for the kernel because it has a bunch of subwarnings that can find real
> > bugs in the kernel such as
> > https://lore.kernel.org/lkml/20200116222658.5285-1-natechancellor@gmail.com/
> > and https://bugs.llvm.org/show_bug.cgi?id=42666, which was specifically
> > requested by Dmitry.
> > 
> > The first patch adds a macro that casts the section variables to
> > unsigned long (uintptr_t), which silences the warning and adds
> > documentation.
> > 
> > Patches two through four silence the warning in the places I have
> > noticed it across all of my builds with -Werror, including arm, arm64,
> > and x86_64 defconfig/allmodconfig/allyesconfig. There might still be
> > more lurking but those will have to be teased out over time.
> > 
> > Patch six finally enables the warning, while leaving one of the
> > subwarnings disabled because it is rather noisy and somewhat pointless
> > for the kernel, where core kernel code is expected to build and run with
> > many different configurations where variable types can be different
> > sizes.
> > 
> 
> For some reason none of these patches apply.  Not sure why - prehaps
> something in the diff headers.
> 
> Anyway, the kmemleak.c code has recently changed in ways which impact
> these patches.  Please take a look at that, redo, retest and resend?
> 
> 

Thank you for doubling back around to this but we are good here. These
warnings have all been fixed in Linus' tree without the need for the
first patch in the series.

For those curious:

63174f61dfaef ("kernel/extable.c: use address-of operator on section symbols")
bf2cbe044da27 ("tracing: Use address-of operator on section symbols")
8306b057a85ec ("lib/dynamic_debug.c: use address-of operator on section symbols")
b0d14fc43d392 ("mm/kmemleak.c: use address-of operator on section symbols")
afe956c577b2d ("kbuild: Enable -Wtautological-compare")

Cheers,
Nathan