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

[Petr Pavlu <petr.pavlu@suse.com>]

On 2/20/24 14:39, Masahiro Yamada wrote:
> On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <petr.pavlu@suse.com> 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@suse.com>
>> ---
> 
> [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