custom memmove implementation proposal. #593

devnexen · 2023-02-19T13:48:57Z

mostly like memcpy with optional bound checking but capable of handling overlapping cases thus using
reverse copy instead.

mjp41

Thanks for doing this. I think the test coverage needs a bit of work. It would be nice to vary the sizes and directions of overlap to make sure the code functions correctly.

mjp41 · 2023-02-20T16:46:13Z

src/test/func/memcpy/func-memcpy.cc

+  my_memmove(ptr, s, sz * sizeof(unsigned int));
+  for (size_t i = 0; i < sz; ++i)
+  {
+    EXPECT(ptr[i] == s[i], "overlap error: {} {}", ptr[i], s[i]);


Suggested change

EXPECT(ptr[i] == s[i], "overlap error: {} {}", ptr[i], s[i]);

EXPECT(ptr[i] == i, "overlap error: {} {}", ptr[i], i);

I think s[i] can be corrupted by the memmove. Better to use the original value of i here.

Would be more complex if you make this test do both directions.

mjp41 · 2023-02-20T16:51:30Z

src/test/func/memcpy/func-memcpy.cc

+  }
+  my_memmove(&s[2], &s[4], sizeof(s[0]));
+  EXPECT(s[2] == s[4], "overlap error: {} {}", s[2], s[4]);
+  my_memmove(&s[15], &s[5], sizeof(s[0]));


Isn't s[15] out of bounds when size == 8?

ah right I forgot to change in the beginning the sizes set were fixed :-)

mjp41 · 2023-02-20T16:53:14Z

src/test/func/memcpy/func-memcpy.cc

