Skip to content

Commit eb635dd

Browse files
sanchdaP403n1x87
andauthored
chore(profiling): add non-running tests for stack v2 (#8576)
This PR creates a harness for independently building and running the stack v2 code. I did it this way for a few reasons * Don't want to add unnecessary overhead to our normal test process * Want to run the build for these native components under multiple static analysis tools * Want to run some of the tests for these components under sanitizers? * But also want to do exhaustive testing of the interfaces, since we've had a few regressions here This PR includes both the tests and the fixups they demand. A separate PR will add a CI job to execute these (or will add it to a pre-existing job, TBD). There IS some dead code in here, since we do provide the recipe for getting and running Infer. During testing, I found that I couldn't reliably get infer to find stdlib headers, which completely broke the quality of its output, so it won't actually be used. At the same time, I think we might want to use it someday, and the setup is a little fiddly, so I'm including it here. I'm recommending a backport to 2.7 for this because the latest 2.7 releases make this feature available, but the implementation is subtly (and sometimes not-so-subtly) broken without the fixups here. Since this feature is so isolated (I'm the only one backporting anything related!), this feels safe...ish. ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) --------- Co-authored-by: sanchda <[email protected]> Co-authored-by: Gabriele N. Tornetta <[email protected]>
1 parent 6c68609 commit eb635dd

34 files changed

+879
-376
lines changed

ddtrace/internal/datadog/profiling/cmake/AnalysisFunc.cmake

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ function(add_ddup_config target)
1010
"$<$<CONFIG:Release>:-s>"
1111
-Wl,--as-needed -Wl,-Bsymbolic-functions -Wl,--gc-sections
1212
)
13-
set_property(TARGET ${target} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
13+
14+
# If we can IPO, then do so
1415
check_ipo_supported(RESULT result)
1516
if (result)
1617
set_property(TARGET ${target} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
@@ -22,6 +23,18 @@ function(add_ddup_config target)
2223
# pointers, so we include it here.
2324
target_compile_options(${target} PRIVATE -fsanitize=${SANITIZE_OPTIONS} -fno-omit-frame-pointer)
2425
target_link_options(${target} PRIVATE -fsanitize=${SANITIZE_OPTIONS})
26+
27+
# We need to do a little bit of work in order to ensure the dynamic *san libraries can be linked
28+
# Procedure adapted from datadog/ddprof :)
29+
execute_process(
30+
COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=
31+
OUTPUT_VARIABLE LIBSAN_LIB_PATH
32+
OUTPUT_STRIP_TRAILING_WHITESPACE COMMAND_ERROR_IS_FATAL ANY
33+
)
34+
set_target_properties(${target} PROPERTIES
35+
INSTALL_RPATH ${LIBSAN_LIB_PATH}
36+
BUILD_RPATH ${LIBSAN_LIB_PATH}
37+
)
2538
endif()
2639

2740
# If DO_FANALYZER is specified and we're using gcc, then we can use -fanalyzer
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# Enable clang-tidy by default
2+
option(DO_CLANGTIDY "Enable clang-tidy" OFF)
3+
4+
# Default executable for clang-tidy
5+
if (NOT DEFINED CLANGTIDY_CMD)
6+
find_program(CLANGTIDY_CMD NAMES clang-tidy)
7+
endif()
8+
9+
function(add_clangtidy_target TARGET)
10+
if (NOT DO_CLANGTIDY)
11+
return()
12+
endif()
13+
14+
if (CLANGTIDY_CMD)
15+
set_target_properties(${TARGET} PROPERTIES CXX_CLANG_TIDY "${CLANGTIDY_CMD};--checks=bugprone-*,clang-analyzer-*,cppcoreguidelines-*,modernize-*,performance-*,readability-*,-modernize-use-trailing-return-type,-performance-avoid-endl")
16+
else()
17+
message(FATAL_ERROR "clang-tidy requested but executable not found")
18+
endif()
19+
endfunction()

ddtrace/internal/datadog/profiling/cmake/FindCppcheck.cmake

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,21 @@ if (TARGET cppcheck_project)
33
return()
44
endif()
55

6+
# Set the default value for the cppcheck option
7+
option(DO_CPPCHECK "Enable cppcheck" OFF)
8+
69
include(ExternalProject)
710

811
# Build cppcheck from sources
912
if (DO_CPPCHECK)
10-
ExternalProject_Add(cppcheck_project
11-
GIT_REPOSITORY https://github.com/danmar/cppcheck.git
12-
GIT_TAG "2.13.3"
13-
CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}/cppcheck
14-
)
15-
set(CPPCHECK_EXECUTABLE ${CMAKE_BINARY_DIR}/cppcheck/bin/cppcheck)
13+
if (NOT CPPCHECK_EXECUTABLE OR NOT EXISTS "${CPPCHECK_EXECUTABLE}")
14+
ExternalProject_Add(cppcheck_project
15+
GIT_REPOSITORY https://github.com/danmar/cppcheck.git
16+
GIT_TAG "2.13.3"
17+
CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}/cppcheck
18+
)
19+
set(CPPCHECK_EXECUTABLE ${CMAKE_BINARY_DIR}/cppcheck/bin/cppcheck)
20+
endif()
1621

1722
# The function we use to register targets for cppcheck would require us to run separate
1823
# commands for each target, which is annoying. Instead we'll consolidate all the targets
@@ -30,7 +35,7 @@ function(add_cppcheck_target)
3035
cmake_parse_arguments(PARSE_ARGV 0 ARG "${options}" "${oneValueArgs}" "${multiValueArgs}")
3136

3237
# Automatically generate the cppcheck target name
33-
set(NAME "cppcheck_dd_${ARGV0}")
38+
set(NAME "cppcheck_${ARGV0}")
3439

3540
if (DO_CPPCHECK)
3641
# Initialize command variable
@@ -50,6 +55,7 @@ function(add_cppcheck_target)
5055

5156
# Append include directories to the command
5257
foreach(INCLUDE_DIR ${ARG_INCLUDE})
58+
message(STATUS "Adding include directory to cppcheck: ${INCLUDE_DIR}")
5359
list(APPEND cppcheck_cmd -I ${INCLUDE_DIR})
5460
endforeach()
5561

@@ -73,4 +79,13 @@ function(add_cppcheck_target)
7379
COMMENT "cppcheck is disabled for ${ARGV0}"
7480
)
7581
endif()
82+
83+
# Create a standard target to run everything
84+
if (NOT TARGET cppcheck)
85+
add_custom_target(cppcheck COMMENT "Runs cppcheck on all projects")
86+
endif()
87+
88+
if (DO_CPPCHECK)
89+
add_dependencies(cppcheck ${NAME})
90+
endif()
7691
endfunction()
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# Only add this project if Infer is not defined
2+
if(TARGET Infer)
3+
return()
4+
endif()
5+
6+
include(ExternalProject)
7+
# Only grab infer dependencies if we need to
8+
if (DO_INFER)
9+
if (NOT Infer_EXECUTABLE OR NOT EXISTS "${Infer_EXECUTABLE}")
10+
set(TAG_INFER
11+
"v1.1.0"
12+
CACHE STRING "infer github tag")
13+
14+
set(Infer_BUILD_DIR ${CMAKE_BINARY_DIR}/infer)
15+
set(Infer_ROOT ${Infer_BUILD_DIR}/infer-${TAG_LIBDATADOG})
16+
17+
message(STATUS "${CMAKE_CURRENT_LIST_DIR}/tools/fetch_infer.sh ${TAG_INFER} ${Infer_ROOT}")
18+
execute_process(
19+
COMMAND "${CMAKE_CURRENT_LIST_DIR}/tools/fetch_infer.sh" ${TAG_INFER} ${Infer_ROOT}
20+
WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}
21+
COMMAND_ERROR_IS_FATAL ANY
22+
)
23+
24+
set(Infer_DIR "${Infer_ROOT}")
25+
set(Infer_EXECUTABLE "${Infer_DIR}/bin/infer")
26+
endif()
27+
endif()
28+
29+
# Add a target for using infer. It does nothing if DO_INFER is not set
30+
function(add_infer_target TARGET)
31+
# Automatically generate the infer target name
32+
set(NAME "infer_dd_${TARGET}")
33+
34+
if (NOT TARGET ${TARGET})
35+
message(FATAL_ERROR "Target ${TARGET} does not exist")
36+
return()
37+
endif()
38+
39+
if (DO_INFER)
40+
# Initialize command variable
41+
set(infer_cmd
42+
${Infer_EXECUTABLE}
43+
--compilation-database ${CMAKE_CURRENT_BINARY_DIR}/compile_commands.json
44+
)
45+
46+
# Define the custom target with the constructed command
47+
add_custom_target(${NAME}
48+
COMMAND ${infer_cmd}
49+
COMMENT "Running infer on ${TARGET}"
50+
)
51+
52+
# infer can't seem to find stdlib headers, so we have to add them to the target so they are included in the compile_commands.json
53+
foreach(inc ${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES})
54+
target_include_directories(${TARGET} PRIVATE ${inc})
55+
endforeach()
56+
57+
# Make the infer target a dependent of the specified target
58+
add_dependencies(${TARGET} ${NAME})
59+
else()
60+
# Define a do-nothing target if infer is disabled
61+
add_custom_target(${NAME}
62+
COMMAND echo "infer target ${NAME} is disabled."
63+
COMMENT "infer is disabled for ${TARGET}"
64+
)
65+
endif()
66+
endfunction()
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
#!/bin/bash
2+
# http://redsymbol.net/articles/unofficial-bash-strict-mode/
3+
set -euox pipefail
4+
IFS=$'\n\t'
5+
6+
usage() {
7+
echo "Usage :"
8+
echo "$0 <version> <path>"
9+
echo ""
10+
echo "Example"
11+
echo " $0 v0.7.0-rc.1 ./vendor"
12+
}
13+
14+
if [ $# != 2 ] || [ "$1" == "-h" ]; then
15+
usage
16+
exit 1
17+
fi
18+
19+
SCRIPTPATH=$(readlink -f "$0")
20+
SCRIPTDIR=$(dirname "$SCRIPTPATH")
21+
22+
MARCH=$(uname -m)
23+
24+
TAG_INFER=$1
25+
TARGET_EXTRACT=$2
26+
27+
CHECKSUM_FILE=${SCRIPTDIR}/infer_checksums.txt
28+
29+
# https://github.com/facebook/infer/releases/download/v1.1.0/infer-linux64-v1.1.0.tar.xz
30+
TAR_INFER=infer-linux64-${TAG_INFER}.tar.xz
31+
GITHUB_URL_INFER=https://github.com/facebook/infer/releases/download/${TAG_INFER}/${TAR_INFER}
32+
33+
SHA256_INFER="blank"
34+
while IFS=' ' read -r checksum filename; do
35+
if [ "$filename" == "$TAR_INFER" ]; then
36+
SHA256_INFER="$checksum $filename"
37+
break
38+
fi
39+
done < "$CHECKSUM_FILE"
40+
41+
if [ "$SHA256_INFER" == "blank" ]; then
42+
echo "Could not find checksum for ${TAR_INFER} in ${CHECKSUM_FILE}"
43+
exit 1
44+
else
45+
echo "Using infer sha256: ${SHA256_INFER}"
46+
fi
47+
48+
mkdir -p "$TARGET_EXTRACT" || true
49+
cd "$TARGET_EXTRACT"
50+
51+
if [[ -e "${TAR_INFER}" ]]; then
52+
already_present=1
53+
else
54+
already_present=0
55+
echo "Downloading infer ${GITHUB_URL_INFER}..."
56+
if command -v curl > /dev/null 2>&1; then
57+
curl -fsSLO "${GITHUB_URL_INFER}"
58+
elif command -v wget > /dev/null 2>&1; then
59+
wget -q -O "${GITHUB_URL_INFER##*/}" "${GITHUB_URL_INFER}"
60+
else
61+
echo "Error: neither curl nor wget is available." >&2
62+
exit 1
63+
fi
64+
fi
65+
66+
echo "Checking infer sha256"
67+
if ! echo "${SHA256_INFER}" | sha256sum -c -; then
68+
echo "Error validating infer SHA256"
69+
echo "Please clear $TARGET_EXTRACT before restarting"
70+
exit 1
71+
fi
72+
73+
if [[ $already_present -eq 0 ]]; then
74+
echo "Extracting ${TAR_INFER}"
75+
tar xf "${TAR_INFER}" --strip-components=1 --no-same-owner
76+
fi
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
5f5d453814422e93e2a70998d8946b09a2721628ff427f67ff0123dea87461d4 infer-linux64-v1.1.0.tar.xz

ddtrace/internal/datadog/profiling/dd_wrapper/CMakeLists.txt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ include(FetchContent)
1515
include(ExternalProject)
1616
include(FindLibdatadog)
1717
include(AnalysisFunc)
18+
include(FindClangtidy)
1819
include(FindCppcheck)
20+
include(FindInfer)
1921

2022
# Library sources
2123
add_library(dd_wrapper SHARED
@@ -59,6 +61,9 @@ add_cppcheck_target(dd_wrapper
5961
SRC ${CMAKE_CURRENT_SOURCE_DIR}/src
6062
)
6163

64+
add_infer_target(dd_wrapper)
65+
add_clangtidy_target(dd_wrapper)
66+
6267
# Add the tests
6368
if (BUILD_TESTING)
6469
enable_testing()

ddtrace/internal/datadog/profiling/dd_wrapper/include/interface.hpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ extern "C"
2727

2828
bool ddup_is_initialized();
2929
void ddup_init();
30-
void ddup_set_runtime_id(std::string_view id);
30+
void ddup_set_runtime_id(std::string_view runtime_id);
3131
bool ddup_upload();
3232

3333
// Proxy functions to the underlying sample
@@ -36,8 +36,8 @@ extern "C"
3636
void ddup_push_cputime(Datadog::Sample* sample, int64_t cputime, int64_t count);
3737
void ddup_push_acquire(Datadog::Sample* sample, int64_t acquire_time, int64_t count);
3838
void ddup_push_release(Datadog::Sample* sample, int64_t release_time, int64_t count);
39-
void ddup_push_alloc(Datadog::Sample* sample, uint64_t size, uint64_t count);
40-
void ddup_push_heap(Datadog::Sample* sample, uint64_t size);
39+
void ddup_push_alloc(Datadog::Sample* sample, int64_t size, int64_t count);
40+
void ddup_push_heap(Datadog::Sample* sample, int64_t size);
4141
void ddup_push_lock_name(Datadog::Sample* sample, std::string_view lock_name);
4242
void ddup_push_threadinfo(Datadog::Sample* sample,
4343
int64_t thread_id,

ddtrace/internal/datadog/profiling/dd_wrapper/include/libdatadog_helpers.hpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include <iostream>
55
#include <string>
66
#include <string_view>
7+
#include <variant>
78

89
extern "C"
910
{
@@ -126,6 +127,16 @@ add_tag(ddog_Vec_Tag& tags, const ExportTagKey key, std::string_view val, std::s
126127
return add_tag(tags, key_sv, val, errmsg);
127128
}
128129

130+
inline std::variant<ddog_prof_Exporter*, ddog_Error>
131+
get_newexporter_result(const ddog_prof_Exporter_NewResult& res)
132+
{
133+
if (res.tag == DDOG_PROF_EXPORTER_NEW_RESULT_OK) {
134+
return res.ok; // NOLINT (cppcoreguidelines-pro-type-union-access)
135+
} else {
136+
return res.err; // NOLINT (cppcoreguidelines-pro-type-union-access)
137+
}
138+
}
139+
129140
// Keep macros from propagating
130141
#undef X_STR
131142
#undef X_ENUM

ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ class Profile
7070
void profile_release();
7171

7272
// String table manipulation
73-
std::string_view insert_or_get(std::string_view sv);
73+
std::string_view insert_or_get(std::string_view str);
7474

7575
// constref getters
7676
const ValueIndex& val();

0 commit comments

Comments
 (0)