Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) #7520

Merged
merged 2 commits into from
Sep 21, 2022
Merged

[C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) #7520

merged 2 commits into from
Sep 21, 2022

Conversation

Naios
Copy link
Contributor

@Naios Naios commented Sep 10, 2022 •
edited
Loading

EDIT: Fix for #7516, see below

@google-cla
Copy link

google-cla bot commented Sep 10, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added the c++ label Sep 10, 2022
@Naios Naios mentioned this pull request Sep 10, 2022
Closed
tungmeoo
tungmeoo reviewed Sep 12, 2022
View reviewed changes
Copy link

@tungmeoo tungmeoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@github-actions github-actions bot added the codegen Involving generating code from schema label Sep 13, 2022
@Naios Naios changed the title [WIP] [C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) [C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) Sep 13, 2022
@Naios
Copy link
Contributor Author

Naios commented Sep 13, 2022

@dbaileychess Could you review this PR please?

I fixed the issue by adding alignment parameters to functions calling StartVector.

Additionally, I spotted several expressions in the codebase of the form array_length = len * sizeof(T) / AlignOf<T>() that do not seem right to me and might need a correction.

@dbaileychess
Copy link
Collaborator

@Naios You need to add your new file to the bazel build rule, its failing CI: https://github.com/google/flatbuffers/blob/master/tests/BUILD.bazel#L7

dbaileychess
dbaileychess reviewed Sep 15, 2022
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
include/flatbuffers/flatbuffer_builder.h Show resolved Hide resolved
include/flatbuffers/flatbuffer_builder.h Outdated Show resolved Hide resolved
include/flatbuffers/flatbuffer_builder.h Outdated Show resolved Hide resolved
include/flatbuffers/flatbuffer_builder.h Outdated Show resolved Hide resolved
include/flatbuffers/flatbuffer_builder.h Show resolved Hide resolved
@dbaileychess
Copy link
Collaborator

Generally looks good to me.

@Naios
Copy link
Contributor Author

Naios commented Sep 15, 2022

@dbaileychess Thank you for your review, I have fixed the outstanding changes and optimizations.

Naios added a commit to Naios/flatbuffers that referenced this pull request Sep 19, 2022
@Naios
[C++] Add the generated test output of google#7520 to the repo ti fix…
0d41b67 
… the bazel CI.
Naios added a commit to Naios/flatbuffers that referenced this pull request Sep 19, 2022
@Naios
[C++] Add the generated test output of google#7520 to the repo to fix…
172ff25 
… the bazel CI.
@Naios
Copy link
Contributor Author

Naios commented Sep 19, 2022 •
edited
Loading

@dbaileychess From my side this PR is finished, do you know why workflow "buildkite/flatbuffers/pr" is failing without any useful output?

@dbaileychess
Copy link
Collaborator

Build kite error:

:bazel: buildifier: found 1 format issue in your WORKSPACE, BUILD and *.bzl files
Please download [buildifier 5.1.0](https://github.com/bazelbuild/buildtools/releases/tag/5.1.0) and run the following command in your workspace:

buildifier tests/BUILD.bazel

Looks like you placed the deps out of alphanumeric order.

@dbaileychess
Copy link
Collaborator

The fuzzer build is failing too, looks like due to stricter warnings on the deprecated method. Can you see if the deprecated method is being used in the fuzzer to fix it?

Naios added a commit to Naios/flatbuffers that referenced this pull request Sep 21, 2022
@Naios
[C++] Add the generated test output of google#7520 to the repo to fix…
e4916e1 
… the bazel CI.
@Naios
[C++] Add a failing unit test for google#7516 (Rare bad buffer conten…
5581ee9 
…t alignment if sizeof(T) != alignof(T))
@Naios
[C++] Fix final buffer alignment when using an array of structs
d4bc2c6 
* 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 google#7516
@Naios
Copy link
Contributor Author

Naios commented Sep 21, 2022 •
edited
Loading

@dbaileychess I probably have fixed the remaining issues, could you approve the outstanding workflows again please?

@dbaileychess dbaileychess added the release-notes The PR should be highlighted in the release notes of the next release it is in. label Sep 21, 2022
@dbaileychess dbaileychess merged commit 72aa85a into google:master Sep 21, 2022
@dbaileychess
Copy link
Collaborator

Thanks a bunch!

@bexcite bexcite mentioned this pull request Mar 29, 2023
Merged
@bexcite bexcite mentioned this pull request Jul 5, 2023
Closed
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@Naios
[C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) (g…
ef6f7e5 
…oogle#7520)

* [C++] Add a failing unit test for google#7516 (Rare bad buffer content alignment if sizeof(T) != alignof(T))

* [C++] Fix final buffer alignment when using an array of structs
* 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 google#7516
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
@Naios
[C++] Rare bad buffer content alignment if sizeof(T) != alignof(T) (g…
ea7752e 
…oogle#7520)

* [C++] Add a failing unit test for google#7516 (Rare bad buffer content alignment if sizeof(T) != alignof(T))

* [C++] Fix final buffer alignment when using an array of structs
* 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 google#7516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tungmeoo tungmeoo tungmeoo left review comments

@dbaileychess dbaileychess dbaileychess approved these changes

Assignees
No one assigned
Labels
c++ codegen Involving generating code from schema release-notes The PR should be highlighted in the release notes of the next release it is in.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Naios @dbaileychess @tungmeoo