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

Limit protobuff symbol visibility #179

Merged
merged 1 commit into from
Oct 1, 2018
Merged

Limit protobuff symbol visibility #179

merged 1 commit into from
Oct 1, 2018

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented Sep 21, 2018

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

@JanuszL JanuszL changed the title Limit protobuff symbol visibility [WIP] Limit protobuff symbol visibility Sep 24, 2018
@JanuszL
Copy link
Contributor Author

JanuszL commented Sep 25, 2018

Builds 36082 and it seems to work at least now. I'm still thinking/working in the mean time on the long term solution.

@JanuszL JanuszL changed the title [WIP] Limit protobuff symbol visibility Limit protobuff symbol visibility Sep 25, 2018
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL
Copy link
Contributor Author

JanuszL commented Sep 27, 2018

I created wrapper for proto_dali class that makes it private. It finally allows us to hide all libprotobuf symbols.

@JanuszL
Copy link
Contributor Author

JanuszL commented Sep 27, 2018

Builds 36211.

Copy link
Contributor

@cliffwoolley cliffwoolley left a comment

Choose a reason for hiding this comment

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

TBH, I only mostly understand how this works by inspection. But if it passes the tests we have in place now without segfaulting then AFAIK it should be ok and better than what we had before.

@JanuszL
Copy link
Contributor Author

JanuszL commented Oct 1, 2018

@cliffwoolley - giving more background. The problem we have so far is that we leaks protobuf symbols. It happens because we expose dali_proto::* in our API and require everyone using to libdali.so to include dali.pb.h. dali.pb.h includes protobuf headers, so all libs depending on libdali.so need to link with libprotobuf. Currently we expose libprotobuf symbols in libdali.so (what is bad). This patch replaces dali_proto::* with DaliProtoPriv which makes dali.pb.h internal. Since now, all that everyone needs to do is to include dali_proto_intern.h which just keeps a pointer to dali_proto::Argument (but for that we don't need to expose dali.pb.h, it is just pointer to some class).
And yes, it doesn't break our tests.

@JanuszL JanuszL merged commit ca0554e into NVIDIA:master Oct 1, 2018
@JanuszL
Copy link
Contributor Author

JanuszL commented Oct 1, 2018

I will deal with OpenCV and some C++ symbols in other PR.

@JanuszL JanuszL deleted the symobl_vis branch October 1, 2018 10:15
pribalta pushed a commit to pribalta/DALI that referenced this pull request Oct 1, 2018
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
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.

2 participants