[libc] fix memcpy builtin looping

[michaelrj]

Summary

previously, memcpy could get stuck in a loop, calling __builtin_memcpy
which would redirect to itself. This patch removes that path.

[sivachandra]

libc/cmake/modules/LLVMLibCLibraryRules.cmake

165 ↗

(On Diff #393545)

Since header libraries are not listed as dependencies using target_link_libraries, this is not sufficient. However, we don't need this at all. Callers of inline_memcpy, which are not memcpy, can tolerate calls to memcpy from inline_memcpy - there is no recursive call to memcpy. Only memcpy should be built with -fno-builtin-memcpy as we don't want inline_memcpy in this case to call memcpy again.

libc/src/string/memory_utils/elements.h

515

This should be sufficient to fix the problem.

[michaelrj]

remove unnecessary compile options

[sivachandra]

libc/cmake/modules/LLVMLibCLibraryRules.cmake

139 ↗

(On Diff #393562)

Remove this also as CMake makes compile options for header only libraries almost pointless.

[michaelrj]

fix small bit I missed

[gchatelet]

libc/src/string/memory_utils/elements.h

515

Yes but I think that the functions embedding inline_memcpy via 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.

[sivachandra]

libc/src/string/memory_utils/elements.h

515

<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?

[gchatelet]

libc/src/string/memory_utils/elements.h

515

<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.

[sivachandra]

libc/src/string/memory_utils/elements.h

515

Only surprising that this behavior is triggered by a change in compiler version.

The problem was triggered by using a very old compiler which does not have __builtin_memcpy_inline. Not that we don't test with old compilers on regular basis - our Arm CI worker uses clang-9. Michael was probably the first person to try to use llvm libc as a libc with an old compiler. Our CI workers only run the unittests. The problem which Michael ran into is probably is indicative of the fact that we don't have good integration testing.

libc/src/string/memory_utils/elements.h

template <size_t Size> struct Builtin {
 
   static void copy(char *__restrict dst, const char *__restrict src) {
 #if LLVM_LIBC_HAVE_MEMORY_SANITIZER || LLVM_LIBC_HAVE_ADDRESS_SANITIZER
     for_loop_copy(dst, src);
 #elif __has_builtin(__builtin_memcpy_inline)
     // __builtin_memcpy_inline guarantees to never call external functions.
     // Unfortunately it is not widely available.
     __builtin_memcpy_inline(dst, src, SIZE);
-#elif __has_builtin(__builtin_memcpy)
-    __builtin_memcpy(dst, src, SIZE);
 #else
     for_loop_copy(dst, src);
 #endif
   }
 
   static void move(char *dst, const char *src) {
 #if LLVM_LIBC_HAVE_MEMORY_SANITIZER || LLVM_LIBC_HAVE_ADDRESS_SANITIZER
     for_loop_move(dst, src);