[v2] kbuild: Use -fmin-function-alignment when available

Commit Message

Petr Pavlu Feb. 15, 2024, 3:16 p.m. UTC

GCC recently added option -fmin-function-alignment, which should appear
in GCC 14. Unlike -falign-functions, this option causes all functions to
be aligned at the specified value, including the cold ones.

Detect availability of -fmin-function-alignment and use it instead of
-falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
and make the workarounds for the broken function alignment conditional
on this setting.

Signed-off-by: Petr Pavlu <[e-mail]>
---

Changes since v1 [1]:
- Check the availability of -fmin-function-alignment only in one place.

[1] https://lore.kernel.org/linux-kbuild/[e-mail]/

 Makefile                       |  7 +++++++
 arch/Kconfig                   | 12 ++++++++++++
 include/linux/compiler_types.h | 10 +++++-----
 kernel/exit.c                  |  5 ++++-
 4 files changed, 28 insertions(+), 6 deletions(-)


base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de

Comments

Nathan Chancellor Feb. 17, 2024, 12:07 a.m. UTC | #1

On Thu, Feb 15, 2024 at 04:16:42PM +0100, Petr Pavlu wrote:
> GCC recently added option -fmin-function-alignment, which should appear
> in GCC 14. Unlike -falign-functions, this option causes all functions to
> be aligned at the specified value, including the cold ones.
> 
> Detect availability of -fmin-function-alignment and use it instead of
> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> and make the workarounds for the broken function alignment conditional
> on this setting.
> 
> Signed-off-by: Petr Pavlu <[e-mail]>

Thanks, this looks good to me from a Kbuild perspective.

Reviewed-by: Nathan Chancellor <[e-mail]>

