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 <petr.pavlu@xxxxxxxx> 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 <petr.pavlu@xxxxxxxx> > >> --- > > > > [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.
