-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
explicitly export public symbols #1321
Conversation
Sanity check: this should not force anything to be exported UNLESS we are actually building shared libbenchmark/libbenchmark_main. |
I'm not sure what you mean. |
Benchmark(Benchmark const&) | ||
#if defined(BENCHMARK_HAS_CXX11) | ||
= delete | ||
#endif | ||
; | ||
|
||
Benchmark& operator=(Benchmark const&) | ||
#if defined(BENCHMARK_HAS_CXX11) | ||
= delete | ||
#endif | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure. I stumbled upon this while fixing the DLL export under MSVC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, these are the linker errors triggered without these changes:
Creating library src\benchmark.lib and object src\benchmark.exp
string_util.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
sysinfo.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
timers.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
perf_counters.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
reporter.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
sleep.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
statistics.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
console_reporter.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
counter.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
csv_reporter.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
json_reporter.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
benchmark_runner.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
colorprint.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
commandlineflags.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
complexity.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
benchmark.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
benchmark_api_internal.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
benchmark_name.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
benchmark_register.cc.obj : error LNK2001: unresolved external symbol "protected: __cdecl benchmark::internal::Benchmark::Benchmark(class benchmark::internal::Benchmark const &)" (??0Benchmark@internal@benchmark@@IEAA@AEBV012@@Z)
src\benchmark.dll : fatal error LNK1120: 1 unresolved externals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to hear an explanation as to why this is okay and not does result in ABI mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I've reverted the changes in a separate commit here so you can investigate the issue yourself. The MSVC 2022 runner fails now at linking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like MSVC does not allow to set the language standard prior to C++11. Therefore, an ABI mismatch cannot generally occur.
That's what i wanted to hear, yes. |
it depends what the cmake version is doing. do the same thing. :) maybe something like https://github.com/RobotLocomotion/drake/blob/master/tools/workspace/lcm/package.BUILD.bazel#L5 ?
that would be a good idea i think. |
b10b6c6
to
8aab6c5
Compare
b4de3c2
to
e46d347
Compare
6a34e33
to
c88145d
Compare
The following parts need to be clarified as well: Line 25 in 6d51a11
Line 62 in 6d51a11
Currently, the |
If there are no opinions, I guess it is fine to increment |
i think i'm ok with this. but i'm also aware there are potential ramifications of the change that i'm not aware of. thank you so much for your patience, @sergiud. @LebedevRI can you take a look at some point and see if you have any remaining concerns? |
49d5503
to
50de9a8
Compare
Looks like @LebedevRI does not want to review. |
ok, i'll take that as an implicit upvote then :) |
Thanks! |
Compiling benchmark as a shared library with
-flto
and-fvisibility=hidden
causes a huge number of linker errors:The problem is caused by missing visibility attributes for symbols used outside of the library (as such part of the public interface even though some of these symbols are declared in the
internal
namespace).This PR adds proper annotations to public symbols using the new
export.h
header. This fixes #1070 and enables fixing #1022.Selective symbol export has the benefit of reducing the size of the generated
libbenchmark.so.*
. For instance, on Ubuntu 20.04 I observe a size reduction from 436K to 420K.Open questions:
export.h
be defined for Bazel builds?CMAKE_CXX_VISIBILITY_PRESET=hidden
and possiblyCMAKE_VISIBILITY_INLINES_HIDDEN=ON
, and-flto
enabled to avoid regressions