This is an archive of the discontinued LLVM Phabricator instance.

[libc] fix memcpy builtin looping
ClosedPublic

Authored by michaelrj on Dec 10 2021, 10:58 AM.

Details

Summary

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

Diff Detail

Event Timeline

michaelrj created this revision.Dec 10 2021, 10:58 AM
michaelrj requested review of this revision.Dec 10 2021, 10:58 AM
sivachandra added inline comments.Dec 10 2021, 11:25 AM
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 marked an inline comment as done.

remove unnecessary compile options

michaelrj edited the summary of this revision. (Show Details)Dec 10 2021, 11:53 AM
sivachandra accepted this revision.Dec 10 2021, 12:06 PM
sivachandra added inline comments.
libc/cmake/modules/LLVMLibCLibraryRules.cmake
139

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

This revision is now accepted and ready to land.Dec 10 2021, 12:06 PM
lntue accepted this revision.Dec 10 2021, 12:14 PM
michaelrj updated this revision to Diff 393581.Dec 10 2021, 1:32 PM
michaelrj marked an inline comment as done.

fix small bit I missed

gchatelet accepted this revision.Dec 12 2021, 2:31 AM
gchatelet added inline comments.
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 added inline comments.Dec 12 2021, 3:01 PM
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 added inline comments.Dec 13 2021, 11:29 AM
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 added inline comments.Dec 13 2021, 12:21 PM
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.

This revision was automatically updated to reflect the committed changes.