[PATCH] x86/boot: clean up headers

[PATCH] x86/boot: clean up headers

[Nick Desaulniers]

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(-)

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

Re: [PATCH] x86/boot: clean up headers

[Nathan Chancellor]

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
> 

Re: [PATCH] x86/boot: clean up headers

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] x86/boot: clean up headers

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2] x86/boot: clean up headers

[Nick Desaulniers]

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
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V1 -> V2:
* Add Reviewed, Tested, Suggested tags.
* Drop linux/types.h; it's included in linux/limits.h.

My original intention was to unsort the headers (sorted in V1), but if
we drop the out of place linux/types.h, then we can insert the two more
specific headers in alphabetic order.

 arch/x86/boot/string.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 315a67b8896b..597589cafb45 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -12,9 +12,9 @@
  * Very basic string functions
  */
 
-#include <linux/types.h>
-#include <linux/kernel.h>
+#include <linux/compiler.h>
 #include <linux/errno.h>
+#include <linux/limits.h>
 #include <asm/asm.h>
 #include "ctype.h"
 #include "string.h"
-- 
2.21.0.352.gf09ad66450-goog

Re: [PATCH v2] x86/boot: clean up headers

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

Hi Nick,

On Mon,  4 Mar 2019 09:50:16 -0800 Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Changes V1 -> V2:
> * Add Reviewed, Tested, Suggested tags.
> * Drop linux/types.h; it's included in linux/limits.h.
> 
> My original intention was to unsort the headers (sorted in V1), but if
> we drop the out of place linux/types.h, then we can insert the two more
> specific headers in alphabetic order.

I don't understand why you care about the include file ordering.

Also, since this file uses things from linux/types.h, I would argue
that we need the explicit include of linux/types.h.  Just imagine if
someone at a later date reomves the linux/types.h include from
linux/limits.h (however unlikely that is).

Just do your bug fix.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3] x86/boot: clean up headers

[Nick Desaulniers]

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
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V2 -> V3:
* keep linux/types.h

Changes V1 -> V2:
* Add Reviewed, Tested, Suggested tags.
* Drop linux/types.h; it's included in linux/limits.h.

 arch/x86/boot/string.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 315a67b8896b..90154df8f125 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -13,8 +13,9 @@
  */
 
 #include <linux/types.h>
-#include <linux/kernel.h>
+#include <linux/compiler.h>
 #include <linux/errno.h>
+#include <linux/limits.h>
 #include <asm/asm.h>
 #include "ctype.h"
 #include "string.h"
-- 
2.21.0.352.gf09ad66450-goog

Re: [PATCH v3] x86/boot: clean up headers

[Nick Desaulniers]

On Mon, Mar 4, 2019 at 4:12 PM Nick Desaulniers <ndesaulniers@google.com> 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.

Well, should have cut that out of the commit message too in V3...sigh.
Boris assuming everyone's ok with this patch; can you just drop this
sentence from the commit message should you apply the patch (or shall
I send a v4)?

>
> Link: https://bugs.llvm.org/show_bug.cgi?id=33587
> Link: https://github.com/ClangBuiltLinux/linux/issues/347
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> Suggested-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes V2 -> V3:
> * keep linux/types.h
>
> Changes V1 -> V2:
> * Add Reviewed, Tested, Suggested tags.
> * Drop linux/types.h; it's included in linux/limits.h.
>
>  arch/x86/boot/string.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> index 315a67b8896b..90154df8f125 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -13,8 +13,9 @@
>   */
>
>  #include <linux/types.h>
> -#include <linux/kernel.h>
> +#include <linux/compiler.h>
>  #include <linux/errno.h>
> +#include <linux/limits.h>
>  #include <asm/asm.h>
>  #include "ctype.h"
>  #include "string.h"
> --
> 2.21.0.352.gf09ad66450-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v3] x86/boot: clean up headers

[Borislav Petkov]

On Mon, Mar 04, 2019 at 04:15:49PM -0800, Nick Desaulniers wrote:
> Well, should have cut that out of the commit message too in V3...sigh.
> Boris assuming everyone's ok with this patch; can you just drop this
> sentence from the commit message should you apply the patch (or shall
> I send a v4)?

I'll take a look once the merge window is over and all the dust settles.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v4] x86/boot: clean up headers

[Nick Desaulniers]

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.

Link: https://bugs.llvm.org/show_bug.cgi?id=33587
Link: https://github.com/ClangBuiltLinux/linux/issues/347
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V3 -> V4:
* Drop sentence from commit message about sorting headers.

Changes V2 -> V3:
* keep linux/types.h

Changes V1 -> V2:
* Add Reviewed, Tested, Suggested tags.
* Drop linux/types.h; it's included in linux/limits.h.

 arch/x86/boot/string.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 315a67b8896b..90154df8f125 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -13,8 +13,9 @@
  */
 
 #include <linux/types.h>
-#include <linux/kernel.h>
+#include <linux/compiler.h>
 #include <linux/errno.h>
+#include <linux/limits.h>
 #include <asm/asm.h>
 #include "ctype.h"
 #include "string.h"
-- 
2.21.0.360.g471c308f928-goog

[tip:x86/urgent] x86/boot: Restrict header scope to make Clang happy

[tip-bot for Nick Desaulniers]

Commit-ID:  a9c640ac96e19b3966357ec9bb586edd2e1e74de
Gitweb:     https://git.kernel.org/tip/a9c640ac96e19b3966357ec9bb586edd2e1e74de
Author:     Nick Desaulniers <ndesaulniers@google.com>
AuthorDate: Thu, 14 Mar 2019 15:14:57 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 21 Mar 2019 12:24:38 +0100

x86/boot: Restrict header scope to make Clang happy

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.

Suggested-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Cc: bp@alien8.de
Cc: niravd@google.com
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Chao Fan <fanc.fnst@cn.fujitsu.com>
Cc: Uros Bizjak <ubizjak@gmail.com>
Link: https://bugs.llvm.org/show_bug.cgi?id=33587
Link: https://github.com/ClangBuiltLinux/linux/issues/347
Link: https://lkml.kernel.org/r/20190314221458.83047-1-ndesaulniers@google.com

---
 arch/x86/boot/string.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 315a67b8896b..90154df8f125 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -13,8 +13,9 @@
  */
 
 #include <linux/types.h>
-#include <linux/kernel.h>
+#include <linux/compiler.h>
 #include <linux/errno.h>
+#include <linux/limits.h>
 #include <asm/asm.h>
 #include "ctype.h"
 #include "string.h"