[PATCH] x86/pgtable: Always inline p4d_index

[PATCH] x86/pgtable: Always inline p4d_index

[Nathan Chancellor]

When building an allyesconfig build with Clang, the kernel fails to link
arch/x86/platform/efi/efi_64.o because of a failed BUILD_BUG_ON:

ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
(.text+0x8e5): undefined reference to `__compiletime_assert_277'

Since there are several BUILD_BUG_ONs in efi_64.c, I isolated it down to
the following:

    BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));

After some research, it turns out that there is a new config option
called NO_AUTO_INLINE, which adds '-fno-inline-functions' to
KBUILD_CFLAGS so that the compiler does not auto inline small functions,
which causes this BUILD_BUG_ON to fail because p4d_index is no longer an
integer constant expression.

According to the help text of the config, functions explicitly marked
inline should still be inlined. As it turns out, GCC and Clang both
support '-fno-inline-functions' but Clang only inlines functions when
they are marked with an always inline annotation[1].

Since it's expected that p4d_index should always be inlined so that its
value can be evaluated at build time, mark it as __always_inline.

[1]: https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CompilerInvocation.cpp#L637-L656

Link: https://github.com/ClangBuiltLinux/linux/issues/256
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 arch/x86/include/asm/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 40616e805292..f78e53382498 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -918,7 +918,7 @@ static inline int p4d_bad(p4d_t p4d)
 }
 #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
 
-static inline unsigned long p4d_index(unsigned long address)
+static __always_inline unsigned long p4d_index(unsigned long address)
 {
 	return (address >> P4D_SHIFT) & (PTRS_PER_P4D - 1);
 }
-- 
2.20.0

Re: [PATCH] x86/pgtable: Always inline p4d_index

[Nick Desaulniers]

On Mon, Dec 10, 2018 at 3:46 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building an allyesconfig build with Clang, the kernel fails to link
> arch/x86/platform/efi/efi_64.o because of a failed BUILD_BUG_ON:
>
> ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> (.text+0x8e5): undefined reference to `__compiletime_assert_277'
>
> Since there are several BUILD_BUG_ONs in efi_64.c, I isolated it down to
> the following:
>
>     BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
>
> After some research, it turns out that there is a new config option
> called NO_AUTO_INLINE, which adds '-fno-inline-functions' to
> KBUILD_CFLAGS so that the compiler does not auto inline small functions,
> which causes this BUILD_BUG_ON to fail because p4d_index is no longer an
> integer constant expression.
>
> According to the help text of the config, functions explicitly marked
> inline should still be inlined. As it turns out, GCC and Clang both
> support '-fno-inline-functions' but Clang only inlines functions when
> they are marked with an always inline annotation[1].
>
> Since it's expected that p4d_index should always be inlined so that its
> value can be evaluated at build time, mark it as __always_inline.
>
> [1]: https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CompilerInvocation.cpp#L637-L656

(resending as text-plain; why does gmail silently switch it back to html...)

Just some top secret github-fu: this link is bound to break over time
as the source changes.  While on that page, if you hit `y`, it's
hotkeyed to update the URL to pin it to a SHA.  That is a little more
robust (the branch or even the tree could still disappear, or history
gets rewritten (hopefully not on a master branch, but I've seen it
happen accidentally (Firefox OS))).

This doesn't look like an invasive change to me, though I'm curious
about the usefulness of CONFIG_NO_AUTO_INLINE if the kernel is still
compiled at -O2.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Thanks for sending!

>
> Link: https://github.com/ClangBuiltLinux/linux/issues/256
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  arch/x86/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 40616e805292..f78e53382498 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -918,7 +918,7 @@ static inline int p4d_bad(p4d_t p4d)
>  }
>  #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
>
> -static inline unsigned long p4d_index(unsigned long address)
> +static __always_inline unsigned long p4d_index(unsigned long address)
>  {
>         return (address >> P4D_SHIFT) & (PTRS_PER_P4D - 1);
>  }
> --
> 2.20.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v2] x86/pgtable: Always inline p4d_index

[Nathan Chancellor]

