Skip to content

Commit

Permalink
fix(profiling): explicitly link with thread library (#10994)
Browse files Browse the repository at this point in the history
python:3.11-bookworm image for aarch64 has `pthread_atfork` in static
libpthread.a not in libpthread.so so need to explicitly link for those
cases. Otherwise, we get undefined symbol `pthread_atfork`.


## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [ ] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
  • Loading branch information
taegyunkim authored Oct 11, 2024
1 parent 5b12969 commit 79cd78d
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 7 deletions.
9 changes: 8 additions & 1 deletion ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ include(FindCppcheck)
include(FindInfer)
include(CheckSymbolExists)

set(THREADS_PREFER_PTHREAD_FLAG ON)
find_package(Threads REQUIRED)

if(NOT Threads_FOUND OR NOT CMAKE_USE_PTHREADS_INIT)
message(FATAL_ERROR "pthread compatible library not found")
endif()

# Library sources
add_library(
dd_wrapper SHARED
Expand All @@ -44,7 +51,7 @@ target_compile_features(dd_wrapper PUBLIC cxx_std_17)

target_include_directories(dd_wrapper PRIVATE include ${Datadog_INCLUDE_DIRS})

target_link_libraries(dd_wrapper PRIVATE ${Datadog_LIBRARIES})
target_link_libraries(dd_wrapper PRIVATE ${Datadog_LIBRARIES} Threads::Threads)

# Figure out the suffix. Try to approximate the cpython way of doing things. C library
check_symbol_exists(__GLIBC__ "features.h" HAVE_GLIBC)
Expand Down
19 changes: 13 additions & 6 deletions ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,19 @@ include(FindCppcheck)
add_subdirectory(../dd_wrapper ${CMAKE_CURRENT_BINARY_DIR}/../dd_wrapper_build)

find_package(Python3 COMPONENTS Interpreter Development)

# Make sure we have necessary Python variables
if(NOT Python3_INCLUDE_DIRS)
message(FATAL_ERROR "Python3_INCLUDE_DIRS not found")
endif()

set(THREADS_PREFER_PTHREAD_FLAG ON)
find_package(Threads REQUIRED)

if(NOT Threads_FOUND OR NOT CMAKE_USE_PTHREADS_INIT)
message(FATAL_ERROR "pthread compatible library not found")
endif()

# Add echion
set(ECHION_COMMIT
"9d5bcc5867d7aefff73c837adcba4ef46eecebc6"
Expand Down Expand Up @@ -70,9 +78,9 @@ target_compile_definitions(${EXTENSION_NAME} PRIVATE UNWIND_NATIVE_DISABLE)
# warning(push, 0 then pop for the same effect.
target_include_directories(
${EXTENSION_NAME} PRIVATE .. # include dd_wrapper from the root in order to make its paths transparent in the code
include)
include)
target_include_directories(${EXTENSION_NAME} SYSTEM PRIVATE ${echion_SOURCE_DIR} ${Python3_INCLUDE_DIRS}
include/vendored include/util)
include/vendored include/util)

# Echion sources need to be given the current platform
if(WIN32)
Expand All @@ -92,11 +100,10 @@ set_target_properties(${EXTENSION_NAME} PROPERTIES SUFFIX "")
# typical.
set_target_properties(${EXTENSION_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN/..")

target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper Threads::Threads)

if(Python3_LIBRARIES)
target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper ${Python3_LIBRARIES})
else()
# for manylinux builds
target_link_libraries(${EXTENSION_NAME} PRIVATE dd_wrapper)
target_link_libraries(${EXTENSION_NAME} PRIVATE ${Python3_LIBRARIES})
endif()

# Extensions are built as dynamic libraries, so PIC is required.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fixes:
- |
profiling: fixes an issue where stack v2 couldn't be enabled as pthread
was not properly linked on some debian based images for aarch64 architecture.

0 comments on commit 79cd78d

Please sign in to comment.