Skip to content

Commit

Permalink
i#3544 RV64: Fix our memcpy return value error (#6873)
Browse files Browse the repository at this point in the history
We optimized RV64 memfuncs in PR #6800. However, there was a mistake
where the return value of memcpy was incorrectly set to ARG2 which led
to segfaults in the release build. This PR addresses this issue.
Additionally, our unit_tests did not catch this error because
`ret = our_memcpy(&i, &j, sizeof(i));` was optimized by the compiler to
`i = j; ret = &i;`. We resolved this by utilizing the `noinline` attribute.
  • Loading branch information
ksco authored Jul 11, 2024
1 parent 560092b commit 20d4548
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 10 deletions.
2 changes: 1 addition & 1 deletion core/arch/riscv64/memfuncs.asm
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ START_FILE
DECLARE_FUNC(memcpy)
GLOBAL_LABEL(memcpy:)
li t6, 32
mv t0, ARG2 /* Save dst for return. */
mv t0, ARG1 /* Save dst for return. */
copy32_:
/* When size is greater than 32, we use 4 ld/sd pairs
* to copy 4*8=32 bytes in each iteration.
Expand Down
32 changes: 23 additions & 9 deletions core/io.c
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,20 @@ test_sscanf_all_specs(void)
typedef void (*memcpy_t)(void *dst, const void *src, size_t n);
typedef void (*memset_t)(void *dst, int src, size_t n);

/* Force compiler to not optimize away our own memcpy and memset.
*/
__attribute__((noinline)) static void *
our_memcpy(void *dst, const void *src, size_t n)
{
return memcpy(dst, src, n);
}

__attribute__((noinline)) static void *
our_memset(void *dst, int src, size_t n)
{
return memset(dst, src, n);
}

static void
test_memcpy_offset_size(size_t src_offset, size_t dst_offset, size_t size)
{
Expand All @@ -934,7 +948,7 @@ test_memcpy_offset_size(size_t src_offset, size_t dst_offset, size_t size)
}
EXPECT(src_offset + size <= sizeof(src), 1);
EXPECT(dst_offset + size <= sizeof(dst), 1);
memcpy(dst + dst_offset, src + src_offset, size);
our_memcpy(dst + dst_offset, src + src_offset, size);
EXPECT(memcmp(dst + dst_offset, src + src_offset, size), 0);
/* Check the bytes just out of bounds, which should still be zero. */
if (dst_offset > 0)
Expand Down Expand Up @@ -962,7 +976,7 @@ test_our_memcpy(void)
}
}
/* Check that memcpy returns dst. */
ret = memcpy(&i, &j, sizeof(i));
ret = our_memcpy(&i, &j, sizeof(i));
EXPECT(ret == &i, 1);
}

Expand All @@ -975,7 +989,7 @@ test_memset_offset_size(int val, int start_offs, int end_offs)
/* Zero without memset. */
for (i = 0; i < sizeof(buf); i++)
buf[i] = 0;
memset(buf + start_offs, val, end);
our_memset(buf + start_offs, val, end);
EXPECT(is_region_memset_to_char(buf + start_offs, end, val), 1);
if (start_offs > 0)
EXPECT(buf[start_offs - 1], 0);
Expand All @@ -998,7 +1012,7 @@ test_our_memset(void)
}
}
/* Check that memset returns dst. */
ret = memset(&i, -1, sizeof(i));
ret = our_memset(&i, -1, sizeof(i));
EXPECT(ret == &i, 1);
}

Expand All @@ -1014,15 +1028,15 @@ our_memcpy_vs_libc(void)
memcpy_t glibc_memcpy = (memcpy_t)dlsym(RTLD_NEXT, "memcpy");
uint64 our_memcpy_start, our_memcpy_end, our_memcpy_time;
uint64 libc_memcpy_start, libc_memcpy_end, libc_memcpy_time;
memset(src, -1, alloc_size);
memset(dst, 0, alloc_size);
our_memset(src, -1, alloc_size);
our_memset(dst, 0, alloc_size);

int tests_size[] = { 1, 4, 128, 512, 8192, alloc_size };
int j;
for (j = 0; j < sizeof(tests_size) / sizeof(int); j++) {
our_memcpy_start = query_time_millis();
for (i = 0; i < loop_count; i++) {
memcpy(src, dst, tests_size[j]);
our_memcpy(src, dst, tests_size[j]);
}
our_memcpy_end = query_time_millis();

Expand Down Expand Up @@ -1066,8 +1080,8 @@ our_memset_vs_libc(void)

our_memset_start = query_time_millis();
for (i = 0; i < loop_count; i++) {
memset(src, -1, alloc_size);
memset(dst, 0, alloc_size);
our_memset(src, -1, alloc_size);
our_memset(dst, 0, alloc_size);
}
our_memset_end = query_time_millis();

Expand Down

0 comments on commit 20d4548

Please sign in to comment.