When building an allyesconfig build with Clang, the kernel fails to link
arch/x86/platform/efi/efi_64.o because of a failed BUILD_BUG_ON:

ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
(.text+0x8e5): undefined reference to `__compiletime_assert_277'

Since there are several BUILD_BUG_ONs in efi_64.c, I isolated it down to
the following:

    BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));

After some research, it turns out that there is a new config option
called NO_AUTO_INLINE, which adds '-fno-inline-functions' to
KBUILD_CFLAGS so that the compiler does not auto inline small functions,
which causes this BUILD_BUG_ON to fail because p4d_index is no longer an
integer constant expression.

According to the help text of the config, functions explicitly marked
inline should still be inlined. As it turns out, GCC and Clang both
support '-fno-inline-functions' but Clang only inlines functions when
they are marked with an always inline annotation[1].

Since it's expected that p4d_index should always be inlined so that its
value can be evaluated at build time, mark it as __always_inline.

[1]: https://github.com/llvm/llvm-project/blob/84cecfcb3db7/clang/lib/Frontend/CompilerInvocation.cpp#L638-L657

Link: https://github.com/ClangBuiltLinux/linux/issues/256
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

v1 -> v2:

* Add Nick's Reviewed-by tag.

* Update the GitHub URL to use the official LLVM repository and pin to a
  SHA so that it is always valid, as suggested by Nick.

 arch/x86/include/asm/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 40616e805292..f78e53382498 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -918,7 +918,7 @@ static inline int p4d_bad(p4d_t p4d)
 }
 #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
 
-static inline unsigned long p4d_index(unsigned long address)
+static __always_inline unsigned long p4d_index(unsigned long address)
 {
 	return (address >> P4D_SHIFT) & (PTRS_PER_P4D - 1);
 }
-- 
2.20.1

Re: [PATCH v2] x86/pgtable: Always inline p4d_index

[Randy Dunlap]

On 1/16/19 5:41 AM, Nathan Chancellor wrote:
> When building an allyesconfig build with Clang, the kernel fails to link
> arch/x86/platform/efi/efi_64.o because of a failed BUILD_BUG_ON:
> 
> ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> (.text+0x8e5): undefined reference to `__compiletime_assert_277'
> 
> Since there are several BUILD_BUG_ONs in efi_64.c, I isolated it down to
> the following:
> 
>     BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> 
> After some research, it turns out that there is a new config option
> called NO_AUTO_INLINE, which adds '-fno-inline-functions' to
> KBUILD_CFLAGS so that the compiler does not auto inline small functions,
> which causes this BUILD_BUG_ON to fail because p4d_index is no longer an
> integer constant expression.
> 
> According to the help text of the config, functions explicitly marked
> inline should still be inlined. As it turns out, GCC and Clang both
> support '-fno-inline-functions' but Clang only inlines functions when
> they are marked with an always inline annotation[1].
> 
> Since it's expected that p4d_index should always be inlined so that its
> value can be evaluated at build time, mark it as __always_inline.
> 
> [1]: https://github.com/llvm/llvm-project/blob/84cecfcb3db7/clang/lib/Frontend/CompilerInvocation.cpp#L638-L657
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/256
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Hi,
I see the same build error when using gcc 4.8.5 (SUSE Linux).
This patch fixes that build error.

Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested

Thanks.

> ---
> 
> v1 -> v2:
> 
> * Add Nick's Reviewed-by tag.
> 
> * Update the GitHub URL to use the official LLVM repository and pin to a
>   SHA so that it is always valid, as suggested by Nick.
> 
>  arch/x86/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 40616e805292..f78e53382498 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -918,7 +918,7 @@ static inline int p4d_bad(p4d_t p4d)
>  }
>  #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
>  
> -static inline unsigned long p4d_index(unsigned long address)
> +static __always_inline unsigned long p4d_index(unsigned long address)
>  {
>  	return (address >> P4D_SHIFT) & (PTRS_PER_P4D - 1);
>  }
> 


-- 
~Randy

Re: [PATCH v2] x86/pgtable: Always inline p4d_index

[Thomas Gleixner]

On Wed, 16 Jan 2019, Nathan Chancellor wrote:

