[libc-commits] [libc] [libc] Fix load_aligned big-endian handling. (PR #185937)

[via libc-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libc

Author: Simon Tatham (statham-arm)

<details>
<summary>Changes</summary>

The variadic template helper `load_aligned` performs a specific case of an unaligned integer load, by loading a sequence of integers from memory at addresses expected to be aligned, and glues the results back together with shifts and ORs into an output.

The implementation works by performing the first load, recursing on a shorter parameter type list for the rest, and recombining via

  first | (rest << size_of_first)           // if little-endian
  (first << size_of_first) | rest           // if big-endian

But the big-endian case is wrong: it should shift left by the size of the _rest_ of the types, not the size of the first. In the case where you load 8, 16 and 8 bits from an odd address, you want

  (first_byte << 24) | (middle_halfword << 8) | (last_byte)

but in fact we were calculating

  (first_byte << 8) | (middle_halfword << 16) | (last_byte)

leading to three out of every four bytes being permuted in a memcpy from an odd to an even address.

---
Full diff: https://github.com/llvm/llvm-project/pull/185937.diff


2 Files Affected:

- (modified) libc/src/string/memory_utils/utils.h (+9-4) 
- (modified) libc/test/src/string/memory_utils/utils_test.cpp (+18) 


``````````diff
diff --git a/libc/src/string/memory_utils/utils.h b/libc/src/string/memory_utils/utils.h
index 86ff4f12e8c26..b0cfd29ca968c 100644
--- a/libc/src/string/memory_utils/utils.h
+++ b/libc/src/string/memory_utils/utils.h
@@ -233,13 +233,18 @@ LIBC_INLINE ValueType load_aligned(CPtr src) {
   static_assert(sizeof(ValueType) >= (sizeof(T) + ... + sizeof(TS)));
   const ValueType value = load<T>(assume_aligned<sizeof(T)>(src));
   if constexpr (sizeof...(TS) > 0) {
-    constexpr size_t SHIFT = sizeof(T) * 8;
     const ValueType next = load_aligned<ValueType, TS...>(src + sizeof(T));
-    if constexpr (Endian::IS_LITTLE)
+    if constexpr (Endian::IS_LITTLE) {
+      // T goes at the bottom of the output, so shift up everything
+      // else by the number of bits in T.
+      constexpr size_t SHIFT = sizeof(T) * 8;
       return value | (next << SHIFT);
-    else if constexpr (Endian::IS_BIG)
+    } else if constexpr (Endian::IS_BIG) {
+      // T goes at the top of the output, so shift it up by the number
+      // of bits in everything else that goes below it.
+      constexpr size_t SHIFT = (0 + ... + sizeof(TS)) * 8;
       return (value << SHIFT) | next;
-    else
+    } else
       static_assert(cpp::always_false<T>, "Invalid endianness");
   } else {
     return value;
diff --git a/libc/test/src/string/memory_utils/utils_test.cpp b/libc/test/src/string/memory_utils/utils_test.cpp
index 2ab2e8d3ad867..8ad0dce5062c6 100644
--- a/libc/test/src/string/memory_utils/utils_test.cpp
+++ b/libc/test/src/string/memory_utils/utils_test.cpp
@@ -138,4 +138,22 @@ TEST(LlvmLibcUtilsTest, LoadStoreAligned) {
   }
 }
 
+TEST(LlvmLibcUtilsTest, LoadStoreAlignedOddAddress) {
+  const uint8_t inbuf[8] = {0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF};
+  uint8_t outbuf[8];
+
+  const uint32_t loaded = load_aligned<uint32_t, uint8_t, uint16_t, uint8_t>(
+      reinterpret_cast<CPtr>(inbuf + 1));
+  if constexpr (Endian::IS_LITTLE) {
+    EXPECT_EQ(loaded, uint32_t(0x89674523));
+  } else if constexpr (Endian::IS_BIG) {
+    EXPECT_EQ(loaded, uint32_t(0x23456789));
+  }
+  store_aligned<uint32_t, uint8_t, uint16_t, uint8_t>(
+      loaded, reinterpret_cast<Ptr>(outbuf + 1));
+
+  for (size_t i = 1; i < 5; ++i)
+    EXPECT_EQ(outbuf[i], inbuf[i]);
+}
+
 } // namespace LIBC_NAMESPACE_DECL

``````````

</details>


https://github.com/llvm/llvm-project/pull/185937