| Message ID | 20200219045423.54190-4-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Silence some instances of -Wtautological-compare and enable globally | expand |
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(); >
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
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
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
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?
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
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
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
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.
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();
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(-)