[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

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

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/cmake/modules/LLVMLibCLibraryRules.cmake

 #      HDRS  <list of .h files part of the library>
 #      DEPENDS <list of dependencies>
 #    )
 function(add_header_library target_name)
   cmake_parse_arguments(
     "ADD_HEADER"
     "" # No optional arguments
     "" # No Single value arguments
-    "HDRS;DEPENDS" # Multi-value arguments
+    "HDRS;DEPENDS;COMPILE_OPTIONS" # Multi-value arguments
     ${ARGN}
   )
 
   if(NOT ADD_HEADER_HDRS)
     message(FATAL_ERROR "'add_header_library' target requires a HDRS list of .h files.")
   endif()
 
   get_fq_target_name(${target_name} fq_target_name)
 
   set(FULL_HDR_PATHS "")
   # TODO: Remove this foreach block when we can switch to the new
   # version of the CMake policy CMP0076.
   foreach(hdr IN LISTS ADD_HEADER_HDRS)
     list(APPEND FULL_HDR_PATHS ${CMAKE_CURRENT_SOURCE_DIR}/${hdr})
   endforeach()
 
   set(interface_target_name "${fq_target_name}.__header_library__")
 
   add_library(${interface_target_name} INTERFACE)
   target_sources(${interface_target_name} INTERFACE ${FULL_HDR_PATHS})
   get_fq_deps_list(fq_deps_list ${ADD_HEADER_DEPENDS})
   if(ADD_HEADER_DEPENDS)
     add_dependencies(${interface_target_name} ${fq_deps_list})
   endif()
+  if(ADD_HEADER_COMPILE_OPTIONS)
+    target_compile_options(${interface_target_name} INTERFACE ${ADD_HEADER_COMPILE_OPTIONS})
+  endif()
 
   add_custom_target(${fq_target_name})
   add_dependencies(${fq_target_name} ${interface_target_name})
   set_target_properties(
     ${fq_target_name}
     PROPERTIES
       "TARGET_TYPE" "${HDR_LIBRARY_TARGET_TYPE}"
       "DEPS" "${fq_deps_list}"
   )
 endfunction(add_header_library)

libc/src/string/memory_utils/CMakeLists.txt

 )
 
 add_header_library(
   memcpy_implementation
   HDRS
     memcpy_implementations.h
   DEPS
     .memory_utils
+  COMPILE_OPTIONS
+    "-fno-builtin-memcpy"
 )
 
 add_header_library(
   memcmp_implementation
   HDRS
     memcmp_implementations.h
   DEPS
     .memory_utils

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);