> ---
> 
> Changes since v1 [1]:
> - Check the availability of -fmin-function-alignment only in one place.
> 
> [1] https://lore.kernel.org/linux-kbuild/[e-mail]/
> 
>  Makefile                       |  7 +++++++
>  arch/Kconfig                   | 12 ++++++++++++
>  include/linux/compiler_types.h | 10 +++++-----
>  kernel/exit.c                  |  5 ++++-
>  4 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7e0b2ad98905..6f20ab5e2e44 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -974,8 +974,15 @@ export CC_FLAGS_CFI
>  endif
>  
>  ifneq ($(CONFIG_FUNCTION_ALIGNMENT),0)
> +# Set the minimal function alignment. Use the newer GCC option
> +# -fmin-function-alignment if it is available, or fall back to -falign-funtions.
> +# See also CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT.
> +ifdef CONFIG_CC_HAS_MIN_FUNCTION_ALIGNMENT
> +KBUILD_CFLAGS += -fmin-function-alignment=$(CONFIG_FUNCTION_ALIGNMENT)
> +else
>  KBUILD_CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
>  endif
> +endif
>  
>  # arch Makefile may override CC so keep this after arch Makefile is included
>  NOSTDINC_FLAGS += -nostdinc
> diff --git a/arch/Kconfig b/arch/Kconfig
> index a5af0edd3eb8..bd6c6335efac 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1507,4 +1507,16 @@ config FUNCTION_ALIGNMENT
>  	default 4 if FUNCTION_ALIGNMENT_4B
>  	default 0
>  
> +config CC_HAS_MIN_FUNCTION_ALIGNMENT
> +	# Detect availability of the GCC option -fmin-function-alignment which
> +	# guarantees minimal alignment for all functions, unlike
> +	# -falign-functions which the compiler ignores for cold functions.
> +	def_bool $(cc-option, -fmin-function-alignment=8)
> +
> +config CC_HAS_SANE_FUNCTION_ALIGNMENT
> +	# Set if the guaranteed alignment with -fmin-function-alignment is
> +	# available or extra care is required in the kernel. Clang provides
> +	# strict alignment always, even with -falign-functions.
> +	def_bool CC_HAS_MIN_FUNCTION_ALIGNMENT || CC_IS_CLANG
> +
>  endmenu
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 663d8791c871..f0152165e83c 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -99,17 +99,17 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
>   *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Label-Attributes.html#index-cold-label-attribute
>   *
>   * When -falign-functions=N is in use, we must avoid the cold attribute as
> - * contemporary versions of GCC drop the alignment for cold functions. Worse,
> - * GCC can implicitly mark callees of cold functions as cold themselves, so
> - * it's not sufficient to add __function_aligned here as that will not ensure
> - * that callees are correctly aligned.
> + * GCC drops the alignment for cold functions. Worse, GCC can implicitly mark
> + * callees of cold functions as cold themselves, so it's not sufficient to add
> + * __function_aligned here as that will not ensure that callees are correctly
> + * aligned.
>   *
>   * See:
>   *
>   *   https://lore.kernel.org/lkml/Y77%2FqVgvaJidFpYt@FVFF77S0Q05N
>   *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c9
>   */
> -#if !defined(CONFIG_CC_IS_GCC) || (CONFIG_FUNCTION_ALIGNMENT == 0)
> +#if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || (CONFIG_FUNCTION_ALIGNMENT == 0)
>  #define __cold				__attribute__((__cold__))
>  #else
>  #define __cold
> diff --git a/kernel/exit.c b/kernel/exit.c
> index dfb963d2f862..5a6fed4ad3df 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
>   *
>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
>   */
> -__weak __function_aligned void abort(void)
> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> +__function_aligned
> +#endif
> +__weak void abort(void)
>  {
>  	BUG();
>  
> 
> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
> -- 
> 2.35.3
>

Mark Rutland Feb. 19, 2024, 5:20 p.m. UTC | #2

On Thu, Feb 15, 2024 at 04:16:42PM +0100, Petr Pavlu wrote:
> GCC recently added option -fmin-function-alignment, which should appear
> in GCC 14. Unlike -falign-functions, this option causes all functions to
> be aligned at the specified value, including the cold ones.
> 
> Detect availability of -fmin-function-alignment and use it instead of
> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> and make the workarounds for the broken function alignment conditional
> on this setting.
> 
> Signed-off-by: Petr Pavlu <[e-mail]>

I don't have a GCC 14 build to play with, but this looks sound to me.

Petr, are you able to test an arm64 kernel with this and DYNAMIC_FTRACE
enabled? i.e. build that, and check that function symbols are all aligned to 8
bytes using objdump or similar? That way we could be pretty sure there's no
other latent issue in this area.

FWIW, given the structure looks sound:

Acked-by: Mark Rutland <[e-mail]>

Mark.

> ---
> 
> Changes since v1 [1]:
> - Check the availability of -fmin-function-alignment only in one place.
> 
> [1] https://lore.kernel.org/linux-kbuild/[e-mail]/
> 
>  Makefile                       |  7 +++++++
>  arch/Kconfig                   | 12 ++++++++++++
>  include/linux/compiler_types.h | 10 +++++-----
>  kernel/exit.c                  |  5 ++++-
>  4 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 7e0b2ad98905..6f20ab5e2e44 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -974,8 +974,15 @@ export CC_FLAGS_CFI
>  endif
>  
>  ifneq ($(CONFIG_FUNCTION_ALIGNMENT),0)
> +# Set the minimal function alignment. Use the newer GCC option
> +# -fmin-function-alignment if it is available, or fall back to -falign-funtions.
> +# See also CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT.
> +ifdef CONFIG_CC_HAS_MIN_FUNCTION_ALIGNMENT
> +KBUILD_CFLAGS += -fmin-function-alignment=$(CONFIG_FUNCTION_ALIGNMENT)
> +else
>  KBUILD_CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
>  endif
> +endif
>  
>  # arch Makefile may override CC so keep this after arch Makefile is included
>  NOSTDINC_FLAGS += -nostdinc
> diff --git a/arch/Kconfig b/arch/Kconfig
> index a5af0edd3eb8..bd6c6335efac 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -1507,4 +1507,16 @@ config FUNCTION_ALIGNMENT
>  	default 4 if FUNCTION_ALIGNMENT_4B
>  	default 0
>  
> +config CC_HAS_MIN_FUNCTION_ALIGNMENT
> +	# Detect availability of the GCC option -fmin-function-alignment which
> +	# guarantees minimal alignment for all functions, unlike
> +	# -falign-functions which the compiler ignores for cold functions.
> +	def_bool $(cc-option, -fmin-function-alignment=8)
> +
> +config CC_HAS_SANE_FUNCTION_ALIGNMENT
> +	# Set if the guaranteed alignment with -fmin-function-alignment is
> +	# available or extra care is required in the kernel. Clang provides
> +	# strict alignment always, even with -falign-functions.
> +	def_bool CC_HAS_MIN_FUNCTION_ALIGNMENT || CC_IS_CLANG
> +
>  endmenu
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 663d8791c871..f0152165e83c 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -99,17 +99,17 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
>   *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Label-Attributes.html#index-cold-label-attribute
>   *
>   * When -falign-functions=N is in use, we must avoid the cold attribute as
> - * contemporary versions of GCC drop the alignment for cold functions. Worse,
> - * GCC can implicitly mark callees of cold functions as cold themselves, so
> - * it's not sufficient to add __function_aligned here as that will not ensure
> - * that callees are correctly aligned.
> + * GCC drops the alignment for cold functions. Worse, GCC can implicitly mark
> + * callees of cold functions as cold themselves, so it's not sufficient to add
> + * __function_aligned here as that will not ensure that callees are correctly
> + * aligned.
>   *
>   * See:
>   *
>   *   https://lore.kernel.org/lkml/Y77%2FqVgvaJidFpYt@FVFF77S0Q05N
>   *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c9
>   */
> -#if !defined(CONFIG_CC_IS_GCC) || (CONFIG_FUNCTION_ALIGNMENT == 0)
> +#if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || (CONFIG_FUNCTION_ALIGNMENT == 0)
>  #define __cold				__attribute__((__cold__))
>  #else
>  #define __cold
> diff --git a/kernel/exit.c b/kernel/exit.c
> index dfb963d2f862..5a6fed4ad3df 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
>   *
>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
>   */
> -__weak __function_aligned void abort(void)
> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> +__function_aligned
> +#endif
> +__weak void abort(void)
>  {
>  	BUG();
>  
> 
> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
> -- 
> 2.35.3
>

Masahiro Yamada Feb. 20, 2024, 1:39 p.m. UTC | #3

On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <[e-mail]> wrote:
>
> GCC recently added option -fmin-function-alignment, which should appear
> in GCC 14. Unlike -falign-functions, this option causes all functions to
> be aligned at the specified value, including the cold ones.
>
> Detect availability of -fmin-function-alignment and use it instead of
> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> and make the workarounds for the broken function alignment conditional
> on this setting.
>
> Signed-off-by: Petr Pavlu <[e-mail]>
> ---

[snip]

> index dfb963d2f862..5a6fed4ad3df 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
>   *
>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
>   */
> -__weak __function_aligned void abort(void)
> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> +__function_aligned
> +#endif
> +__weak void abort(void)
>  {
>         BUG();





__function_aligned is conditionally defined in
include/linux/compiler_types.h, and then it is
conditionally used in kernel/exit.c

This is unreadable.




You may want to move CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
to include/linux/compiler_types.h, as this is more
aligned with what you did for __cold.



if !defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) && \
               CONFIG_FUNCTION_ALIGNMENT > 0
#define __function_aligned       __aligned(CONFIG_FUNCTION_ALIGNMENT)
#else
#define __function_aligned
#endif





However, an even more elegant approach is to unify
the two #ifdef blocks because __cold and __function_aligned
are related to each other.



#if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
                 (CONFIG_FUNCTION_ALIGNMENT == 0)
#define __cold                 __attribute__((__cold__))
#define __function_aligned
#else
#define __cold
#define __function_aligned     __aligned(CONFIG_FUNCTION_ALIGNMENT)
#endif









--
Best Regards
Masahiro Yamada

Petr Pavlu Feb. 20, 2024, 3:28 p.m. UTC | #4

On 2/19/24 18:20, Mark Rutland wrote:
> On Thu, Feb 15, 2024 at 04:16:42PM +0100, Petr Pavlu wrote:
>> GCC recently added option -fmin-function-alignment, which should appear
>> in GCC 14. Unlike -falign-functions, this option causes all functions to
>> be aligned at the specified value, including the cold ones.
>>
>> Detect availability of -fmin-function-alignment and use it instead of
>> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
>> and make the workarounds for the broken function alignment conditional
>> on this setting.
>>
>> Signed-off-by: Petr Pavlu <[e-mail]>
> 
> I don't have a GCC 14 build to play with, but this looks sound to me.
> 
> Petr, are you able to test an arm64 kernel with this and DYNAMIC_FTRACE
> enabled? i.e. build that, and check that function symbols are all aligned to 8
> bytes using objdump or similar? That way we could be pretty sure there's no
> other latent issue in this area.

I tested an arm64 kernel with DYNAMIC_FTRACE, in particular with
DYNAMIC_FTRACE_WITH_CALL_OPS=y. That is actually the primary motivation
for this patch. We ran in our environment into some incorrectly aligned
functions with this option despite the kernel workarounds. They were
reported as "Misaligned patch-site" warnings from ftrace_call_adjust().
I don't observe them anymore with -fmin-function-alignment in my tests.
Sorry, I should have mentioned this motivation in the commit message.

Petr Pavlu Feb. 21, 2024, 10:38 a.m. UTC | #5

On 2/20/24 14:39, Masahiro Yamada wrote:
> On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <[e-mail]> wrote:
>>
>> GCC recently added option -fmin-function-alignment, which should appear
>> in GCC 14. Unlike -falign-functions, this option causes all functions to
>> be aligned at the specified value, including the cold ones.
>>
>> Detect availability of -fmin-function-alignment and use it instead of
>> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
>> and make the workarounds for the broken function alignment conditional
>> on this setting.
>>
>> Signed-off-by: Petr Pavlu <[e-mail]>
>> ---
> 
> [snip]
> 
>> index dfb963d2f862..5a6fed4ad3df 100644
>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
>>   *
>>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
>>   */
>> -__weak __function_aligned void abort(void)
>> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
>> +__function_aligned
>> +#endif
>> +__weak void abort(void)
>>  {
>>         BUG();
> 
> 
> 
> 
> 
> __function_aligned is conditionally defined in
> include/linux/compiler_types.h, and then it is
> conditionally used in kernel/exit.c
> 
> This is unreadable.
> 
> 
> 
> 
> You may want to move CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> to include/linux/compiler_types.h, as this is more
> aligned with what you did for __cold.
> 
> 
> 
> if !defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) && \
>                CONFIG_FUNCTION_ALIGNMENT > 0
> #define __function_aligned       __aligned(CONFIG_FUNCTION_ALIGNMENT)
> #else
> #define __function_aligned
> #endif
> 
> 
> 
> 
> 
> However, an even more elegant approach is to unify
> the two #ifdef blocks because __cold and __function_aligned
> are related to each other.
> 
> 
> 
> #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
>                  (CONFIG_FUNCTION_ALIGNMENT == 0)
> #define __cold                 __attribute__((__cold__))
> #define __function_aligned
> #else
> #define __cold
> #define __function_aligned     __aligned(CONFIG_FUNCTION_ALIGNMENT)
> #endif

I didn't want to make __function_aligned conditional on
CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT because the macro has a fairly
general name. One could decide to mark a variable as __function_aligned
and with the above code, it would no longer produce an expected result
when -fmin-function-alignment is available.

__function_aligned was introduced c27cd083cfb9 ("Compiler attributes:
GCC cold function alignment workarounds") only for aligning the abort()
function and has not been so far used anywhere else.

If the above unification is preferred, I think it would be good to
additionally rename the macro in order to prevent the mentioned misuse,
perhaps to __force_function_alignment.

#if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
		(CONFIG_FUNCTION_ALIGNMENT == 0)
#define __cold				__attribute__((__cold__))
#define __force_function_alignment
#else
#define __cold
#define __force_function_alignment	__aligned(CONFIG_FUNCTION_ALIGNMENT)
#endif

Would this be ok?

Mark Rutland Feb. 21, 2024, 10:49 a.m. UTC | #6

On Wed, Feb 21, 2024 at 11:38:38AM +0100, Petr Pavlu wrote:
> On 2/20/24 14:39, Masahiro Yamada wrote:
> > On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <[e-mail]> wrote:
> >>
> >> GCC recently added option -fmin-function-alignment, which should appear
> >> in GCC 14. Unlike -falign-functions, this option causes all functions to
> >> be aligned at the specified value, including the cold ones.
> >>
> >> Detect availability of -fmin-function-alignment and use it instead of
> >> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> and make the workarounds for the broken function alignment conditional
> >> on this setting.
> >>
> >> Signed-off-by: Petr Pavlu <[e-mail]>
> >> ---
> > 
> > [snip]
> > 
> >> index dfb963d2f862..5a6fed4ad3df 100644
> >> --- a/kernel/exit.c
> >> +++ b/kernel/exit.c
> >> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
> >>   *
> >>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
> >>   */
> >> -__weak __function_aligned void abort(void)
> >> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> +__function_aligned
> >> +#endif
> >> +__weak void abort(void)
> >>  {
> >>         BUG();
> > 
> > 
> > 
> > 
> > 
> > __function_aligned is conditionally defined in
> > include/linux/compiler_types.h, and then it is
> > conditionally used in kernel/exit.c
> > 
> > This is unreadable.
> > 
> > 
> > 
> > 
> > You may want to move CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> > to include/linux/compiler_types.h, as this is more
> > aligned with what you did for __cold.
> > 
> > 
> > 
> > if !defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) && \
> >                CONFIG_FUNCTION_ALIGNMENT > 0
> > #define __function_aligned       __aligned(CONFIG_FUNCTION_ALIGNMENT)
> > #else
> > #define __function_aligned
> > #endif
> > 
> > 
> > 
> > 
> > 
> > However, an even more elegant approach is to unify
> > the two #ifdef blocks because __cold and __function_aligned
> > are related to each other.
> > 
> > 
> > 
> > #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
> >                  (CONFIG_FUNCTION_ALIGNMENT == 0)
> > #define __cold                 __attribute__((__cold__))
> > #define __function_aligned
> > #else
> > #define __cold
> > #define __function_aligned     __aligned(CONFIG_FUNCTION_ALIGNMENT)
> > #endif
> 
> I didn't want to make __function_aligned conditional on
> CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT because the macro has a fairly
> general name. One could decide to mark a variable as __function_aligned
> and with the above code, it would no longer produce an expected result
> when -fmin-function-alignment is available.
> 
> __function_aligned was introduced c27cd083cfb9 ("Compiler attributes:
> GCC cold function alignment workarounds") only for aligning the abort()
> function and has not been so far used anywhere else.
> 
> If the above unification is preferred, I think it would be good to
> additionally rename the macro in order to prevent the mentioned misuse,
> perhaps to __force_function_alignment.
> 
> #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
> 		(CONFIG_FUNCTION_ALIGNMENT == 0)
> #define __cold				__attribute__((__cold__))
> #define __force_function_alignment
> #else
> #define __cold
> #define __force_function_alignment	__aligned(CONFIG_FUNCTION_ALIGNMENT)
> #endif
> 
> Would this be ok?

FWIW, renaming this to __force_function_alignment makes sense to me, and I'm
happy with the above.

Mark.

Mark Rutland Feb. 21, 2024, 10:50 a.m. UTC | #7

On Tue, Feb 20, 2024 at 04:28:23PM +0100, Petr Pavlu wrote:
> On 2/19/24 18:20, Mark Rutland wrote:
> > On Thu, Feb 15, 2024 at 04:16:42PM +0100, Petr Pavlu wrote:
> >> GCC recently added option -fmin-function-alignment, which should appear
> >> in GCC 14. Unlike -falign-functions, this option causes all functions to
> >> be aligned at the specified value, including the cold ones.
> >>
> >> Detect availability of -fmin-function-alignment and use it instead of
> >> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> and make the workarounds for the broken function alignment conditional
> >> on this setting.
> >>
> >> Signed-off-by: Petr Pavlu <[e-mail]>
> > 
> > I don't have a GCC 14 build to play with, but this looks sound to me.
> > 
> > Petr, are you able to test an arm64 kernel with this and DYNAMIC_FTRACE
> > enabled? i.e. build that, and check that function symbols are all aligned to 8
> > bytes using objdump or similar? That way we could be pretty sure there's no
> > other latent issue in this area.
> 
> I tested an arm64 kernel with DYNAMIC_FTRACE, in particular with
> DYNAMIC_FTRACE_WITH_CALL_OPS=y. That is actually the primary motivation
> for this patch. We ran in our environment into some incorrectly aligned
> functions with this option despite the kernel workarounds. They were
> reported as "Misaligned patch-site" warnings from ftrace_call_adjust().
> I don't observe them anymore with -fmin-function-alignment in my tests.
> Sorry, I should have mentioned this motivation in the commit message.

No problem; thanks for confirming!

Mark.

Masahiro Yamada Feb. 21, 2024, 11:38 a.m. UTC | #8

On Wed, Feb 21, 2024 at 7:38 PM Petr Pavlu <[e-mail]> wrote:
>
> On 2/20/24 14:39, Masahiro Yamada wrote:
> > On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <[e-mail]> wrote:
> >>
> >> GCC recently added option -fmin-function-alignment, which should appear
> >> in GCC 14. Unlike -falign-functions, this option causes all functions to
> >> be aligned at the specified value, including the cold ones.
> >>
> >> Detect availability of -fmin-function-alignment and use it instead of
> >> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> and make the workarounds for the broken function alignment conditional
> >> on this setting.
> >>
> >> Signed-off-by: Petr Pavlu <[e-mail]>
> >> ---
> >
> > [snip]
> >
> >> index dfb963d2f862..5a6fed4ad3df 100644
> >> --- a/kernel/exit.c
> >> +++ b/kernel/exit.c
> >> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
> >>   *
> >>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
> >>   */
> >> -__weak __function_aligned void abort(void)
> >> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> +__function_aligned
> >> +#endif
> >> +__weak void abort(void)
> >>  {
> >>         BUG();
> >
> >
> >
> >
> >
> > __function_aligned is conditionally defined in
> > include/linux/compiler_types.h, and then it is
> > conditionally used in kernel/exit.c
> >
> > This is unreadable.
> >
> >
> >
> >
> > You may want to move CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> > to include/linux/compiler_types.h, as this is more
> > aligned with what you did for __cold.
> >
> >
> >
> > if !defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) && \
> >                CONFIG_FUNCTION_ALIGNMENT > 0
> > #define __function_aligned       __aligned(CONFIG_FUNCTION_ALIGNMENT)
> > #else
> > #define __function_aligned
> > #endif
> >
> >
> >
> >
> >
> > However, an even more elegant approach is to unify
> > the two #ifdef blocks because __cold and __function_aligned
> > are related to each other.
> >
> >
> >
> > #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
> >                  (CONFIG_FUNCTION_ALIGNMENT == 0)
> > #define __cold                 __attribute__((__cold__))
> > #define __function_aligned
> > #else
> > #define __cold
> > #define __function_aligned     __aligned(CONFIG_FUNCTION_ALIGNMENT)
> > #endif
>
> I didn't want to make __function_aligned conditional on
> CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT because the macro has a fairly
> general name. One could decide to mark a variable as __function_aligned
> and with the above code, it would no longer produce an expected result
> when -fmin-function-alignment is available.
>
> __function_aligned was introduced c27cd083cfb9 ("Compiler attributes:
> GCC cold function alignment workarounds") only for aligning the abort()
> function and has not been so far used anywhere else.
>
> If the above unification is preferred, I think it would be good to
> additionally rename the macro in order to prevent the mentioned misuse,
> perhaps to __force_function_alignment.
>
> #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
>                 (CONFIG_FUNCTION_ALIGNMENT == 0)
> #define __cold                          __attribute__((__cold__))
> #define __force_function_alignment
> #else
> #define __cold
> #define __force_function_alignment      __aligned(CONFIG_FUNCTION_ALIGNMENT)
> #endif
>
> Would this be ok?





Or, you can always add __function_aligned to abort()
whether CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT is y or n.


I think you did not need to modify kernel/exit.c









--
Best Regards
Masahiro Yamada

Petr Pavlu Feb. 21, 2024, 12:58 p.m. UTC | #9

On 2/21/24 12:38, Masahiro Yamada wrote:
> On Wed, Feb 21, 2024 at 7:38 PM Petr Pavlu <[e-mail]> wrote:
>>
>> On 2/20/24 14:39, Masahiro Yamada wrote:
>>> On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <[e-mail]> wrote:
>>>>
>>>> GCC recently added option -fmin-function-alignment, which should appear
>>>> in GCC 14. Unlike -falign-functions, this option causes all functions to
>>>> be aligned at the specified value, including the cold ones.
>>>>
>>>> Detect availability of -fmin-function-alignment and use it instead of
>>>> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
>>>> and make the workarounds for the broken function alignment conditional
>>>> on this setting.
>>>>
>>>> Signed-off-by: Petr Pavlu <[e-mail]>
>>>> ---
>>>
>>> [snip]
>>>
>>>> index dfb963d2f862..5a6fed4ad3df 100644
>>>> --- a/kernel/exit.c
>>>> +++ b/kernel/exit.c
>>>> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
>>>>   *
>>>>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
>>>>   */
>>>> -__weak __function_aligned void abort(void)
>>>> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
>>>> +__function_aligned
>>>> +#endif
>>>> +__weak void abort(void)
>>>>  {
>>>>         BUG();
>>>
>>>
>>>
>>>
>>>
>>> __function_aligned is conditionally defined in
>>> include/linux/compiler_types.h, and then it is
>>> conditionally used in kernel/exit.c
>>>
>>> This is unreadable.
>>>
>>>
>>>
>>>
>>> You may want to move CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
>>> to include/linux/compiler_types.h, as this is more
>>> aligned with what you did for __cold.
>>>
>>>
>>>
>>> if !defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) && \
>>>                CONFIG_FUNCTION_ALIGNMENT > 0
>>> #define __function_aligned       __aligned(CONFIG_FUNCTION_ALIGNMENT)
>>> #else
>>> #define __function_aligned
>>> #endif
>>>
>>>
>>>
>>>
>>>
>>> However, an even more elegant approach is to unify
>>> the two #ifdef blocks because __cold and __function_aligned
>>> are related to each other.
>>>
>>>
>>>
>>> #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
>>>                  (CONFIG_FUNCTION_ALIGNMENT == 0)
>>> #define __cold                 __attribute__((__cold__))
>>> #define __function_aligned
>>> #else
>>> #define __cold
>>> #define __function_aligned     __aligned(CONFIG_FUNCTION_ALIGNMENT)
>>> #endif
>>
>> I didn't want to make __function_aligned conditional on
>> CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT because the macro has a fairly
>> general name. One could decide to mark a variable as __function_aligned
>> and with the above code, it would no longer produce an expected result
>> when -fmin-function-alignment is available.
>>
>> __function_aligned was introduced c27cd083cfb9 ("Compiler attributes:
>> GCC cold function alignment workarounds") only for aligning the abort()
>> function and has not been so far used anywhere else.
>>
>> If the above unification is preferred, I think it would be good to
>> additionally rename the macro in order to prevent the mentioned misuse,
>> perhaps to __force_function_alignment.
>>
>> #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
>>                 (CONFIG_FUNCTION_ALIGNMENT == 0)
>> #define __cold                          __attribute__((__cold__))
>> #define __force_function_alignment
>> #else
>> #define __cold
>> #define __force_function_alignment      __aligned(CONFIG_FUNCTION_ALIGNMENT)
>> #endif
>>
>> Would this be ok?
> 
> 
> 
> 
> 
> Or, you can always add __function_aligned to abort()
> whether CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT is y or n.
> 
> 
> I think you did not need to modify kernel/exit.c

Ah, that looks as the simplest option, thanks.

-- Petr

Patch

diff --git a/Makefile b/Makefile
index 7e0b2ad98905..6f20ab5e2e44 100644
--- a/Makefile
+++ b/Makefile
@@ -974,8 +974,15 @@  export CC_FLAGS_CFI
 endif
 
 ifneq ($(CONFIG_FUNCTION_ALIGNMENT),0)
+# Set the minimal function alignment. Use the newer GCC option
+# -fmin-function-alignment if it is available, or fall back to -falign-funtions.
+# See also CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT.
+ifdef CONFIG_CC_HAS_MIN_FUNCTION_ALIGNMENT
+KBUILD_CFLAGS += -fmin-function-alignment=$(CONFIG_FUNCTION_ALIGNMENT)
+else
 KBUILD_CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
 endif
+endif
 
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc
diff --git a/arch/Kconfig b/arch/Kconfig
index a5af0edd3eb8..bd6c6335efac 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1507,4 +1507,16 @@  config FUNCTION_ALIGNMENT
 	default 4 if FUNCTION_ALIGNMENT_4B
 	default 0
 
+config CC_HAS_MIN_FUNCTION_ALIGNMENT
+	# Detect availability of the GCC option -fmin-function-alignment which
+	# guarantees minimal alignment for all functions, unlike
+	# -falign-functions which the compiler ignores for cold functions.
+	def_bool $(cc-option, -fmin-function-alignment=8)
+
+config CC_HAS_SANE_FUNCTION_ALIGNMENT
+	# Set if the guaranteed alignment with -fmin-function-alignment is
+	# available or extra care is required in the kernel. Clang provides
+	# strict alignment always, even with -falign-functions.
+	def_bool CC_HAS_MIN_FUNCTION_ALIGNMENT || CC_IS_CLANG
+
 endmenu
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 663d8791c871..f0152165e83c 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -99,17 +99,17 @@  static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Label-Attributes.html#index-cold-label-attribute
  *
  * When -falign-functions=N is in use, we must avoid the cold attribute as
- * contemporary versions of GCC drop the alignment for cold functions. Worse,
- * GCC can implicitly mark callees of cold functions as cold themselves, so
- * it's not sufficient to add __function_aligned here as that will not ensure
- * that callees are correctly aligned.
+ * GCC drops the alignment for cold functions. Worse, GCC can implicitly mark
+ * callees of cold functions as cold themselves, so it's not sufficient to add
+ * __function_aligned here as that will not ensure that callees are correctly
+ * aligned.
  *
  * See:
  *
  *   https://lore.kernel.org/lkml/Y77%2FqVgvaJidFpYt@FVFF77S0Q05N
  *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c9
  */
-#if !defined(CONFIG_CC_IS_GCC) || (CONFIG_FUNCTION_ALIGNMENT == 0)
+#if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || (CONFIG_FUNCTION_ALIGNMENT == 0)
 #define __cold				__attribute__((__cold__))
 #else
 #define __cold
diff --git a/kernel/exit.c b/kernel/exit.c
index dfb963d2f862..5a6fed4ad3df 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1920,7 +1920,10 @@  EXPORT_SYMBOL(thread_group_exited);
  *
  * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
  */
-__weak __function_aligned void abort(void)
+#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
+__function_aligned
+#endif
+__weak void abort(void)
 {
 	BUG();