Use natural alignment for prevector by laanwj · Pull Request #17530 · bitcoin/bitcoin

laanwj · 2019-11-20T09:17:21Z

#pragma pack(1) prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures.

It also triggers UBsan — see #17156 (comment) and #17510.

So remove the pragmas.

`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin#17156 (comment) and bitcoin#17510.

This can be done now that prevector is properly aligned.

maflcko · 2019-11-20T12:42:12Z

@jamesob Would be nice to see IBD performance on this?

practicalswift · 2019-11-20T15:08:57Z

Concept ACK: thanks for tackling this!

Would be nice to see IBD performance numbers.

Very glad to see that float-divide-by-zero (-fsanitize=undefined) and unsigned-integer-overflow (-fsanitize=integer) are the only suppressions left for -fsanitize=integer,undefined after the merge of this PR.

We can get rid of the float-divide-by-zero (-fsanitize=undefined) suppressions if PR #17208 is merged too. Fans of the sanitizers: please consider reviewing :)

DrahtBot · 2019-11-20T15:12:42Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17208 (Make all tests pass UBSan without using any UBSan suppressions by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jamesob · 2019-11-20T15:38:56Z

@jamesob Would be nice to see IBD performance on this?

On it!

practicalswift · 2019-12-03T10:22:56Z

@jamesob Any news on the IBD performance? It would be really nice to see if you have the time :)

laanwj · 2019-12-03T10:33:34Z

I think this is the correct thing to do no matter the performance result, but still it'd be nice to see.

practicalswift · 2019-12-03T10:42:04Z

@laanwj Agree totally. I really look forward to seeing this merged: this alignment issue creates a lot of sanitiser noise (or signal depending how you see it...) for me when doing my continuous fuzzing of Bitcoin Core :)

practicalswift · 2019-12-06T10:31:10Z

ACK 5432306 -- diff looks correct

practicalswift · 2019-12-09T21:44:14Z

People interested in getting rid of the two UBSan suppressions needed after the merge of this PR might want to review PR #17208 ("Make all tests pass UBSan without using any UBSan suppressions") too :)

laanwj · 2019-12-10T04:04:41Z

~~Wait, does this introduce new needed suppression?~~ Oh you mean the other, remaining ones

TBH it seems that there's completely no interest in this.

sipa · 2019-12-10T04:43:02Z

What is the sizeof(CScript) on x86_64 before and after this change?

laanwj · 2019-12-10T05:06:15Z

before: 32
after: 40

laanwj · 2019-12-10T05:27:35Z

Couldn't fix that by reshuffling fields either, e.g. the usual suggestion of putting big fields first

    union direct_or_indirect {
        char direct[sizeof(T) * N];
        struct {
            char* indirect;
            size_type capacity;
        };
    } _union = {};
    size_type _size = 0;

… a prevector<28, unsigned char> is still 40 bytes

sipa · 2019-12-10T05:46:45Z

See also #17060, which makes a bigger prevector layout change (avoiding the memory increase without pragma pack).

laanwj · 2019-12-10T05:49:47Z

That PR changes the layout, but it looks like it relies just as much on pragma pack(1) there.

sizeof(Coin) before: 48
sizeof(Coin) after: 56

ajtowns · 2019-12-10T06:03:45Z

This seems like it does the right thing to me fwiw:

--- a/src/prevector.h
+++ b/src/prevector.h
@@ -15,7 +15,6 @@
 #include <type_traits>
 #include <utility>
 
-#pragma pack(push, 1)
 /** Implements a drop-in replacement for std::vector<T> which stores up to N
  *  elements directly (without heap allocation). The types Size and Diff are
  *  used to store element counts, and can be any unsigned + signed type.
@@ -45,6 +44,9 @@ public:
     typedef value_type* pointer;
     typedef const value_type* const_pointer;
 
+    static_assert(N % alignof(Size) == 0, "Must have preallocation as a multiple of size");
+    static_assert(alignof(char*) % alignof(Size) == 0, "Must have Size be aligned whenever char* would be aligned");
+
     class iterator {
         T* ptr;
     public:
@@ -147,14 +149,23 @@ public:
     };
 
 private:
-    size_type _size = 0;
+
+#pragma pack(push,1)
+    // we don't want padding added here, we will ensure indirect is
+    // correctly aligned manually, which will in turn ensure capacity
+    // is aligned.
     union direct_or_indirect {
         char direct[sizeof(T) * N];
         struct {
-            size_type capacity;
             char* indirect;
+            size_type capacity;
         };
-    } _union = {};
+    };
+#pragma pack(pop)
+
+    alignas(alignof(char*)) direct_or_indirect _union = {};
+    size_type _size = 0;
+private:
 
     T* direct_ptr(difference_type pos) { return reinterpret_cast<T*>(_union.direct) + pos; }
     const T* direct_ptr(difference_type pos) const { return reinterpret_cast<const T*>(_union.direct) + pos; }
@@ -523,6 +534,5 @@ public:
         return item_ptr(0);
     }
 };
-#pragma pack(pop)

laanwj · 2019-12-10T06:16:38Z

Will leave this to you @ajtowns

practicalswift · 2019-12-10T07:27:14Z

@ajtowns Would you mind opening a PR with that suggested change? It would be nice to get this fixed :)

ajtowns · 2019-12-10T08:21:18Z

The saga continues in #17708

5f26855 test: Remove ubsan alignment suppressions (Wladimir J. van der Laan) 9d933ef prevector: avoid misaligned member accesses (Anthony Towns) Pull request description: Ensure prevector data is appropriately aligned. Earlier discussion in #17530. **Edit laanwj**: In contrast to #17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang) | Struct | (size,align) before | (size,align) after | | ------------- | ------------- | ------- | | Coin | 48, 8 | 48, 8 | | CCoinsCacheEntry | 56, 8 | 56, 8 | | CScript | 32, 1 | 32, 8 | ACKs for top commit: laanwj: ACK 5f26855 practicalswift: ACK 5f26855 jonatack: ACK 5f26855 Tree-SHA512: 98d112d6856f683d5b212410b73f3071d2994f1efb046a2418a35890aa1cf1aa7c96a960fc2e963fa15241e861093c1ea41951cf5b4b5431f88345eb1dd0a98a

5f26855 test: Remove ubsan alignment suppressions (Wladimir J. van der Laan) 9d933ef prevector: avoid misaligned member accesses (Anthony Towns) Pull request description: Ensure prevector data is appropriately aligned. Earlier discussion in bitcoin#17530. **Edit laanwj**: In contrast to bitcoin#17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang) | Struct | (size,align) before | (size,align) after | | ------------- | ------------- | ------- | | Coin | 48, 8 | 48, 8 | | CCoinsCacheEntry | 56, 8 | 56, 8 | | CScript | 32, 1 | 32, 8 | ACKs for top commit: laanwj: ACK 5f26855 practicalswift: ACK 5f26855 jonatack: ACK 5f26855 Tree-SHA512: 98d112d6856f683d5b212410b73f3071d2994f1efb046a2418a35890aa1cf1aa7c96a960fc2e963fa15241e861093c1ea41951cf5b4b5431f88345eb1dd0a98a

laanwj added the Utils/log/libs label Nov 20, 2019

test: Remove ubsan alignment suppressions

5432306

This can be done now that prevector is properly aligned.

laanwj force-pushed the 2019_11_prevector branch from 63c4d98 to 5432306 Compare November 20, 2019 09:58

fanquake requested a review from sipa November 20, 2019 15:11

laanwj closed this Dec 10, 2019

ajtowns mentioned this pull request Dec 10, 2019

prevector: avoid misaligned member accesses #17708

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

laanwj commented Nov 20, 2019

Uh oh!

maflcko commented Nov 20, 2019

Uh oh!

practicalswift commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 20, 2019

Conflicts

Uh oh!

jamesob commented Nov 20, 2019

Uh oh!

practicalswift commented Dec 3, 2019

Uh oh!

laanwj commented Dec 3, 2019

Uh oh!

practicalswift commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Dec 6, 2019

Uh oh!

practicalswift commented Dec 9, 2019

Uh oh!

laanwj commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 10, 2019

Uh oh!

laanwj commented Dec 10, 2019

Uh oh!

laanwj commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 10, 2019

Uh oh!

laanwj commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Dec 10, 2019

Uh oh!

laanwj commented Dec 10, 2019

Uh oh!

practicalswift commented Dec 10, 2019

Uh oh!

ajtowns commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Nov 20, 2019 •

edited

Loading

practicalswift commented Dec 3, 2019 •

edited

Loading

laanwj commented Dec 10, 2019 •

edited

Loading

laanwj commented Dec 10, 2019 •

edited

Loading

laanwj commented Dec 10, 2019 •

edited

Loading