Skip to content

Commit

Permalink
[C++] Fix final buffer alignment when using an array of structs
Browse files Browse the repository at this point in the history
* A struct can have an arbitrary size and therefore sizeof(struct) == alignof(struct)
  does not hold anymore as for value primitives.
* This patch fixes this by introducing alignment parameters to various
  CreateVector*/StartVector calls.
* Closes #7516
  • Loading branch information
Naios committed Sep 15, 2022
1 parent 952a2c0 commit 6de9bbb
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 15 deletions.
36 changes: 23 additions & 13 deletions include/flatbuffers/flatbuffer_builder.h
Expand Up @@ -449,7 +449,7 @@ class FlatBufferBuilder {
}
template<typename T> void PreAlign(size_t len) {
AssertScalarT<T>();
PreAlign(len, sizeof(T));
PreAlign(len, AlignOf<T>());
}
/// @endcond

Expand Down Expand Up @@ -589,11 +589,15 @@ class FlatBufferBuilder {
return PushElement(static_cast<uoffset_t>(len));
}

void StartVector(size_t len, size_t elemsize) {
void StartVector(size_t len, size_t elemsize, size_t alignment) {
NotNested();
nested = true;
PreAlign<uoffset_t>(len * elemsize);
PreAlign(len * elemsize, elemsize); // Just in case elemsize > uoffset_t.
PreAlign(len * elemsize, alignment); // Just in case elemsize > uoffset_t.
}

template<typename T> void StartVector(size_t len) {
return StartVector(len, sizeof(T), AlignOf<T>());
}

// Call this right before StartVector/CreateVector if you want to force the
Expand Down Expand Up @@ -627,7 +631,7 @@ class FlatBufferBuilder {
// If this assert hits, you're specifying a template argument that is
// causing the wrong overload to be selected, remove it.
AssertScalarT<T>();
StartVector(len, sizeof(T));
StartVector<T>(len);
if (len == 0) { return Offset<Vector<T>>(EndVector(len)); }
// clang-format off
#if FLATBUFFERS_LITTLEENDIAN
Expand Down Expand Up @@ -668,7 +672,7 @@ class FlatBufferBuilder {

template<typename T>
Offset<Vector<Offset<T>>> CreateVector(const Offset<T> *v, size_t len) {
StartVector(len, sizeof(Offset<T>));
StartVector<Offset<T>>(len);
for (auto i = len; i > 0;) { PushElement(v[--i]); }
return Offset<Vector<Offset<T>>>(EndVector(len));
}
Expand All @@ -688,7 +692,7 @@ class FlatBufferBuilder {
// an array. Instead, read elements manually.
// Background: https://isocpp.org/blog/2012/11/on-vectorbool
Offset<Vector<uint8_t>> CreateVector(const std::vector<bool> &v) {
StartVector(v.size(), sizeof(uint8_t));
StartVector<uint8_t>(v.size());
for (auto i = v.size(); i > 0;) {
PushElement(static_cast<uint8_t>(v[--i]));
}
Expand Down Expand Up @@ -762,7 +766,7 @@ class FlatBufferBuilder {
for (auto it = begin; it != end; ++it) {
buf_.scratch_push_small(CreateString(*it));
}
StartVector(size, sizeof(Offset<String>));
StartVector<Offset<String>>(size);
for (auto i = 1; i <= size; i++) {
// Note we re-evaluate the buf location each iteration to account for any
// underlying buffer resizing that may occur.
Expand All @@ -782,7 +786,7 @@ class FlatBufferBuilder {
/// where the vector is stored.
template<typename T>
Offset<Vector<const T *>> CreateVectorOfStructs(const T *v, size_t len) {
StartVector(len * sizeof(T) / AlignOf<T>(), AlignOf<T>());
StartVector(len * sizeof(T) / AlignOf<T>(), sizeof(T), AlignOf<T>());
if (len > 0) {
PushBytes(reinterpret_cast<const uint8_t *>(v), sizeof(T) * len);
}
Expand Down Expand Up @@ -1025,16 +1029,22 @@ class FlatBufferBuilder {
/// written to at a later time to serialize the data into a `vector`
/// in the buffer.
uoffset_t CreateUninitializedVector(size_t len, size_t elemsize,
uint8_t **buf) {
size_t alignment, uint8_t **buf) {
NotNested();
StartVector(len, elemsize);
StartVector(len, elemsize, alignment);
buf_.make_space(len * elemsize);
auto vec_start = GetSize();
auto vec_end = EndVector(len);
*buf = buf_.data_at(vec_start);
return vec_end;
}

FLATBUFFERS_ATTRIBUTE([[deprecated("call the version above instead")]])
uoffset_t CreateUninitializedVector(size_t len, size_t elemsize,
uint8_t **buf) {
return CreateUninitializedVector(len, elemsize, elemsize, buf);
}

/// @brief Specialized version of `CreateVector` for non-copying use cases.
/// Write the data any time later to the returned buffer pointer `buf`.
/// @tparam T The data type of the data that will be stored in the buffer
Expand All @@ -1053,7 +1063,7 @@ class FlatBufferBuilder {
template<typename T>
Offset<Vector<const T *>> CreateUninitializedVectorOfStructs(size_t len,
T **buf) {
return CreateUninitializedVector(len, sizeof(T),
return CreateUninitializedVector(len, sizeof(T), AlignOf<T>(),
reinterpret_cast<uint8_t **>(buf));
}

Expand All @@ -1064,7 +1074,7 @@ class FlatBufferBuilder {
Offset<Vector<T>> CreateVectorScalarCast(const U *v, size_t len) {
AssertScalarT<T>();
AssertScalarT<U>();
StartVector(len, sizeof(T));
StartVector<T>(len);
for (auto i = len; i > 0;) { PushElement(static_cast<T>(v[--i])); }
return Offset<Vector<T>>(EndVector(len));
}
Expand Down Expand Up @@ -1173,7 +1183,7 @@ class FlatBufferBuilder {
// Allocates space for a vector of structures.
// Must be completed with EndVectorOfStructs().
template<typename T> T *StartVectorOfStructs(size_t vector_size) {
StartVector(vector_size * sizeof(T) / AlignOf<T>(), AlignOf<T>());
StartVector(vector_size * sizeof(T) / AlignOf<T>(), sizeof(T), AlignOf<T>());
return reinterpret_cast<T *>(buf_.make_space(vector_size * sizeof(T)));
}

Expand Down
4 changes: 3 additions & 1 deletion src/idl_parser.cpp
Expand Up @@ -1614,6 +1614,7 @@ CheckedError Parser::ParseVector(const Type &type, uoffset_t *ovalue,
});
ECHECK(err);

const size_t alignment = InlineAlignment(type);
const size_t len = count * InlineSize(type) / InlineAlignment(type);
const size_t elemsize = InlineAlignment(type);
const auto force_align = field->attributes.Lookup("force_align");
Expand All @@ -1623,7 +1624,8 @@ CheckedError Parser::ParseVector(const Type &type, uoffset_t *ovalue,
if (align > 1) { builder_.ForceVectorAlignment(len, elemsize, align); }
}

builder_.StartVector(len, elemsize);
// TODO Fix using element alignment as size (`elemsize`)!
builder_.StartVector(len, elemsize, alignment);
for (uoffset_t i = 0; i < count; i++) {
// start at the back, since we're building the data backwards.
auto &val = field_stack_.back().first;
Expand Down
3 changes: 2 additions & 1 deletion src/reflection.cpp
Expand Up @@ -689,9 +689,10 @@ Offset<const Table *> CopyTable(FlatBufferBuilder &fbb,
FLATBUFFERS_FALLTHROUGH(); // fall thru
default: { // Scalars and structs.
auto element_size = GetTypeSize(element_base_type);
auto element_alignment = element_size; // For primitive elements
if (elemobjectdef && elemobjectdef->is_struct())
element_size = elemobjectdef->bytesize();
fbb.StartVector(vec->size(), element_size);
fbb.StartVector(vec->size(), element_size, element_alignment);
fbb.PushBytes(vec->Data(), element_size * vec->size());
offset = fbb.EndVector(vec->size());
break;
Expand Down

0 comments on commit 6de9bbb

Please sign in to comment.