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

[POC RFC] header only API for singletons #1525

Closed
wants to merge 18 commits into from

Conversation

marcalff
Copy link
Member

Fix header only API for singletons (#1520)

Changes

DRAFT -- implement header only API singletons.

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Fix header only API for singletons (open-telemetry#1520)
@marcalff marcalff requested a review from a team July 28, 2022 15:31
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #1525 (2eed305) into main (b520480) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1525      +/-   ##
==========================================
- Coverage   85.02%   84.94%   -0.07%     
==========================================
  Files         156      156              
  Lines        4977     4959      -18     
==========================================
- Hits         4231     4212      -19     
- Misses        746      747       +1     
Impacted Files Coverage Δ
api/include/opentelemetry/baggage/baggage.h 97.28% <100.00%> (-0.04%) ⬇️
...ntelemetry/context/propagation/global_propagator.h 100.00% <100.00%> (ø)
...pi/include/opentelemetry/context/runtime_context.h 97.54% <100.00%> (-0.05%) ⬇️
api/include/opentelemetry/metrics/provider.h 100.00% <100.00%> (ø)
api/include/opentelemetry/trace/provider.h 100.00% <100.00%> (ø)
api/include/opentelemetry/trace/trace_state.h 97.54% <100.00%> (-0.05%) ⬇️
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) ⬇️
api/include/opentelemetry/trace/noop.h 93.11% <0.00%> (+6.90%) ⬆️

@lalitb
Copy link
Member

lalitb commented Jul 29, 2022

Did a quick test with this PR on Linux, by creating an instrumented library with -fvisible=hidden flag and the library is able to use the singleton tracer provider from the application. So changes seem to work on Linux.

Need to check on Windows, but based on what @owent has already tested here, it may not work with PE ABI as __declspec(dllexport) needs to be held inside the compile unit.

@owent
Copy link
Member

owent commented Jul 29, 2022

I have add more examples here, and __declspec (selectany) of MSVC works well, but __attribute__ ((selectany)) of GCC on Windows do not work as expected,do I miss anything?

@marcalff
Copy link
Member Author

marcalff commented Jul 29, 2022

Resources:

Paper from Ulrich Drepper, see section 2.2 export control, page 17:

https://akkadia.org/drepper/dsohowto.pdf

Gnu visibility doc

https://gcc.gnu.org/wiki/Visibility

@owent
Copy link
Member

owent commented Jul 30, 2022

Resources:

Paper from Ulrich Drepper, see section 2.2 export control, page 17:

https://akkadia.org/drepper/dsohowto.pdf

Gnu visibility doc

https://gcc.gnu.org/wiki/Visibility

I have a quick look at https://akkadia.org/drepper/dsohowto.pdf, I think it's for ELF ABI of Linux, not PE ABI of Windows.
And the visibility attribute of gcc document(https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes) also says The possible values of visibility_type correspond to the visibility settings in the ELF gABI. .

My question is: is there any way to make the singleton unique with GCC on Windows?

@lalitb
Copy link
Member

lalitb commented Aug 24, 2022

@owent - We agreed this PR to be scoped for Linux and Mac. Let us know if you have any comment on that.

@owent
Copy link
Member

owent commented Aug 25, 2022

@owent - We agreed this PR to be scoped for Linux and Mac. Let us know if you have any comment on that.

Fine by me. I think we can document this if we do not support build shared library with gcc and clang on Windows, can we also force set BUILD_SHARED_LIB to OFF when we use gcc and clang on Windows to reduce misuse?

@lalitb @marcalff

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

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

LGTM, Do we need set BUILD_SHARED_LIB to OFF with gcc on Windows to avoid misuse?

@marcalff
Copy link
Member Author

marcalff commented Sep 5, 2022

@lalitb , @esigo , @owent

I added a singleton_test unit test, and makefiles for both CMake and Bazel.

This test does not build properly on windows platforms, and I can not figure out why.

"component_c" is compiled as a shared library / DDL (expected),
but then when linking the final singleton_test binary, the makefile wants a component_c.lib instead of component_c.ddl,
which is incorrect.

On linux, the same makefile with cmake properly builds static libraries (component_a, component_b) and shared libraries (component_c, d, e and f) as expected.

For bazel, I could not find a proper way to express in the BUILD file how to link a test binary singleton_test with a mix of static (component_a and b) and shared (component_c, d, e and f) libraries.

Any help appreciated, the goal is to have first a unit test that builds everywhere.

Regards,
-- Marc

@@ -29,8 +44,8 @@ class Provider
*/
static nostd::shared_ptr<MeterProvider> GetMeterProvider() noexcept
{
std::lock_guard<common::SpinLockMutex> guard(lock);
nostd::shared_ptr<MeterProvider> result(provider);
std::lock_guard<common::SpinLockMutex> guard(s.lock);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not use a short name here

@owent
Copy link
Member

owent commented Sep 5, 2022

@lalitb , @esigo , @owent

I added a singleton_test unit test, and makefiles for both CMake and Bazel.

This test does not build properly on windows platforms, and I can not figure out why.

"component_c" is compiled as a shared library / DDL (expected), but then when linking the final singleton_test binary, the makefile wants a component_c.lib instead of component_c.ddl, which is incorrect.

On linux, the same makefile with cmake properly builds static libraries (component_a, component_b) and shared libraries (component_c, d, e and f) as expected.

For bazel, I could not find a proper way to express in the BUILD file how to link a test binary singleton_test with a mix of static (component_a and b) and shared (component_c, d, e and f) libraries.

Any help appreciated, the goal is to have first a unit test that builds everywhere.

Regards, -- Marc

Do we need declare do_something_in_c as default visibility or __declspec(dllexport)/__declspec(dllimport) ? I find component_c is built as a shared library but export nothing.

To my knowledge, MSVC will create both .lib and .dll for dynamic libraries. .lib is for linking and .dll is for runtime.

@marcalff
Copy link
Member Author

marcalff commented Sep 5, 2022

Do we need declare do_something_in_c as default visibility or __declspec(dllexport)/__declspec(dllimport) ? I find component_c is built as a shared library but export nothing.

Good point, fixing this.

Thanks

@marcalff
Copy link
Member Author

This PR contains proof of concept code, for a general fix.

It is now closed, for the benefit of:

Work for windows should continue in PR #1595.

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.

3 participants