[libc-commits] [PATCH] D115542: [libc] fix memcpy builtin looping

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/src/string/memory_utils/elements.h:515
-#elif __has_builtin(__builtin_memcpy)
-    __builtin_memcpy(dst, src, SIZE);
 #else
----------------
sivachandra wrote:
> gchatelet wrote:
> > sivachandra wrote:
> > > This should be sufficient to fix the problem.
> > Yes but I think that the functions embedding `inline_memcpy` via [[ https://github.com/llvm/llvm-project/blob/main/libc/src/string/memory_utils/memcpy_implementations.h | memcpy_implementations.h ]] should still be marked `-fno-builtin-memcpy` overwise clang might still detect the copy for loop as `memcpy` and call it instead of embedding it.
> > 
> > To be safe we should have all string functions marked with `-fno-builtin`. This would prevent the recursive call for all of `memmove`, `memcmp`, `memset`, etc.
> <some function which is not `memcpy`> => inline_memcpy => `memcpy` - This scenario will not lead to a recursive call to `memcpy`.
> 
> If we do not want string functions in general to call the memory primitives (`memcpy` and friends), we should of course use `-fno-builtin`. But, do we really want that in general?
> <some function which is not `memcpy`> => inline_memcpy => `memcpy` - This scenario will not lead to a recursive call to `memcpy`.

Yes it will not, but it may call a different memcpy. This is OK. Only surprising that this behavior is triggered by a change in compiler version.
 
> If we do not want string functions in general to call the memory primitives (`memcpy` and friends), we should of course use `-fno-builtin`. But, do we really want that in general?

String functions can still call `memcpy` and other functions on their own and it will do the right thing. `-fno-builtin` only prevents the compiler from synthesizing these calls based on what it thinks the code is doing. IMHO adding `-fno-builtin` to libc string functions will give us peace of mind. I think we should be the one deciding if a string function ought to call another string function.

No strong opinion here though but I feel we're on the safer side this way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115542/new/

https://reviews.llvm.org/D115542