opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

[POC RFC] header only API for singletons

Open marcalff opened this issue 2 years ago • 5 comments

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

marcalff avatar Jul 28 '22 15:07 marcalff

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%) :arrow_down:
...ntelemetry/context/propagation/global_propagator.h 100.00% <100.00%> (ø)
...pi/include/opentelemetry/context/runtime_context.h 97.54% <100.00%> (-0.05%) :arrow_down:
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%) :arrow_down:
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) :arrow_down:
api/include/opentelemetry/trace/noop.h 93.11% <0.00%> (+6.90%) :arrow_up:

codecov[bot] avatar Jul 28 '22 16:07 codecov[bot]

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.

lalitb avatar Jul 29 '22 04:07 lalitb

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?

owent avatar Jul 29 '22 07:07 owent

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

marcalff avatar Jul 29 '22 18:07 marcalff

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?

owent avatar Jul 30 '22 07:07 owent

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

lalitb avatar Aug 24 '22 17:08 lalitb

@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

owent avatar Aug 25 '22 06:08 owent

@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

marcalff avatar Sep 05 '22 08:09 marcalff

@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.

owent avatar Sep 05 '22 08:09 owent

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 avatar Sep 05 '22 08:09 marcalff

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

It is now closed, for the benefit of:

  • PR #1604, a fix for gcc and clang, ready for merge
  • PR #1595, a tentative fix for windows, still a proof of concept.

Work for windows should continue in PR #1595.

marcalff avatar Sep 12 '22 11:09 marcalff