#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 https://github.com/bitcoin/bitcoin/pull/17156#issuecomment-543123631 and #17510.
This can be done now that prevector is properly aligned.
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 :)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
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.
sizeof(CScript) on x86_64 before and after this change?
Couldn’t fix that by reshuffling fields either, e.g. the usual suggestion of putting big fields first
0 union direct_or_indirect {
1 char direct[sizeof(T) * N];
2 struct {
3 char* indirect;
4 size_type capacity;
5 };
6 } _union = {};
7 size_type _size = 0;
… a prevector<28, unsigned char> is still 40 bytes
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
This seems like it does the right thing to me fwiw:
0--- a/src/prevector.h
1+++ b/src/prevector.h
2@@ -15,7 +15,6 @@
3 #include <type_traits>
4 #include <utility>
5
6-#pragma pack(push, 1)
7 /** Implements a drop-in replacement for std::vector<T> which stores up to N
8 * elements directly (without heap allocation). The types Size and Diff are
9 * used to store element counts, and can be any unsigned + signed type.
10@@ -45,6 +44,9 @@ public:
11 typedef value_type* pointer;
12 typedef const value_type* const_pointer;
13
14+ static_assert(N % alignof(Size) == 0, "Must have preallocation as a multiple of size");
15+ static_assert(alignof(char*) % alignof(Size) == 0, "Must have Size be aligned whenever char* would be aligned");
16+
17 class iterator {
18 T* ptr;
19 public:
20@@ -147,14 +149,23 @@ public:
21 };
22
23 private:
24- size_type _size = 0;
25+
26+#pragma pack(push,1)
27+ // we don't want padding added here, we will ensure indirect is
28+ // correctly aligned manually, which will in turn ensure capacity
29+ // is aligned.
30 union direct_or_indirect {
31 char direct[sizeof(T) * N];
32 struct {
33- size_type capacity;
34 char* indirect;
35+ size_type capacity;
36 };
37- } _union = {};
38+ };
39+#pragma pack(pop)
40+
41+ alignas(alignof(char*)) direct_or_indirect _union = {};
42+ size_type _size = 0;
43+private:
44
45 T* direct_ptr(difference_type pos) { return reinterpret_cast<T*>(_union.direct) + pos; }
46 const T* direct_ptr(difference_type pos) const { return reinterpret_cast<const T*>(_union.direct) + pos; }
47@@ -523,6 +534,5 @@ public:
48 return item_ptr(0);
49 }
50 };
51-#pragma pack(pop)