Copilot commented on code in PR #3360:
URL: https://github.com/apache/brpc/pull/3360#discussion_r3487438178
##########
src/butil/third_party/snappy/snappy-stubs-internal.h:
##########
@@ -164,6 +164,16 @@ inline void UNALIGNED_STORE64(void *p, uint64_t v) {
// This can be more efficient than UNALIGNED_LOAD64 + UNALIGNED_STORE64
// on some platforms, in particular ARM.
inline void UnalignedCopy64(const void *src, void *dst) {
+#if defined(__riscv) && __riscv_xlen == 64
+ // RISC-V optimized: single ld/sd pair for 8-byte copy
+ uint64_t tmp;
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ "sd %0, %2\n\t"
+ : "=&r"(tmp)
+ : "m"(*(const uint64_t*)src), "m"(*(uint64_t*)dst)
+ : "memory");
+#else
Review Comment:
`UnalignedCopy64` is expected to work for unaligned addresses (the file
otherwise uses memcpy-based UNALIGNED_LOAD/STORE on architectures without
unaligned support), but the RV64 specialization uses `ld`/`sd` unconditionally.
This can fault on misaligned `src`/`dst` and also forms potentially misaligned
`uint64_t` lvalues in the asm constraints. Consider only using `ld`/`sd` when
both pointers are 8-byte aligned and otherwise falling back to the existing
memcpy-based implementation.
##########
src/butil/third_party/snappy/snappy.cc:
##########
@@ -97,9 +97,33 @@ static const uint32_t kMaximumTagLength = 5; //
COPY_4_BYTE_OFFSET plus the act
// or memmove().
static inline void IncrementalCopy(const char* src, char* op, ssize_t len) {
assert(len > 0);
+#if defined(__riscv) && __riscv_xlen == 64
+ // RISC-V optimized: use 8-byte copies when possible
+ if (len >= 8 && (op - src >= 8 || src - op >= 8)) {
+ // Non-overlapping or safe overlap: copy 8 bytes at a time
+ do {
+ uint64_t tmp;
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ "sd %0, %2\n\t"
+ : "=&r"(tmp)
+ : "m"(*(const uint64_t*)src), "m"(*(uint64_t*)op)
+ : "memory");
+ src += 8;
+ op += 8;
+ len -= 8;
+ } while (len >= 8);
+ }
Review Comment:
The RISC-V path uses `ld`/`sd` on `src`/`op` without checking 8-byte
alignment, but `IncrementalCopy` operates on `char*` and may be called with
unaligned pointers. On RV64 implementations that trap on misaligned 64-bit
accesses, this can crash; additionally, using `"m"(*(const
uint64_t*)src)`/`"m"(*(uint64_t*)op)` forms potentially misaligned `uint64_t`
lvalues (UB) and doesn’t model the store as an output operand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
