Fix unaligned load and stores: (#4528) #4531

seelabs · 2023-05-22T14:18:38Z

Unaligned load and stores are supported by both intel and arm CPUs, however, this is UB in C++. Replacing this with a memcpy fixes the undefined behavior and the compiled assembly code is equivalent to the original (so there is no penalty to using memcpy).

Fix #4528

[ x] Bug fix (non-breaking change which fixes an issue)

seelabs · 2023-05-22T15:09:19Z

Note: I pushed a separate commit for the xxhash fix. I think the SlabAllocator fix should be uncontroversial, but the xxhash fix should be considered separately.

a-noni-mousse · 2023-05-22T17:38:11Z

src/ripple/beast/hash/impl/xxhash.cpp

@@ -260,7 +260,13 @@ FORCE_INLINE U64
 XXH_readLE64_align(const void* ptr, XXH_endianess endian, XXH_alignment align)
 {
 if (align == XXH_unaligned)
- return endian == XXH_littleEndian ? A64(ptr) : XXH_swap64(A64(ptr));
+ {


The XXHASH code has special config for this called XXH_USE_UNALIGNED_ACCESS. You can see it in https:/XRPLF/rippled/blob/develop/src/ripple/beast/hash/impl/xxhash.cpp#L130 and maybe you can set this for compiling without making the changes to the external code?

It looks like that would have a larger performance penalty than the change I made (although I admit I haven't looked into it that deeply). If people would rather leave it unmodified we can look at the perf penalty of XXH_USE_UNALIGNED_ACCESS; I think I'm weakly in favor of modifying xxhash with memcpy, but I'm fine backing out the change if there are objections.

Personally, I'm fine with modifying xxhash with memcpy(). We've had this copy of the code in our code base for nine years or so. And it looks like our copy is already not quite pristine. Putting the memcpy() where we know we need it makes the patch easy to understand. Messing with the macro introduces more variables and makes the change riskier.

okey
I think that doing this change changse the external files from the XXHASH code and making future upgrades more tricky. But this is frequently done in this code base anyways and not updating often like with the very old secp256k1 lib so maybe its okey for this also!

I wonder whether using xxhash makes sense. For a lot of what's being hashed, the data is already effectively random (SHA-256 hashes and the like) so using a great hash to hash it again seems a little pointless, especially if we're having to introduce a dependency (that, as noted, isn't really kept up to date).

I agree with Nik that hashing something that's already a SHA-256 hash doesn't add value. We could look to see if we could remove XXHASH. I'll make a issue.

Edit: Issue is here: #4547

a-noni-mousse · 2023-05-22T17:41:44Z

src/ripple/basics/SlabAllocator.h

-#include <boost/align.hpp>
-#include <boost/container/static_vector.hpp>
-#include <boost/predef.h>
-


Is the predef ok to remove from here because the code uses it for BOOST_OS_LINUX on next ligne. Is it maybe better to remove the linux special code?

These includes are still here, I just moved them earlier in the file. (We usually put boost includes should come before system includes). This isn't needed for this patch, but I thought I'd clean that up while I was edit this file.

As for removing the linux specific code: It looks like Nik is ambivalent about the hint, and it's possible he may reconsider that, but I don't think we should touch that in this patch.

a-noni-mousse · 2023-05-22T17:42:02Z

src/ripple/basics/SlabAllocator.h

@@ -76,7 +78,9 @@ class SlabAllocator

 while (data + item <= p_ + size_)
 {
- *reinterpret_cast<std::uint8_t**>(data) = l_;
+ // Use memcpy to avoid unaligned UB


maybe use the std:: prefix for memcpy

Fixed in a553a13 [fold] Replace memcpy with std::memcpy

Edit: I also confirmed the compiler optimized std::memcpy the same way it did memcpy (ref: https://godbolt.org/z/4E1YT4Gs6)

Looks fine.

scottschurr

👍 Thanks for doing this. The added memcpy()s all look right to me. And I especially appreciate the comments indicating why the memcpy()s are there. Additionally, code coverage is excellent on all of the changed lines.

Have you had a chance to run UBSan on the modified code? It would be great if these changes fix the reports. But even if it doesn't fix the UBSan reports I think these are all good changes.

seelabs · 2023-05-22T20:50:24Z

@scottschurr Yes, I have run the sanitizer, this does not fix all the issues. The remaining issues are either in test code or part of nubd. I would like to get this down to zero, but this is a good first step.

/home/swd/projs/ripple/ours/src/test/app/ValidatorSite_test.cpp:177:16: runtime error: load of value 224, which is not a valid value for type 'bool'
/home/swd/projs/ripple/ours/src/test/beast/LexicalCast_test.cpp:41:24: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
/home/swd/projs/ripple/ours/src/test/beast/LexicalCast_test.cpp:41:24: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x561843240f44 for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x561843240f4c for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x561843240f54 for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x561843240f5c for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x55aa1f0969c4 for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x55aa1f0969cc for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x55aa1f0969d4 for type 'const uint64_t', which requires 8 byte alignment
/home/swd/.conan/data/nudb/2.0.8/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/nudb/detail/xxhash.hpp:168:53: runtime error: load of misaligned address 0x55aa1f0969dc for type 'const uint64_t', which requires 8 byte alignment
/home/swd/projs/ripple/ours/src/test/app/ValidatorSite_test.cpp:593:60: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'

scottschurr · 2023-05-22T20:55:52Z

@seelabs, sorry, I wasn't clear. I didn't expect these changes to fix all the issues. I just wanted to confirm that the memcpy()s fixed up the specific problems that we wanted to address in this pull request. Sounds like, yes they do. That's great news! Yes, an excellent first step. Thanks for doing it!

nbougalis · 2023-05-28T22:06:25Z

src/ripple/beast/hash/impl/xxhash.cpp

@@ -260,7 +260,13 @@ FORCE_INLINE U64
 XXH_readLE64_align(const void* ptr, XXH_endianess endian, XXH_alignment align)
 {
 if (align == XXH_unaligned)
- return endian == XXH_littleEndian ? A64(ptr) : XXH_swap64(A64(ptr));
+ {


I wonder whether using xxhash makes sense. For a lot of what's being hashed, the data is already effectively random (SHA-256 hashes and the like) so using a great hash to hash it again seems a little pointless, especially if we're having to introduce a dependency (that, as noted, isn't really kept up to date).

nbougalis · 2023-05-28T22:08:31Z

src/ripple/basics/SlabAllocator.h

@@ -76,7 +78,9 @@ class SlabAllocator

 while (data + item <= p_ + size_)
 {
- *reinterpret_cast<std::uint8_t**>(data) = l_;
+ // Use memcpy to avoid unaligned UB


Looks fine.

intelliot · 2023-05-28T23:14:44Z

I'll default to holding this until 1.12 (expected ~Sept 2023) unless anyone comments that they would like to see it included in 1.11 (expected June 2023).

Unaligned load and stores are supported by both intel and arm CPUs, however, this is UB in C++. Replacing this with a `memcpy` fixes the undefined behavior and the compiled assembly code is equivalent to the original (so there is no penalty to using memcpy).

seelabs · 2023-05-30T15:32:27Z

Squahsed and forced pushed.

seelabs · 2023-05-30T15:39:18Z

@intelliot I'm fine holding this until 1.12

intelliot · 2023-05-31T18:57:46Z

This was discussed today and it is OK to include in 1.11. I intend to merge it shortly.

seelabs requested review from nbougalis, HowardHinnant and scottschurr May 22, 2023 14:18

seelabs assigned HowardHinnant and scottschurr May 22, 2023

a-noni-mousse reviewed May 22, 2023

View reviewed changes

scottschurr approved these changes May 22, 2023

View reviewed changes

intelliot linked an issue May 24, 2023 that may be closed by this pull request

[ubsan] load of misaligned address in ripple/ours/src/ripple/basics/SlabAllocator.h #4528

Closed

intelliot requested a review from drlongle May 25, 2023 05:59

nbougalis approved these changes May 28, 2023

View reviewed changes

intelliot added this to the 1.12 milestone May 28, 2023

seelabs force-pushed the misaligned-ops branch from a553a13 to b9c66ab Compare May 30, 2023 15:31

seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 30, 2023

HowardHinnant approved these changes May 31, 2023

View reviewed changes

intelliot removed this from the 1.12 milestone May 31, 2023

intelliot merged commit f709311 into XRPLF:develop May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unaligned load and stores: (#4528) #4531

Fix unaligned load and stores: (#4528) #4531

seelabs commented May 22, 2023 •

edited by intelliot

Loading

seelabs commented May 22, 2023

a-noni-mousse May 22, 2023

seelabs May 22, 2023

scottschurr May 22, 2023

a-noni-mousse May 22, 2023

nbougalis May 28, 2023

seelabs May 30, 2023 •

edited

Loading

a-noni-mousse May 22, 2023

seelabs May 22, 2023

a-noni-mousse May 22, 2023

seelabs May 22, 2023 •

edited

Loading

nbougalis May 28, 2023

scottschurr left a comment

seelabs commented May 22, 2023

scottschurr commented May 22, 2023

nbougalis May 28, 2023

nbougalis May 28, 2023

intelliot commented May 28, 2023

seelabs commented May 30, 2023

seelabs commented May 30, 2023

intelliot commented May 31, 2023

Fix unaligned load and stores: (#4528) #4531

Fix unaligned load and stores: (#4528) #4531

Conversation

seelabs commented May 22, 2023 • edited by intelliot Loading

seelabs commented May 22, 2023

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

seelabs May 30, 2023 • edited Loading

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

seelabs May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

seelabs commented May 22, 2023

scottschurr commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented May 28, 2023

seelabs commented May 30, 2023

seelabs commented May 30, 2023

intelliot commented May 31, 2023

seelabs commented May 22, 2023 •

edited by intelliot

Loading

seelabs May 30, 2023 •

edited

Loading

seelabs May 22, 2023 •

edited

Loading