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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/string/memory_utils/elements.h:515
-#elif __has_builtin(__builtin_memcpy)
-    __builtin_memcpy(dst, src, SIZE);
 #else
----------------
gchatelet wrote:
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115542