[llvm] [Transforms] LoopIdiomRecognize recognize strlen and wcslen (PR #108985)

[Martin Storsjö via llvm-commits]

mstorsjo wrote:

> @mstorsjo The hang seems to have come from https://github.com/wine-mirror/wine/blob/0927c5c3da7cda8cf476416260286bd299ad6319/dlls/ntdll/string.c#L423
> 
> ```c
> size_t __cdecl strlen( const char *str )
> {
>     const char *s = str;
>     while (*s) s++;
>     return s - str;
> }
> ```
> 
> Where the strlen idiom is recognized and replaced by a call to it self. These type of scenarios are problematic in general.

Thanks for locating the issue, before I had time to dig deeper!

> For example, in GCC without `-ffreestanding` similar things can happen, where `memset` is accidentally defined as direct recursion.
> 
> ```c
> $ gcc --version
> gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-22)
> Copyright (C) 2018 Free Software Foundation, Inc.
> 
> void *memset(void *b, int c, size_t len) {
>   int i;
>   unsigned char *p = b;
>   i = 0;
>   while (len > 0) {
>     *p = c;
>     p++;
>     len--;
>   }
>   return b;
> }
> 
> 0000000000400590 <memset>:
>   400590:	48 85 d2             	test   %rdx,%rdx
>   400593:	74 1b                	je     4005b0 <memset+0x20>
>   400595:	48 83 ec 08          	sub    $0x8,%rsp
>   400599:	40 0f b6 f6          	movzbl %sil,%esi
>   40059d:	e8 ee ff ff ff       	callq  400590 <memset>
>   4005a2:	48 83 c4 08          	add    $0x8,%rsp
>   4005a6:	c3                   	retq   
>   4005a7:	66 0f 1f 84 00 00 00 	nopw   0x0(%rax,%rax,1)
>   4005ae:	00 00 
>   4005b0:	48 89 f8             	mov    %rdi,%rax
>   4005b3:	c3                   	retq   
>   4005b4:	66 2e 0f 1f 84 00 00 	nopw   %cs:0x0(%rax,%rax,1)
>   4005bb:	00 00 00 
>   4005be:	66 90                	xchg   %ax,%ax
> ```
> 
> I'm not sure if there exists a solution in general that can solve all cases of this scenario. The best solution for this is probably to pass `-ffreestanding` or the `-no-builtin-<function>`.

Indeed, this is a recurring problem when implementing many standard C functions. Wine does actually build some source files with `-fno-builtin`, exactly for this reason - https://github.com/wine-mirror/wine/blob/wine-10.4/tools/makedep.c#L3287-L3294. But this file lies outside of what those patterns cover. CC @cjacek


> Otherwise for this patch, what we can do is skip the loop idiom for functions if they have name matching `strlen` and `wcslen`. I have put up the updated patch here: #132572

Thanks, that sounds like a good idea! First when I read this, I was a bit hesitant whether you should need to implement something like that just due to this case (as Wine probably should just extend their use of `-fno-builtin` to cover this implementation too), but if there already is a mechanism for skipping these optimizations in some functions, it sounds good to extend it to cover these too.

That kind of fix would probably be useful in many other places as well; e.g. if implementing `double exp2(double x) { return pow(2, x); }`, it can also be turned back into a recursive call to `exp2()` (although that may only happen if compiled with some extra `-f` flags). A name based safeguard would probably be useful there too, and in the case of almost all libcall transformations.

> But we can still get into similar problems with indirect recursion.
> 
> ```c
> size_t strlen(const char* str) {
>   return foo(str);
> }
> __attribute__((noinline)) size_t foo(const char* str) {
>   const char *s = str;
>   while (*s) s++;
>   return s - str;
> }
> ```
> 
> However, I envision that these type of scenarios will be exceedingly rare.

Yes, that sounds quite rare. And if one does that, one definitely needs to use `-ffreestanding` or `-fno-builtin` to avoid the issue.

> Does either of those 2 options work for you?

I think both sound like a good way forward; I would hope that Wine can extend their use of `-fno-builtin` to cover this case, and your fix to avoid transforming `strlen` seems like it avoids the issue as well. Thanks!

https://github.com/llvm/llvm-project/pull/108985