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.

Sign up for GitHub

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

mac shlib dependencies fixes #1891

Closed
wants to merge 3 commits into from
Closed

mac shlib dependencies fixes #1891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97d3942
mac shlib dependencies fixes [all symbols referenced by shlib must re…
ays7 Dec 2, 2022
eb5d695
CR tip: have only otlp_grpc_client lib link against gRPC & make that …
ays7 Dec 31, 2022
9665120
Link gRPC::grpc++ into dependent libs privately
ays7 Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions exporters/ostream/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ target_include_directories(
opentelemetry_exporter_ostream_metrics
PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>")
target_link_libraries(opentelemetry_exporter_ostream_metrics
PUBLIC opentelemetry_metrics)
PUBLIC opentelemetry_metrics opentelemetry_resources)
install(
TARGETS opentelemetry_exporter_ostream_metrics
EXPORT "${PROJECT_NAME}-target"
Expand All @@ -54,9 +54,8 @@ install(
PATTERN "metric_exporter.h")
if(BUILD_TESTING)
add_executable(ostream_metric_test test/ostream_metric_test.cc)
target_link_libraries(
ostream_metric_test ${GTEST_BOTH_LIBRARIES}
opentelemetry_exporter_ostream_metrics opentelemetry_resources)
target_link_libraries(ostream_metric_test ${GTEST_BOTH_LIBRARIES}
opentelemetry_exporter_ostream_metrics)
gtest_add_tests(
TARGET ostream_metric_test
TEST_PREFIX exporter.
Expand Down
22 changes: 12 additions & 10 deletions exporters/otlp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ target_include_directories(
set(OPENTELEMETRY_OTLP_TARGETS opentelemetry_otlp_recordable)
target_link_libraries(
opentelemetry_otlp_recordable
PUBLIC opentelemetry_trace opentelemetry_resources opentelemetry_proto)
PUBLIC opentelemetry_common opentelemetry_trace opentelemetry_resources
opentelemetry_proto)

if(WITH_LOGS_PREVIEW)
target_link_libraries(opentelemetry_otlp_recordable PUBLIC opentelemetry_logs)
Expand All @@ -33,7 +34,8 @@ if(WITH_OTLP_GRPC)
PROPERTIES EXPORT_NAME otlp_grpc_client)
target_link_libraries(
opentelemetry_exporter_otlp_grpc_client
PUBLIC opentelemetry_sdk opentelemetry_ext opentelemetry_proto)
PUBLIC opentelemetry_common opentelemetry_sdk opentelemetry_ext
opentelemetry_proto)

target_link_libraries(opentelemetry_exporter_otlp_grpc_client
PRIVATE gRPC::grpc++)
Expand All @@ -60,8 +62,8 @@ if(WITH_OTLP_GRPC)

target_link_libraries(
opentelemetry_exporter_otlp_grpc
PUBLIC opentelemetry_otlp_recordable
opentelemetry_exporter_otlp_grpc_client)
PUBLIC opentelemetry_otlp_recordable opentelemetry_exporter_otlp_grpc_client
PRIVATE gRPC::grpc++)
owent marked this conversation as resolved.
Show resolved Hide resolved

list(APPEND OPENTELEMETRY_OTLP_TARGETS opentelemetry_exporter_otlp_grpc)

Expand All @@ -75,8 +77,8 @@ if(WITH_OTLP_GRPC)

target_link_libraries(
opentelemetry_exporter_otlp_grpc_log
PUBLIC opentelemetry_otlp_recordable
opentelemetry_exporter_otlp_grpc_client)
PUBLIC opentelemetry_otlp_recordable opentelemetry_exporter_otlp_grpc_client
PRIVATE gRPC::grpc++)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we link gRPC::grpc++ into opentelemetry_exporter_otlp_grpc, opentelemetry_exporter_otlp_grpc_log and opentelemetry_exporter_otlp_grpc_metrics? If we build otel-cpp as static libraries, they will link it just by link opentelemetry_exporter_otlp_grpc_client, and if we build otel-cpp as dynamic libraries, I think the global variables of gRPC are both in opentelemetry_exporter_otlp_grpc_client, opentelemetry_exporter_otlp_grpc, opentelemetry_exporter_otlp_grpc_log and opentelemetry_exporter_otlp_grpc_metrics, the symbol overlap problem still exists.