+void check_overlaps(size_t size)
+{
+  START_TEST("memmove overlaps, size {}", size);
+  auto* s = static_cast<unsigned int*>(my_malloc(size * sizeof(unsigned int)));
+  auto sz = size / 2;
+  for (size_t i = 0; i < size; ++i)
+  {
+    s[i] = static_cast<unsigned int>(i);
+  }
+  my_memmove(&s[2], &s[4], sizeof(s[0]));
+  EXPECT(s[2] == s[4], "overlap error: {} {}", s[2], s[4]);
+  my_memmove(&s[15], &s[5], sizeof(s[0]));
+  EXPECT(s[15] == s[5], "overlap error: {} {}", s[15], s[5]);
+  auto ptr = s;
+  my_memmove(ptr, s, size * sizeof(s[0]));
+  EXPECT(ptr == s, "overlap error: {} {}", ptr, s);
+  my_free(s);


The first part of this test feels like it is debug to get things working. The second part has a lot of uniformity. Perhaps, just drop the first part, or make it a separate test: check_overlaps1 and check_overlaps2.

mjp41 · 2023-02-20T16:55:14Z

src/test/func/memcpy/func-memcpy.cc

+  {
+    s[i] = static_cast<unsigned int>(i);
+  }
+  ptr = s + sz;


This only provides an overlap in one direction.

Suggested change

ptr = s + sz;

auto dst = after ? s + sz : s;

auto src = after ? s : s + sz;

and then use src and dst below?

mjp41 · 2023-02-20T17:01:31Z

src/snmalloc/global/memcpy.h

+      return report_fatal_bounds_error(
+        dst, len, "memmove with destination out of bounds of heap allocation");
+
+    if (dst > src)


Would

Suggested change

if (dst > src)

if ((dst - src) < len)

be a better check? Currently it doesn't check that there is actually an overlap? @davidchisnall

I guess my concern here is because you have dropped to a slow byte by byte copy. Half of all non-overlapping memmoves will be slow.

right .. I ll follow your suggestion.

mjp41

Thanks for fixing the tests.

mjp41 · 2023-02-23T20:46:14Z

src/test/func/memcpy/func-memcpy.cc

+void check_overlaps2(size_t size)
+{
+  START_TEST("memmove overlaps2, size {}", size);
+  auto sz = size / 2;


I wonder if it would be good to separate the offset from the size being copied. The current test only covers very small overlaps.

Suggested change

auto sz = size / 2;

auto sz = size / 2;

auto offset = size / 3;

Then use offset in the calculations using after?

mjp41 · 2023-02-23T20:49:03Z

src/snmalloc/global/memcpy.h

+      return report_fatal_bounds_error(
+        dst, len, "memmove with destination out of bounds of heap allocation");
+
+    if (dst > src)


I guess my concern here is because you have dropped to a slow byte by byte copy. Half of all non-overlapping memmoves will be slow.

mjp41 · 2023-02-28T09:56:19Z

@devnexen sorry for the delay in running the CI. It looks like there are a few failures in the tests, and clangformat is not happy.

devnexen · 2023-02-28T21:29:04Z

weird as the issue seem about check_bounds about using memcpy ?
Also my clang-format(9) does not report format issue..

mjp41 · 2023-03-01T09:48:50Z

I think it is the last line here:

Check fail: dst[u] == i in /home/runner/work/snmalloc/snmalloc/src/test/func/memcpy/func-memcpy.cc on 200  
in test void check_overlaps2(size_t) [after = false] overlap error: 0x4 0x2

https://github.com/microsoft/snmalloc/actions/runs/4256879925/jobs/7476767702#step:9:16563

The clangformat might be it has cached an old file list. If you try rerunning cmake, and then the clangformat target?

mostly like memcpy with optional bound checking but capable of handling overlapping cases thus using reverse copy instead.

mjp41

LGTM

This reverts commit 01885f5.

* Revert "custom memmove implementation proposal. (#593)" This reverts commit 01885f5. * disable memmove fuzzing

mjp41 reviewed Feb 20, 2023

View reviewed changes

mjp41 reviewed Feb 23, 2023

View reviewed changes

devnexen force-pushed the memmove_impl branch from 19db257 to 2aa9388 Compare February 23, 2023 21:10

devnexen force-pushed the memmove_impl branch 2 times, most recently from be22da7 to 9065f38 Compare March 8, 2023 21:29

devnexen force-pushed the memmove_impl branch 2 times, most recently from c14ddfd to 00f84d1 Compare March 13, 2023 21:01

devnexen force-pushed the memmove_impl branch from 00f84d1 to a38415a Compare November 2, 2024 15:10

devnexen marked this pull request as ready for review November 2, 2024 15:10

custom memmove implementation proposal.

ca9d282

mostly like memcpy with optional bound checking but capable of handling overlapping cases thus using reverse copy instead.

devnexen force-pushed the memmove_impl branch from a38415a to ca9d282 Compare November 16, 2024 07:26

mjp41 approved these changes Nov 16, 2024

View reviewed changes

mjp41 enabled auto-merge (squash) November 16, 2024 07:34

mjp41 merged commit 01885f5 into microsoft:main Nov 16, 2024
52 checks passed

mjp41 mentioned this pull request Nov 19, 2024

fix snmalloc::memmove reverse block copy. #689

Closed

devnexen added a commit to devnexen/snmalloc that referenced this pull request Nov 20, 2024

Revert "custom memmove implementation proposal. (microsoft#593)"

006ce12

This reverts commit 01885f5.

mjp41 mentioned this pull request Nov 20, 2024

Revert "custom memmove implementation proposal. (#593)" #692

Merged

mjp41 pushed a commit that referenced this pull request Nov 20, 2024

Revert "custom memmove implementation proposal. (#593)" (#692)

fd560d4

* Revert "custom memmove implementation proposal. (#593)" This reverts commit 01885f5. * disable memmove fuzzing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom memmove implementation proposal. #593

custom memmove implementation proposal. #593

devnexen commented Feb 19, 2023

mjp41 left a comment

mjp41 Feb 20, 2023

mjp41 Feb 20, 2023

mjp41 Feb 20, 2023

devnexen Feb 20, 2023

mjp41 Feb 20, 2023

mjp41 Feb 20, 2023

mjp41 Feb 20, 2023

mjp41 Feb 23, 2023

devnexen Feb 23, 2023

mjp41 left a comment

mjp41 Feb 23, 2023

mjp41 Feb 23, 2023

mjp41 commented Feb 28, 2023

devnexen commented Feb 28, 2023

mjp41 commented Mar 1, 2023

mjp41 left a comment

	EXPECT(ptr[i] == s[i], "overlap error: {} {}", ptr[i], s[i]);
	EXPECT(ptr[i] == i, "overlap error: {} {}", ptr[i], i);

	ptr = s + sz;
	auto dst = after ? s + sz : s;
	auto src = after ? s : s + sz;

	auto sz = size / 2;
	auto sz = size / 2;
	auto offset = size / 3;

custom memmove implementation proposal. #593

custom memmove implementation proposal. #593

Conversation

devnexen commented Feb 19, 2023

mjp41 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjp41 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjp41 commented Feb 28, 2023

devnexen commented Feb 28, 2023

mjp41 commented Mar 1, 2023

mjp41 left a comment

Choose a reason for hiding this comment