| Message ID | 20190302000715.130116-1-ndesaulniers@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: clean up headers | expand |
On Fri, Mar 01, 2019 at 04:07:14PM -0800, Nick Desaulniers wrote: > The inclusion of <linux/kernel.h> was causing issue as the definition of > __arch_hweight64 from arch/x86/include/asm/arch_hweight.h eventually gets > included. The definition is problematic when compiled with -m16 (all code > in arch/x86/boot/ is) as the "D" inline assembly constraint is rejected > by both compilers when passed an argument of type long long (regardless > of signedness, anything smaller is fine). > > Because GCC performs inlining before semantic analysis, and > __arch_hweight64 is dead in this translation unit, GCC does not report > any issues at compile time. Clang does the semantic analysis in the > front end, before inlining (run in the middle) can determine the code is > dead. I consider this another case of PR33587, which I think we can do > more work to solve. > > It turns out that arch/x86/boot/string.c doesn't actually need > linux/kernel.h, simply linux/limits.h and linux/compiler.h. Include them, > and sort the headers alphabetically. > > Link: https://bugs.llvm.org/show_bug.cgi?id=33587 > Link: https://github.com/ClangBuiltLinux/linux/issues/347 > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com> > --- > Note that this only regresses for us on linux-next (not mainline). > > arch/x86/boot/string.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c > index 315a67b8896b..f149316116d0 100644 > --- a/arch/x86/boot/string.c > +++ b/arch/x86/boot/string.c > @@ -12,10 +12,11 @@ > * Very basic string functions > */ > > -#include <linux/types.h> > -#include <linux/kernel.h> > -#include <linux/errno.h> > #include <asm/asm.h> > +#include <linux/compiler.h> > +#include <linux/errno.h> > +#include <linux/limits.h> > +#include <linux/types.h> > #include "ctype.h" > #include "string.h" > > -- > 2.21.0.352.gf09ad66450-goog >
Hi Nick, On Fri, 1 Mar 2019 16:07:14 -0800 Nick Desaulniers <ndesaulniers@google.com> wrote: > > It turns out that arch/x86/boot/string.c doesn't actually need > linux/kernel.h, simply linux/limits.h and linux/compiler.h. Include them, > and sort the headers alphabetically. One small nit: please do not do the sort in the same commit as the bug fix. It just complicates the review and has (a remote) possibility of adding a new problem. (Not a big issue here, but in general cleanups and bug fixes should be separate.) -- Cheers, Stephen Rothwell
Hi Nick, On Sat, 2 Mar 2019 13:27:50 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > On Fri, 1 Mar 2019 16:07:14 -0800 Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > It turns out that arch/x86/boot/string.c doesn't actually need > > linux/kernel.h, simply linux/limits.h and linux/compiler.h. Include them, > > and sort the headers alphabetically. > > One small nit: please do not do the sort in the same commit as the bug > fix. It just complicates the review and has (a remote) possibility of > adding a new problem. (Not a big issue here, but in general cleanups > and bug fixes should be separate.) Also it is *very* common for headers to be included in the order: linux/ asm/ and then local. -- Cheers, Stephen Rothwell
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c index 315a67b8896b..f149316116d0 100644 --- a/arch/x86/boot/string.c +++ b/arch/x86/boot/string.c @@ -12,10 +12,11 @@ * Very basic string functions */ -#include <linux/types.h> -#include <linux/kernel.h> -#include <linux/errno.h> #include <asm/asm.h> +#include <linux/compiler.h> +#include <linux/errno.h> +#include <linux/limits.h> +#include <linux/types.h> #include "ctype.h" #include "string.h"
The inclusion of <linux/kernel.h> was causing issue as the definition of __arch_hweight64 from arch/x86/include/asm/arch_hweight.h eventually gets included. The definition is problematic when compiled with -m16 (all code in arch/x86/boot/ is) as the "D" inline assembly constraint is rejected by both compilers when passed an argument of type long long (regardless of signedness, anything smaller is fine). Because GCC performs inlining before semantic analysis, and __arch_hweight64 is dead in this translation unit, GCC does not report any issues at compile time. Clang does the semantic analysis in the front end, before inlining (run in the middle) can determine the code is dead. I consider this another case of PR33587, which I think we can do more work to solve. It turns out that arch/x86/boot/string.c doesn't actually need linux/kernel.h, simply linux/limits.h and linux/compiler.h. Include them, and sort the headers alphabetically. Link: https://bugs.llvm.org/show_bug.cgi?id=33587 Link: https://github.com/ClangBuiltLinux/linux/issues/347 Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Note that this only regresses for us on linux-next (not mainline). arch/x86/boot/string.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) -- 2.21.0.352.gf09ad66450-goog