Copy link
Member

Choose a reason for hiding this comment

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

@ays7 - Can you reply to @owent concerns here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@owent gRPC::grpc++ must be linked either directly or via shared dependency into all libs that make direct gRPC API calls to satisfy Mac shared library linker -- Mac shared libraries are linked like executables on linux -- all referenced symbols must be resolved to somewhere.
One way to achieve this is by exposing gPRC as a public dependency of opentelemetry_exporter_otlp_grpc_client, which is a common dependency of the 3 libs you mentioned -- that was my original suggestion.

For the static linking: here is my understanding: when linking shared libs on linux, linker first attempts to resolve referenced symbols to one of linked in shared libraries & if that fails to one of linked in static libraries -- in a later case code from the static library is copied into the output.

With that in mind, in the weird scenario of static gRPC/dynamic OTEL libraries mix, what happens is - bulk of gPRC library code is embedded into opentelemetry_exporter_otlp_grpc_client and all other otlp libs [which are dependent on _client] reference gRPC code from the _client .so and not explicitly linked static gRPC library.

There is a potential issue if, say, both opentelemetry_exporter_otlp_grpc_log & opentelemetry_exporter_otlp_grpc_metrics call certain function that is not called inside opentelemetry_exporter_otlp_grpc_client -- in that case both _log & _metrics may get a copy of some gRPC lib code. Fortunately there are no gRPC symbols like that at this point, but maybe it is indeed safer to declare gRPC lib as common PUBLIC dependency as I originally suggested?

Copy link
Member

Choose a reason for hiding this comment

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

We can not link static gRPC into more than one dynamic libraries and executable. There will be more than one copy of some global variables in gRPC and some of them will be initialized more than once and some will not be initialized for ever.So the solution is just link gRPC into one dynamic library, that is opentelemetry_exporter_otlp_grpc_client, other libraries should only use the API from opentelemetry_exporter_otlp_grpc_client and can not use gRPC APIs directly.
Here is a simple sample to reproduce the problem in gRPC.

a.h

struct foo {
  int bar;
  foo();
  ~foo();

  void print(const char*);

  static foo _;
};

void print_static_global(const char*);

a.cpp

#include "a.h"

#include <iostream>
#include <thread>
#include <chrono>
#include <memory>

struct bar {
  foo* ptr = nullptr;
};

foo foo::_;
static bar s_;

foo::foo(): bar(127) {
    s_.ptr = this;
    std::cout<< "construct "<< this<< std::endl;
}

foo::~foo() {
    std::cout<< "destroy "<< this<< std::endl;
}

void foo::print(const char* prefix) {
    std::cout<< prefix<< "-foo: "<< this<< ": "<< bar<< std::endl;
}

void print_static_global(const char* prefix) {
    foo::_.print(prefix);
    std::cout<< prefix<< "-piblic API bar: "<< s_.ptr<< std::endl;
}

std::shared_ptr<std::thread> g_t(new std::thread([]() {
  std::this_thread::sleep_for(std::chrono::seconds{1});
  std::cout<< "internal API bar: "<< s_.ptr<< std::endl;
}), [](std::thread* thd) {
  thd->join();
  delete thd;
});

b.cpp

#include "a.h"

void dll_func_b() {
    print_static_global("b");
}

c.cpp

#include "a.h"

void dll_func_b();

int main() {
  print_static_global("c");
  dll_func_b();
  return 0;
}

Compile and run

[owent@VM-144-59-centos test]$ clang++ a.cpp -o libtest_a.a -c -fPIC -pthread 
[owent@VM-144-59-centos test]$ clang++ b.cpp -o libtest_b.so -shared -fPIC -L$PWD -ltest_a -pthread
[owent@VM-144-59-centos test]$ clang++ c.cpp -fPIC -L$PWD -ltest_b -ltest_a '-Wl,-rpath=$ORIGIN' -pthread
[owent@VM-144-59-centos test]$ ./a.out 
[owent@VM-144-59-centos test]$ ./a.out 
construct 0x55af97755338
construct 0x55af97755338
c-foo: 0x55af97755338: 127
c-piblic API bar: 0x55af97755338
b-foo: 0x55af97755338: 127
b-piblic API bar: 0x55af97755338
internal API bar: (nil)
internal API bar: 0x55af97755338
destroy 0x55af97755338
destroy 0x55af97755338
[owent@VM-144-59-centos test]$

