This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add strncpy implementation.
ClosedPublic

Authored by cheng.w on Nov 12 2020, 7:28 PM.

Details

Summary

Add libc strncpy implementation.

Diff Detail

Event Timeline

cheng.w created this revision.Nov 12 2020, 7:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 7:28 PM
cheng.w requested review of this revision.Nov 12 2020, 7:28 PM
cheng.w edited the summary of this revision. (Show Details)Nov 12 2020, 7:33 PM
cheng.w edited the summary of this revision. (Show Details)Nov 12 2020, 8:10 PM

Thanks for the patch. I have few nits and a question inline.

libc/src/string/strncpy.cpp
19

WDYT about a memcpy followed by a memset (for the null characters)? Both these functions in libcs are optimized in some manner so we will be leveraging that. We will need an additional strlen operation though.

libc/test/src/string/strncpy_test.cpp
1

Nit: Fix this. You probably need a space after strncpy.

12

Nit: May be name it SameSrcSizeAndCopySize instead of ReturnRightValue?

20

Nit: May be name it CopySizeNotEqualToSrcSize instead of OffsetDest.

Thanks Siva.

libc/src/string/strncpy.cpp
19

By referring to strcpy, I used memcpy and strlen at first.
But it seems that this method avoids the overhead of:

  • strlen goes through src for the length.
  • function calls.

I am not sure whick method is more appropriate. So I upload it for suggestions.

Thanks for your patience.

libc/src/string/strncpy.cpp
19

By referring to strcpy, I used memcpy and strlen at first.
But it seems that this method avoids the overhead of:

  • strlen goes through src for the length.
  • function calls.

I am not sure whick method is more appropriate. So I upload it for suggestions.

I am not opposed to any approach. At the same time, I do not feel qualified enough to drive an opinion. So, adding @gchatelet for guidance.

Thx for the patch!

libc/src/string/strncpy.cpp
19

It's hard to be assertive on which one is best here. It certainly depends on how strncpy is used (buffer sizes, value of n, size of the zeroing, ...).
I'd say let's keep the implementation simple and optimize if we see performance issues.

22

There are two modes for computing strncpy:

  1. copy up until \0 is found
  2. fill with \0

You can only ever go from 1 to 2, never the opposite.
j is only used in 1 and is incremented at each loop iteration.

So it seems to me that you don't need j, you can just use i.

I've been fiddling a bit with different implementations (https://godbolt.org/z/Eb9xq9) and the following one seems simple and nice

char *strncpy(char *__restrict dest, const char *__restrict src, size_t n) {
  size_t i = 0;
  for (; i < n && src[i] != '\0'; ++i) dest[i] = src[i];
  for (; i < n; ++i) dest[i] = '\0';
  return dest;
}

Added bonus, the compiler recognizes for (; i < n; ++i) dest[i] = '\0'; as a memset which lowers this function's footprint. IMHO the filling with zero part is not the most used part of the algorithm so it's nice to outline it.

24

Can you use dest[i] instead of *(dest + i)? I believe it's more readable.
Here and above (same for src).

libc/test/src/string/strncpy_test.cpp
44

Would you mind splitting this into 3 tests to ease readability?
Also a test fixture like so would improve readability.

struct StrncpyFixture : public __llvm_libc::testing::Test {
  void check_strncpy(__llvm_libc::cpp::MutableArrayRef<char> dst,
                     __llvm_libc::cpp::ArrayRef<char> src, size_t n,
                     __llvm_libc::cpp::ArrayRef<char> expected) {
    // Making sure we don't overflow buffer.
    ASSERT_GE(dst.size(), n);
    // Making sure strncpy returns dst.
    ASSERT_EQ(__llvm_libc::strncpy(dst.data(), src.data(), n), dst.data());
    // Expected must be of the same size as Dest.
    ASSERT_EQ(dst.size(), expected.size());
    for (size_t i = 0; i < expected.size(); ++i)
      ASSERT_EQ(expected[i], dst[i]);
  }
};

TEST_F(StrncpyFixture, Untouched) {
  char dst[] = {'a', 'b'};
  char src[] = {'x', '\0'};
  char expected[] = {'a', 'b'};
  check_strncpy(dst, src, 0, expected);
}

TEST_F(StrncpyFixture, CopyOne) {
  char dst[] = {'a', 'b'};
  char src[] = {'x', 'y'};
  char expected[] = {'x', 'b'}; // no \0 is appended
  check_strncpy(dst, src, 1, expected);
}

TEST_F(StrncpyFixture, CopyNull) {
  char dst[] = {'a', 'b'};
  char src[] = {'\0', 'y'};
  char expected[] = {'\0', 'b'};
  check_strncpy(dst, src, 1, expected);
}

TEST_F(StrncpyFixture, CopyPastSrc) {
  char dst[] = {'a', 'b'};
  char src[] = {'\0', 'y'};
  char expected[] = {'\0', '\0'};
  check_strncpy(dst, src, 2, expected);
}
cheng.w retitled this revision from Add strncpy implementation. to [libc] Add strncpy implementation..Nov 25 2020, 11:17 PM
cheng.w updated this revision to Diff 307761.Nov 25 2020, 11:23 PM

Take code from Chatelet. Fix some nits.

cheng.w marked 4 inline comments as done.Nov 25 2020, 11:41 PM

I am sorry to occupy too much time of you both. I initially just want to optimize it a little bit.

gchatelet accepted this revision.Nov 26 2020, 1:18 AM

Thank you for your contribution!
Please wait for @sivachandra before submitting.

This revision is now accepted and ready to land.Nov 26 2020, 1:18 AM
sivachandra accepted this revision.Nov 29 2020, 8:19 PM
This revision was automatically updated to reflect the committed changes.
cheng.w marked 5 inline comments as done.Dec 2 2020, 4:56 AM