Page MenuHomePhabricator

[libc] Add memmove implementation.
ClosedPublic

Authored by cheng.w on Dec 14 2020, 12:46 AM.

Details

Summary

Use memcpy rather than copying bytes one by one, for there might be large
size structs to move.

Diff Detail

Event Timeline

cheng.w created this revision.Dec 14 2020, 12:46 AM
cheng.w requested review of this revision.Dec 14 2020, 12:46 AM
cheng.w updated this revision to Diff 315034.Wed, Jan 6, 6:02 PM
cheng.w edited the summary of this revision. (Show Details)
cheng.w removed a reviewer: sivachandra.
cheng.w added a subscriber: sivachandra.

Sorry for the delay. And yes, you did the right thing, @gchatelet is the best reviewer for this. Overall, this LGTM, but I prefer @gchatelet to approve it.

gchatelet requested changes to this revision.Mon, Jan 11, 5:20 AM

My apologies for the late reply. I've made a bunch of comments.

libc/src/string/memmove.cpp
22

assert(count <= SSIZE_MAX);

@sivachandra I don't remember if we allow asserts in the library?

23

nit: subscript operator is clearer IMHO dest_m[offset] = src_m[offset]

38

Technically the pointer difference type is ptrdiff_t, you may want to rewrite this as:

#include "src/stdlib/abs_utils.h" // __llvm_libc::integer_abs
#include <cstdint> // ptrdiff_t, PTRDIFF_MAX
...
assert(count <= PTRDIFF_MAX);
if (__llvm_libc::integer_abs(src_c - dest_c) >= static_cast<ptrdiff_t>(count))

Also update the comment above.

53–56

The code generated here seems suboptimal and quite large https://godbolt.org/z/fMnfv8.
This can be bad for icache pressure (i.e. calling memmove from time to time will evict precious cache lines from iL1).

That said, we can keep this version for now and optimize later on.

libc/test/src/string/memmove_test.cpp
43

typo

This revision now requires changes to proceed.Mon, Jan 11, 5:20 AM
michaelrj added inline comments.
libc/src/string/memmove.cpp
26

This line should be LLVM_LIBC_FUNCTION(void *, memmove, (void *dest, const void *src, size_t count)) { since we recently changed the macro that's used for external functions.

cheng.w marked 4 inline comments as done.Tue, Jan 12, 2:30 AM

Address comments.

libc/src/string/memmove.cpp
38

It should be careful with pointer operations and limits. Is assert more like a debug or test macro?

53–56

Yes, it is verbose. Maybe inline asm like cld/std, rep movsb would help. I leave a TODO here.

cheng.w updated this revision to Diff 316032.Tue, Jan 12, 2:31 AM
cheng.w marked an inline comment as done.
gchatelet accepted this revision.Tue, Jan 12, 3:46 AM

LGTM, can you please wait for @sivachandra answer about using assert in code before submitting? Thx!

libc/src/string/memmove.cpp
32

remove or update

This revision is now accepted and ready to land.Tue, Jan 12, 3:46 AM

LGTM, can you please wait for @sivachandra answer about using assert in code before submitting? Thx!

Sorry I missed this earlier. But, I don't see any assert now. Was it removed?

I don't think there is a hard and fast rule about assert but we should try to avoid them in order not to couple parts of the libc. For error conditions, if we align with the standards' requirements and add tests for them, I think that is sufficient.

But of course, if there is real benefit got by using an assert, we can use it.

libc/src/string/memmove.cpp
15

I think you should add a DEPS for this header in the CMake file?

cheng.w updated this revision to Diff 316547.Wed, Jan 13, 6:38 PM
cheng.w marked 3 inline comments as done.
sivachandra accepted this revision.Wed, Jan 13, 8:41 PM
sivachandra added inline comments.
libc/src/string/memmove.cpp
15

Newline?

cheng.w updated this revision to Diff 316829.Thu, Jan 14, 6:53 PM
cheng.w marked an inline comment as done.
This revision was landed with ongoing or failed builds.Thu, Jan 14, 8:08 PM
This revision was automatically updated to reflect the committed changes.