GH-46714: [C++] Use hidden symbol visibility in Meson configuration #46715
Conversation
|
|
|
FWIW the hidden symbol visibility appears to shave a couple MB off of the libarrow release size (43MB -> 41 MB locally) |
cpp/src/arrow/compute/expression.cc
Outdated
| @@ -260,7 +260,9 @@ bool Expression::Equals(const Expression& other) const { | |||
| return false; | |||
| } | |||
|
|
|||
| bool Identical(const Expression& l, const Expression& r) { return l.impl_ == r.impl_; } | |||
| ARROW_EXPORT bool Identical(const Expression& l, const Expression& r) { | |||
There was a problem hiding this comment.
Why do we need to use ARROW_EXPORT in .cc not .h?
There was a problem hiding this comment.
I had some dejavu on this and we actually discussed this over a year ago back in #41872 (comment)
It looks like with clang and gcc 12+ this is required to be attached to the definition.
Alternatively, maybe we can make this not a friend qualified method in the header?
There was a problem hiding this comment.
Oh!
@pitrou Do you have any opinion for this?
There was a problem hiding this comment.
This could easily be a static method on Expression instead of a friend function.
There was a problem hiding this comment.
I've tried to move as many of these from the module to the header; however, I do not think the default visibility for the friend methods can be moved to the header, but rather must be attached to the definition:
https://stackoverflow.com/a/52332379/621736
I suppose we could alternatively move the friend definitions into the header files?
As an aside, it looks like the code base currently has an incomplete (?) macro intended for use with friends, ARROW_FRIEND_EXPORT:
arrow/cpp/src/arrow/util/visibility.h
Line 48 in 5a52916
This is perhaps facing the limitation with friend exports needing to be attached to the definition linked above. Maybe this should be removed?
github-actions
bot
added
awaiting changes
Why is that? |
| @@ -81,6 +81,7 @@ arrow_acero_lib = library( | |||
| 'arrow-acero', | |||
| sources: arrow_acero_srcs, | |||
| dependencies: [arrow_dep], | |||
| gnu_symbol_visibility: 'hidden', | |||
There was a problem hiding this comment.
Does it also work with clang or is it gcc-specific?
There was a problem hiding this comment.
This works with clang as well
The idea is just that Windows has a harder requirement around symbols not being public by default. Of course, there are extra macros that would need to be defined to fully support the Windows linker requirements, but this is at least a sanity check that we are exporting the right symbols. I am also under the assumption that most developers are on *nix platforms, so it helps them detect potential symbol export issues sooner, rather than waiting for them to pop up in Windows CI |
|
It would be nice to try applying those changes on the CMake side as well. I agree that it would be good hygiene to have to mark exported symbols explicitly. |
github-actions
bot
added
awaiting change review
Any objection to doing this as a follow up? I did try that previously and ran into issues with some of the downstream crossbow jobs specifically on macOS, so I'm a bit hesitant to be opening a pandora's box with the CMake config in this PR |
|
No, it's fine. |
Rationale for this change
This will eventually make it easier to support Windows through the Meson configuration, while also providing the documented benefits of hidden symbol visibility
What changes are included in this PR?
Meson libraries have been set to use hidden symbol visibility, and corresponding definitions have been updated to use the ARROW_EXPORT macro
Are these changes tested?
Yes
Are there any user-facing changes?
No