On Wed, Feb 19, 2020 at 6:34 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > > On Tue, 18 Feb 2020 21:54:20 -0700 > Nathan Chancellor <natechancellor@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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
