| Message ID | 20230309174854.350143-1-vincenzopalazzodev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] x86: suppress warning generated by W=1 | expand |
On Thu, Mar 09, 2023 at 06:48:54PM +0100, Vincenzo Palazzo wrote: > suppress unused warnings and fix the error that there is > with the W=1 enabled. Why are you building with that option enabled? It's not a normal one at all. > > Warning generated > > arch/x86/entry/common.c:238:24: error: no previous prototype for ‘do_SYSENTER_32’ [-Werror=missing-prototypes] > 238 | __visible noinstr long do_SYSENTER_32(struct pt_regs *regs) > > Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com> > --- > arch/x86/entry/common.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c > index 6c2826417b33..8f17b2c3e9de 100644 > --- a/arch/x86/entry/common.c > +++ b/arch/x86/entry/common.c > @@ -234,6 +234,9 @@ __visible noinstr long do_fast_syscall_32(struct pt_regs *regs) > #endif > } > > +/* prototype is a placeholder to suppress the missing prototype worning */ > +long do_SYSENTER_32(struct pt_regs *regs); This feels wrong, sorry, and you have a spelling error in your comment. > + > /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */ > __visible noinstr long do_SYSENTER_32(struct pt_regs *regs) It's wrong because look, you define it right here so why do you need a prototype? thanks, greg k-h
On Fri Mar 10, 2023 at 8:33 AM CET, Greg KH wrote: > On Thu, Mar 09, 2023 at 06:48:54PM +0100, Vincenzo Palazzo wrote: > > suppress unused warnings and fix the error that there is > > with the W=1 enabled. > > Why are you building with that option enabled? It's not a normal one at > all. I was using this option because I would like to see if my c code has warnings, but currently it is not possible compile the kernel with W=1. Maybe I'm missing a little bit of history here, this is not the correct option to compile the kernel with -Wall? Or it is the wrong way to compile the kernel with -Wall, so at this point another good patch if we do not want to see the warnings, is rework the W=1 to allow a subset of warnings, like the one in this patch? > > > > > Warning generated > > > > arch/x86/entry/common.c:238:24: error: no previous prototype for ‘do_SYSENTER_32’ [-Werror=missing-prototypes] > > 238 | __visible noinstr long do_SYSENTER_32(struct pt_regs *regs) > > > > Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com> > > --- > > arch/x86/entry/common.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c > > index 6c2826417b33..8f17b2c3e9de 100644 > > --- a/arch/x86/entry/common.c > > +++ b/arch/x86/entry/common.c > > @@ -234,6 +234,9 @@ __visible noinstr long do_fast_syscall_32(struct pt_regs *regs) > > #endif > > } > > > > +/* prototype is a placeholder to suppress the missing prototype worning */ > > +long do_SYSENTER_32(struct pt_regs *regs); > > This feels wrong, sorry, and you have a spelling error in your comment. Yeah it is, in fact this do not make sense because we are in a c file. However, looks like it is the only option to suppress the warnings in this case? Do you know some magic __attribute__ that can suppress it for a not static function like the one in this PATCH? > > > + > > /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */ > > __visible noinstr long do_SYSENTER_32(struct pt_regs *regs) > > It's wrong because look, you define it right here so why do you need a > prototype? Here the problem is the macros __visible that force the function to be not static because need to be visible outside, and in C as far I know a procedure without a prototype need to be static, for this reason I made the previous trick to suppress the warning. I thought also to modify the macros in some way but I did not have a good idea. P.S: Thanks to catch the typo, I will provide a v2 when we converge in a common solution. Cheers! Vincent.
On Fri, Mar 10, 2023 at 10:19:53AM +0100, Vincenzo Palazzo wrote: > On Fri Mar 10, 2023 at 8:33 AM CET, Greg KH wrote: > > On Thu, Mar 09, 2023 at 06:48:54PM +0100, Vincenzo Palazzo wrote: > > > suppress unused warnings and fix the error that there is > > > with the W=1 enabled. > > > > Why are you building with that option enabled? It's not a normal one at > > all. > > I was using this option because I would like to see if my c code has > warnings, but currently it is not possible compile the kernel with > W=1. Yes, that is the main issue here, the kernel does not build cleanly for lots of good reasons (i.e. foolish compiler warnings that are not actually pointing out a real problem), so that option is not enabled by default right now. So this change is not needed right now, perhaps you can fix up the compiler to not make this type of warning for code that is correct? > Maybe I'm missing a little bit of history here, this is not the correct > option to compile the kernel with -Wall? Yes, but the kernel does not build cleanly with that option, as you have found out. Fixes for when the kernel code is wrong is great to have, but not at the expense of "we are doing this only to shut up the compiler because it does not understand our code" like you are doing here, sorry. thanks, greg k-h
On Fri Mar 10, 2023 at 10:28 AM CET, Greg KH wrote: > On Fri, Mar 10, 2023 at 10:19:53AM +0100, Vincenzo Palazzo wrote: > > On Fri Mar 10, 2023 at 8:33 AM CET, Greg KH wrote: > > > On Thu, Mar 09, 2023 at 06:48:54PM +0100, Vincenzo Palazzo wrote: > > > > suppress unused warnings and fix the error that there is > > > > with the W=1 enabled. > > > > > > Why are you building with that option enabled? It's not a normal one at > > > all. > > > > I was using this option because I would like to see if my c code has > > warnings, but currently it is not possible compile the kernel with > > W=1. > > Yes, that is the main issue here, the kernel does not build cleanly for > lots of good reasons (i.e. foolish compiler warnings that are not > actually pointing out a real problem), so that option is not enabled by > default right now. I see, I was missing this info :) thanks! > > So this change is not needed right now, perhaps you can fix up the > compiler to not make this type of warning for code that is correct? I would happy to, but I am pretty sure that the compiler guys had a strong option to keep this option enabled. However, I could try to make an informative email to see if there is any way to fix this warning without write hacky code. > > > Maybe I'm missing a little bit of history here, this is not the correct > > option to compile the kernel with -Wall? > > Yes, but the kernel does not build cleanly with that option, as you have > found out. Fixes for when the kernel code is wrong is great to have, > but not at the expense of "we are doing this only to shut up the > compiler because it does not understand our code" like you are doing > here, sorry. Yep, I see you point and I share your idea to do not try to do this kind of suppress work just because the code do not looks like write in the standart way. > > thanks, Thanks to you for this good information. Cheers! Vincent.
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c index 6c2826417b33..8f17b2c3e9de 100644 --- a/arch/x86/entry/common.c +++ b/arch/x86/entry/common.c @@ -234,6 +234,9 @@ __visible noinstr long do_fast_syscall_32(struct pt_regs *regs) #endif } +/* prototype is a placeholder to suppress the missing prototype worning */ +long do_SYSENTER_32(struct pt_regs *regs); + /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */ __visible noinstr long do_SYSENTER_32(struct pt_regs *regs) {
suppress unused warnings and fix the error that there is with the W=1 enabled. Warning generated arch/x86/entry/common.c:238:24: error: no previous prototype for ‘do_SYSENTER_32’ [-Werror=missing-prototypes] 238 | __visible noinstr long do_SYSENTER_32(struct pt_regs *regs) Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com> --- arch/x86/entry/common.c | 3 +++ 1 file changed, 3 insertions(+)