Date:   Thu, 20 Oct 2022 15:47:53 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Willy Tarreau <w@....eu>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        kernel test robot <lkp@...el.com>,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tools/nolibc: fix missing strlen() definition and
 infinite loop with gcc-12

On Sun, Oct 09, 2022 at 08:29:36PM +0200, Willy Tarreau wrote:
> When built at -Os, gcc-12 recognizes an strlen() pattern in nolibc_strlen()
> and replaces it with a jump to strlen(), which is not defined as a symbol
> and breaks compilation. Worse, when the function is called strlen(), the
> function is simply replaced with a jump to itself, hence becomes an
> infinite loop.
> 
> One way to avoid this is to always set -ffreestanding, but the calling
> code doesn't know this and there's no way (either via attributes or
> pragmas) to globally enable it from include files, effectively leaving
> a painful situation for the caller.
> 
> Alexey suggested to place an empty asm() statement inside the loop to
> stop gcc from recognizing a well-known pattern, which happens to work
> pretty fine. At least it allows us to make sure our local definition
> is not replaced with a self jump.
> 
> The function only needs to be renamed back to strlen() so that the symbol
> exists, which implies that nolibc_strlen() which is used on variable
> strings has to be declared as a macro that points back to it before the
> strlen() macro is redifined.
> 
> It was verified to produce valid code with gcc 3.4 to 12.1 at different
> optimization levels, and both with constant and variable strings.
> 
> In case this problem surfaces again in the future, an alternate approach
> consisting in adding an optimize("no-tree-loop-distribute-patterns")
> function attribute for gcc>=12 worked as well but is less pretty.
> 
> Reported-by: kernel test robot <yujie.liu@...el.com>
> Link: https://lore.kernel.org/r/202210081618.754a77db-yujie.liu@intel.com
> Fixes: 66b6f755ad45 ("rcutorture: Import a copy of nolibc")
> Fixes: 96980b833a21 ("tools/nolibc/string: do not use __builtin_strlen() at -O0")
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Alexey Dobriyan <adobriyan@...il.com>
> Signed-off-by: Willy Tarreau <w@....eu>

Hearing no objections, I have pulled this one in for review and testing,
thank you!

							Thanx, Paul

> ---
> v2: dropped the attribute(optimize) in favor of an empty asm() statement
> 
> ---
>  tools/include/nolibc/string.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
> index bef35bee9c44..718a405ffbc3 100644
> --- a/tools/include/nolibc/string.h
> +++ b/tools/include/nolibc/string.h
> @@ -125,14 +125,18 @@ char *strcpy(char *dst, const char *src)
>  }
>  
>  /* this function is only used with arguments that are not constants or when
> - * it's not known because optimizations are disabled.
> + * it's not known because optimizations are disabled. Note that gcc 12
> + * recognizes an strlen() pattern and replaces it with a jump to strlen(),
> + * thus itself, hence the asm() statement below that's meant to disable this
> + * confusing practice.
>   */
>  static __attribute__((unused))
> -size_t nolibc_strlen(const char *str)
> +size_t strlen(const char *str)
>  {
>  	size_t len;
>  
> -	for (len = 0; str[len]; len++);
> +	for (len = 0; str[len]; len++)
> +		asm("");
>  	return len;
>  }
>  
> @@ -140,13 +144,12 @@ size_t nolibc_strlen(const char *str)
>   * the two branches, then will rely on an external definition of strlen().
>   */
>  #if defined(__OPTIMIZE__)
> +#define nolibc_strlen(x) strlen(x)
>  #define strlen(str) ({                          \
>  	__builtin_constant_p((str)) ?           \
>  		__builtin_strlen((str)) :       \
>  		nolibc_strlen((str));           \
>  })
> -#else
> -#define strlen(str) nolibc_strlen((str))
>  #endif
>  
>  static __attribute__((unused))
> -- 
> 2.35.3
> 

Powered by blists - more mailing lists