> When building an allyesconfig build with Clang, the kernel fails to link
> arch/x86/platform/efi/efi_64.o because of a failed BUILD_BUG_ON:
> 
> ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> (.text+0x8e5): undefined reference to `__compiletime_assert_277'
> 
> Since there are several BUILD_BUG_ONs in efi_64.c, I isolated it down to
> the following:
> 
>     BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> 
> After some research, it turns out that there is a new config option
> called NO_AUTO_INLINE, which adds '-fno-inline-functions' to
> KBUILD_CFLAGS so that the compiler does not auto inline small functions,
> which causes this BUILD_BUG_ON to fail because p4d_index is no longer an
> integer constant expression.

There is no tree where this config option exists.

> According to the help text of the config, functions explicitly marked
> inline should still be inlined. As it turns out, GCC and Clang both
> support '-fno-inline-functions' but Clang only inlines functions when
> they are marked with an always inline annotation[1].
> 
> Since it's expected that p4d_index should always be inlined so that its
> value can be evaluated at build time, mark it as __always_inline.

Sorry no, that's just papering over the problem.

The point is that NO_AUTO_INLINE is/was meant to prevent the compiler from
automatically inlining functions, which are NOT marked inline in any form
in order to expand the traceability.

With GCC this makes sense, because it still inlines functions which are
solely marked inline and even if it decides not to inline them it will
evaluate them if they expand to a build time constant expression.

Now Clang decided to give -fno-inline-functions a different meaning,
i.e. the same as GCC has for -fno-inline, which prevents inlining for
everything except functions which are marked __always_inline.

So just "fixing" the build wreckage by duct taping one single instance of
inline functions is really a bad idea. The resulting kernel will be
bloatware because all regular inlines and we have tons of them will turn
into function calls even if the function overhead is larger than the
resulting inline code. Not to talk about the performance impact.

Anyway, as this option is found to be nowhere there is no point to apply
this patch.

Thanks,

	tglx

Re: [PATCH v2] x86/pgtable: Always inline p4d_index

[Nathan Chancellor]

On Wed, Jan 16, 2019 at 10:54:25PM +0100, Thomas Gleixner wrote:
> On Wed, 16 Jan 2019, Nathan Chancellor wrote:
> 
> > When building an allyesconfig build with Clang, the kernel fails to link
> > arch/x86/platform/efi/efi_64.o because of a failed BUILD_BUG_ON:
> > 
> > ld: arch/x86/platform/efi/efi_64.o: in function `efi_sync_low_kernel_mappings':
> > (.text+0x8e5): undefined reference to `__compiletime_assert_277'
> > 
> > Since there are several BUILD_BUG_ONs in efi_64.c, I isolated it down to
> > the following:
> > 
> >     BUILD_BUG_ON(p4d_index(EFI_VA_END) != p4d_index(MODULES_END));
> > 
> > After some research, it turns out that there is a new config option
> > called NO_AUTO_INLINE, which adds '-fno-inline-functions' to
> > KBUILD_CFLAGS so that the compiler does not auto inline small functions,
> > which causes this BUILD_BUG_ON to fail because p4d_index is no longer an
> > integer constant expression.
> 
> There is no tree where this config option exists.
> 

Hmmm, appears that Linus rejected the pull that would have added
this option, which sat in -next for a month and a half.

https://lore.kernel.org/lkml/CAHk-=wiLt3rgeyM3BWAd5VJrKcnxxuHybwoQiDGMgyspo6oXDg@mail.gmail.com/

It is no longer in -next or the kbuild tree so I guess it was scrapped.

> > According to the help text of the config, functions explicitly marked
> > inline should still be inlined. As it turns out, GCC and Clang both
> > support '-fno-inline-functions' but Clang only inlines functions when
> > they are marked with an always inline annotation[1].
> > 
> > Since it's expected that p4d_index should always be inlined so that its
> > value can be evaluated at build time, mark it as __always_inline.
> 
> Sorry no, that's just papering over the problem.
> 
> The point is that NO_AUTO_INLINE is/was meant to prevent the compiler from
> automatically inlining functions, which are NOT marked inline in any form
> in order to expand the traceability.
> 
> With GCC this makes sense, because it still inlines functions which are
> solely marked inline and even if it decides not to inline them it will
> evaluate them if they expand to a build time constant expression.
> 
> Now Clang decided to give -fno-inline-functions a different meaning,
> i.e. the same as GCC has for -fno-inline, which prevents inlining for
> everything except functions which are marked __always_inline.
> 
> So just "fixing" the build wreckage by duct taping one single instance of
> inline functions is really a bad idea. The resulting kernel will be
> bloatware because all regular inlines and we have tons of them will turn
> into function calls even if the function overhead is larger than the
> resulting inline code. Not to talk about the performance impact.
> 

Far points. I don't disagree that this is a band aid patch but I was
more concerned about the build error than the runtime impact since this
was uncovered with an allyesconfig build, which we aren't booting
anyways.

> Anyway, as this option is found to be nowhere there is no point to apply
> this patch.
> 

Indeed. If the config reappears, it should probably 'depends on CC_IS_GCC'.

> Thanks,
> 
> 	tglx
> 

Thank you for the reply and review!
Nathan
> 
> 
>