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

Don't read protobuf static data across shared library lines directly #6979

Merged
merged 23 commits into from
Aug 21, 2024

Conversation

ThadHouse
Copy link
Member

@ThadHouse ThadHouse commented Aug 18, 2024

Reading exported data from shared objects on windows is broken. It requires __declspec(dllimport). However, this is problematic, as we use the same static libraries both from a shared and static context. So we can't just blindly apply dllimport.

The linker should have caught this, as data members are exported in a different way. However, due to a bug in native-utils, data member symbols were exposed directly. However, interacting with those data member was completely broken.

The only way we can really solve this is to just not use static data members. We're pretty good about this in WPILib itself. However, protobuf is absolutely terrible at this. There are a ton of inline functions that access global data. For the protobuf library itself, we can solve this easily enough.

However, for the generated protobuf code, this is much more problematic. The member needed to bypass the global data is private. This means using just the stock protobuf code, this problem is not solvable. But, protobuf generated code has insertion points. Those insertion points let us add our own code into the generated code via a protoc plugin. And it just so happens that an insertion point exists to add extra public methodsto the generated protobuf header. There is also an insertion point to let us add to the cpp file.

The methods we need are the getters, for unpacking protobufs. For any protobuf that has a message as a member, we generate a new wpi_x() getter (the existing one is just x(), where x is the field name). We then implement this in the cpp file. A trick we can use is in the cpp file, we can safely call the x() function, as the cpp file is in the same library as the global. Thus we can call that inline method, and not actually need to directly access any internal private state of the protobuf object.

TL;DR, all protobuf classes that have messages as fields now have a wpi_x() accessor that must be used instead of x() if you want the code to work on windows. After wpilibsuite/native-utils#212, the bad code will fail to link, rather then just fail at runtime.

CMake is kind of broken right now, as we have to manually export all protobuf functions. I'm investigating a solution to this, but currently using cmake builds on windows is likely broken (Although luckily this is our least used cmake platform for now, so thats generally ok until we can fix it).

Closes #6443

@ThadHouse
Copy link
Member Author

/format

Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a CI check similar to pregenerate.yml or upstream-utils.yml that makes sure the jar file matches the jar file generated from protoplugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll probably add that in a later PR. Right now we need to work on getting this in because we're very broken right now.

@ThadHouse ThadHouse marked this pull request as ready for review August 20, 2024 05:53
@ThadHouse ThadHouse requested a review from a team as a code owner August 20, 2024 05:53
wpimath/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@Gold856 Gold856 left a comment

Choose a reason for hiding this comment

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

Can confirm that this fixes my local CMake build on Windows 11 (protoc 25) and fixes #6443.

@PeterJohnson PeterJohnson merged commit a9ac5b8 into wpilibsuite:main Aug 21, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sim GUI can't display protobuf data
5 participants