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.

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

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

Merged
merged 2 commits into from Sep 21, 2022

Conversation

Naios
Copy link
Contributor

@Naios Naios commented Sep 10, 2022

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.

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

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 added a commit to Naios/flatbuffers that referenced this pull request Sep 19, 2022
@Naios
Copy link
Contributor Author

Naios commented Sep 19, 2022

@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
* 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

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants