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? -- Thanks, Petr