$ORIGIN may be replaced by @loader_path on macOS.

As you see, the global variable 0x55af97755338 is constructed twice and destroyed twice,And the internal API use different static bar s_;. I didn't test it on macOS but i guess it has the same problem.It's more complicated in gRPC and it will make gRPC do not work in some dynamic libraries.

Actually, if we set gRPC as private dependency of opentelemetry_exporter_otlp_grpc_client, and when we build opentelemetry_exporter_otlp_grpc_client into a static library. All the targets will links gRPC automatically by just link opentelemetry_exporter_otlp_grpc_client.

Copy link
Contributor Author

@ays7 ays7 Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@owent Thanks for the test - I will play with it soon.
In a mean time here is another idea: anything particularly wrong with combining all _otlp_grpc_ libraries into a single opentelemetry_exporter_otlp_grpc library?

And one more thing: if static gRPC library can only be linked into one DLL then any application that use gRPC [either directly or indirectly via, say, third party dependency] will be in conflict with otlp_grpc libraries. Which logically means that use of static gRPC libraries is a bad idea. Right?

Copy link
Member

Choose a reason for hiding this comment

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

@owent Thanks for the test - I will play with it soon. In a mean time here is another idea: anything particularly wrong with combining all _otlp_grpc_ libraries into a single opentelemetry_exporter_otlp_grpc library?

I prefer spilt modules one by one personally, the build system may built all libraries but most users will use only part of them.

It will cost more memory and CPU if we pack all codes into one library, we have a project which will cost about 8+GB memory to link every executable in this project and it's easy OOM when we link them concurrency.We have done many jobs to split dependencies into small libraries to solve this problem.

And one more thing: if static gRPC library can only be linked into one DLL then any application that use gRPC [either directly or indirectly via, say, third party dependency] will be in conflict with otlp_grpc libraries. Which logically means that use of static gRPC libraries is a bad idea. Right?

Yes, in my understanding, it's safer to use static or dynamic libraries for all dependencies, not mixed. There were issues report this problem when only use otlp-cpp, so I think the only thing we can do is keep it right when users only use and link otlp-cpp’s SDK.

Copy link
Member

Choose a reason for hiding this comment

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

@ays7 - Do you have any questions on above comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lalitb Sorry, I don't think I see a way out: static linking gRPC is a bad idea one way or another. IMHO.
Looks like @owent is exploring some alternatives with PR #2005 -- perhaps this PR can be abandoned [as long as #2005 fixes mac build too]


list(APPEND OPENTELEMETRY_OTLP_TARGETS opentelemetry_exporter_otlp_grpc_log)

Expand All @@ -89,8 +91,8 @@ if(WITH_OTLP_GRPC)

target_link_libraries(
opentelemetry_exporter_otlp_grpc_metrics
PUBLIC opentelemetry_otlp_recordable
opentelemetry_exporter_otlp_grpc_client)
PUBLIC opentelemetry_otlp_recordable opentelemetry_exporter_otlp_grpc_client
PRIVATE gRPC::grpc++)
owent marked this conversation as resolved.
Show resolved Hide resolved

list(APPEND OPENTELEMETRY_OTLP_TARGETS
opentelemetry_exporter_otlp_grpc_metrics)
Expand All @@ -102,8 +104,8 @@ if(WITH_OTLP_HTTP)
PROPERTIES EXPORT_NAME otlp_http_client)
target_link_libraries(
opentelemetry_exporter_otlp_http_client
PUBLIC opentelemetry_sdk opentelemetry_proto opentelemetry_http_client_curl
nlohmann_json::nlohmann_json)
PUBLIC opentelemetry_common opentelemetry_sdk opentelemetry_proto
opentelemetry_http_client_curl nlohmann_json::nlohmann_json)
if(nlohmann_json_clone)
add_dependencies(opentelemetry_exporter_otlp_http_client
nlohmann_json::nlohmann_json)
Expand Down