[Lldb-commits] [lldb] 159b872 - [llvm][telemetry]Change Telemetry-disabling mechanism. (#128534)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Feb 26 10:01:56 PST 2025
Author: Vy Nguyen
Date: 2025-02-26T13:01:53-05:00
New Revision: 159b872b37363511a359c800bcc9230bb09f2457
URL: https://github.com/llvm/llvm-project/commit/159b872b37363511a359c800bcc9230bb09f2457
DIFF: https://github.com/llvm/llvm-project/commit/159b872b37363511a359c800bcc9230bb09f2457.diff
LOG: [llvm][telemetry]Change Telemetry-disabling mechanism. (#128534)
Details:
- Previously, we used the LLVM_BUILD_TELEMETRY flag to control whether
any Telemetry code will be built. This has proven to cause more nuisance
to both users of the Telemetry and any further extension of it. (Eg., we
needed to put #ifdef around caller/user code)
- So the new approach is to:
+ Remove this flag and introduce LLVM_ENABLE_TELEMETRY which would be
true by default.
+ If LLVM_ENABLE_TELEMETRY is set to FALSE (at buildtime), the library
would still be built BUT Telemetry cannot be enabled. And no data can be
collected.
The benefit of this is that it simplifies user (and extension) code
since we just need to put the check on Config::EnableTelemetry. Besides,
the Telemetry library itself is very small, hence the additional code to
be built would not cause any difference in build performance.
---------
Co-authored-by: Pavel Labath <pavel at labath.sk>
Added:
Modified:
lldb/source/Core/CMakeLists.txt
lldb/source/Core/Telemetry.cpp
lldb/unittests/Core/CMakeLists.txt
lldb/unittests/Core/TelemetryTest.cpp
llvm/CMakeLists.txt
llvm/cmake/modules/LLVMConfig.cmake.in
llvm/include/llvm/Config/llvm-config.h.cmake
llvm/include/llvm/Telemetry/Telemetry.h
llvm/lib/CMakeLists.txt
llvm/lib/Telemetry/Telemetry.cpp
llvm/unittests/CMakeLists.txt
llvm/unittests/Telemetry/TelemetryTest.cpp
llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
utils/bazel/llvm_configs/llvm-config.h.cmake
Removed:
################################################################################
diff --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index 82fb5f42f9f4b..d5d8a9d5088fc 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -16,10 +16,6 @@ if (LLDB_ENABLE_CURSES)
endif()
endif()
-if (LLVM_BUILD_TELEMETRY)
- set(TELEMETRY_DEPS Telemetry)
-endif()
-
# TODO: Add property `NO_PLUGIN_DEPENDENCIES` to lldbCore
add_lldb_library(lldbCore
Address.cpp
@@ -84,7 +80,7 @@ add_lldb_library(lldbCore
Support
Demangle
TargetParser
- ${TELEMETRY_DEPS}
+ Telemetry
)
add_dependencies(lldbCore
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index a0c9fb83e3a6b..fef63e3696e70 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -5,11 +5,6 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
-
-#include "llvm/Config/llvm-config.h"
-
-#ifdef LLVM_BUILD_TELEMETRY
-
#include "lldb/Core/Telemetry.h"
#include "lldb/Core/Debugger.h"
#include "lldb/Utility/LLDBLog.h"
@@ -67,7 +62,11 @@ llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
}
std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr;
-TelemetryManager *TelemetryManager::GetInstance() { return g_instance.get(); }
+TelemetryManager *TelemetryManager::GetInstance() {
+ if (!Config::BuildTimeEnableTelemetry)
+ return nullptr;
+ return g_instance.get();
+}
void TelemetryManager::SetInstance(std::unique_ptr<TelemetryManager> manager) {
g_instance = std::move(manager);
@@ -75,5 +74,3 @@ void TelemetryManager::SetInstance(std::unique_ptr<TelemetryManager> manager) {
} // namespace telemetry
} // namespace lldb_private
-
-#endif // LLVM_BUILD_TELEMETRY
diff --git a/lldb/unittests/Core/CMakeLists.txt b/lldb/unittests/Core/CMakeLists.txt
index d4d3764b67ae3..60265f794b5e8 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -1,6 +1,3 @@
-if (LLVM_BUILD_TELEMETRY)
- set(TELEMETRY_DEPS Telemetry)
-endif()
add_lldb_unittest(LLDBCoreTests
CommunicationTest.cpp
@@ -31,5 +28,5 @@ add_lldb_unittest(LLDBCoreTests
LLVMTestingSupport
LINK_COMPONENTS
Support
- ${TELEMETRY_DEPS}
+ Telemetry
)
diff --git a/lldb/unittests/Core/TelemetryTest.cpp b/lldb/unittests/Core/TelemetryTest.cpp
index 3ee6451429619..22fade58660b5 100644
--- a/lldb/unittests/Core/TelemetryTest.cpp
+++ b/lldb/unittests/Core/TelemetryTest.cpp
@@ -5,11 +5,6 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
-
-#include "llvm/Config/llvm-config.h"
-
-#ifdef LLVM_BUILD_TELEMETRY
-
#include "lldb/Core/PluginInterface.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Telemetry.h"
@@ -71,7 +66,13 @@ class FakePlugin : public telemetry::TelemetryManager {
} // namespace lldb_private
-TEST(TelemetryTest, PluginTest) {
+#if LLVM_ENABLE_TELEMETRY
+#define TELEMETRY_TEST(suite, test) TEST(suite, test)
+#else
+#define TELEMETRY_TEST(suite, test) TEST(DISABLED_##suite, test)
+#endif
+
+TELEMETRY_TEST(TelemetryTest, PluginTest) {
// This would have been called by the plugin reg in a "real" plugin
// For tests, we just call it directly.
lldb_private::FakePlugin::Initialize();
@@ -94,5 +95,3 @@ TEST(TelemetryTest, PluginTest) {
ASSERT_EQ("FakeTelemetryPlugin", ins->GetInstanceName());
}
-
-#endif // LLVM_BUILD_TELEMETRY
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index 88512d0f1dd96..9a5cd1d985886 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -835,7 +835,7 @@ option (LLVM_ENABLE_DOXYGEN "Use doxygen to generate llvm API documentation." OF
option (LLVM_ENABLE_SPHINX "Use Sphinx to generate llvm documentation." OFF)
option (LLVM_ENABLE_OCAMLDOC "Build OCaml bindings documentation." ON)
option (LLVM_ENABLE_BINDINGS "Build bindings." ON)
-option (LLVM_BUILD_TELEMETRY "Build the telemetry library. This does not enable telemetry." ON)
+option (LLVM_ENABLE_TELEMETRY "Enable the telemetry library. If set to OFF, library cannot be enabled after build (eg., at runtime)" ON)
set(LLVM_INSTALL_DOXYGEN_HTML_DIR "${CMAKE_INSTALL_DOCDIR}/llvm/doxygen-html"
CACHE STRING "Doxygen-generated HTML documentation install directory")
diff --git a/llvm/cmake/modules/LLVMConfig.cmake.in b/llvm/cmake/modules/LLVMConfig.cmake.in
index 28655ee3ab87d..5ccc66b8039bf 100644
--- a/llvm/cmake/modules/LLVMConfig.cmake.in
+++ b/llvm/cmake/modules/LLVMConfig.cmake.in
@@ -100,7 +100,7 @@ set(LLVM_ENABLE_PIC @LLVM_ENABLE_PIC@)
set(LLVM_BUILD_32_BITS @LLVM_BUILD_32_BITS@)
-set(LLVM_BUILD_TELEMETRY @LLVM_BUILD_TELEMETRY@)
+set(LLVM_ENABLE_TELEMETRY @LLVM_ENABLE_TELEMETRY@)
if (NOT "@LLVM_PTHREAD_LIB@" STREQUAL "")
set(LLVM_PTHREAD_LIB "@LLVM_PTHREAD_LIB@")
diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake b/llvm/include/llvm/Config/llvm-config.h.cmake
index 239f9dd3f38db..4230eb850fa4b 100644
--- a/llvm/include/llvm/Config/llvm-config.h.cmake
+++ b/llvm/include/llvm/Config/llvm-config.h.cmake
@@ -201,7 +201,7 @@
/* Define if logf128 is available */
#cmakedefine LLVM_HAS_LOGF128
-/* Define if building LLVM with LLVM_BUILD_TELEMETRY */
-#cmakedefine LLVM_BUILD_TELEMETRY ${LLVM_BUILD_TELEMETRY}
+/* Define if building LLVM with LLVM_ENABLE_TELEMETRY */
+#cmakedefine01 LLVM_ENABLE_TELEMETRY
#endif
diff --git a/llvm/include/llvm/Telemetry/Telemetry.h b/llvm/include/llvm/Telemetry/Telemetry.h
index 42319f3ef51f2..f8d0fefd8b6a2 100644
--- a/llvm/include/llvm/Telemetry/Telemetry.h
+++ b/llvm/include/llvm/Telemetry/Telemetry.h
@@ -64,11 +64,16 @@ class Serializer {
/// This struct can be extended as needed to add additional configuration
/// points specific to a vendor's implementation.
struct Config {
- virtual ~Config() = default;
+ static constexpr bool BuildTimeEnableTelemetry = LLVM_ENABLE_TELEMETRY;
// If true, telemetry will be enabled.
const bool EnableTelemetry;
- Config(bool E) : EnableTelemetry(E) {}
+
+ explicit Config() : EnableTelemetry(BuildTimeEnableTelemetry) {}
+
+ // Telemetry can only be enabled if both the runtime and buildtime flag
+ // are set.
+ explicit Config(bool E) : EnableTelemetry(E && BuildTimeEnableTelemetry) {}
virtual std::optional<std::string> makeSessionId() { return std::nullopt; }
};
diff --git a/llvm/lib/CMakeLists.txt b/llvm/lib/CMakeLists.txt
index d0a2bc9294381..f6465612d30c0 100644
--- a/llvm/lib/CMakeLists.txt
+++ b/llvm/lib/CMakeLists.txt
@@ -41,9 +41,7 @@ add_subdirectory(ProfileData)
add_subdirectory(Passes)
add_subdirectory(TargetParser)
add_subdirectory(TextAPI)
-if (LLVM_BUILD_TELEMETRY)
- add_subdirectory(Telemetry)
-endif()
+add_subdirectory(Telemetry)
add_subdirectory(ToolDrivers)
add_subdirectory(XRay)
if (LLVM_INCLUDE_TESTS)
diff --git a/llvm/lib/Telemetry/Telemetry.cpp b/llvm/lib/Telemetry/Telemetry.cpp
index d86ad9c1c37bb..caac7e6c332cb 100644
--- a/llvm/lib/Telemetry/Telemetry.cpp
+++ b/llvm/lib/Telemetry/Telemetry.cpp
@@ -21,6 +21,8 @@ void TelemetryInfo::serialize(Serializer &serializer) const {
}
Error Manager::dispatch(TelemetryInfo *Entry) {
+ assert(Config::BuildTimeEnableTelemetry &&
+ "Telemetry should have been enabled");
if (Error Err = preDispatch(Entry))
return Err;
diff --git a/llvm/unittests/CMakeLists.txt b/llvm/unittests/CMakeLists.txt
index 12e229b1c3498..81abce51b8939 100644
--- a/llvm/unittests/CMakeLists.txt
+++ b/llvm/unittests/CMakeLists.txt
@@ -63,9 +63,7 @@ add_subdirectory(Support)
add_subdirectory(TableGen)
add_subdirectory(Target)
add_subdirectory(TargetParser)
-if (LLVM_BUILD_TELEMETRY)
- add_subdirectory(Telemetry)
-endif()
+add_subdirectory(Telemetry)
add_subdirectory(Testing)
add_subdirectory(TextAPI)
add_subdirectory(Transforms)
diff --git a/llvm/unittests/Telemetry/TelemetryTest.cpp b/llvm/unittests/Telemetry/TelemetryTest.cpp
index 6b37e89f16435..c1b9f38bc1eec 100644
--- a/llvm/unittests/Telemetry/TelemetryTest.cpp
+++ b/llvm/unittests/Telemetry/TelemetryTest.cpp
@@ -212,7 +212,13 @@ std::shared_ptr<Config> getTelemetryConfig(const TestContext &Ctxt) {
return std::make_shared<Config>(false);
}
-TEST(TelemetryTest, TelemetryDisabled) {
+#if LLVM_ENABLE_TELEMETRY
+#define TELEMETRY_TEST(suite, test) TEST(suite, test)
+#else
+#define TELEMETRY_TEST(suite, test) TEST(DISABLED_##suite, test)
+#endif
+
+TELEMETRY_TEST(TelemetryTest, TelemetryDisabled) {
TestContext Context;
Context.HasVendorPlugin = false;
@@ -221,7 +227,7 @@ TEST(TelemetryTest, TelemetryDisabled) {
EXPECT_EQ(nullptr, Manager);
}
-TEST(TelemetryTest, TelemetryEnabled) {
+TELEMETRY_TEST(TelemetryTest, TelemetryEnabled) {
const std::string ToolName = "TelemetryTestTool";
// Preset some params.
diff --git a/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn b/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
index 5a13545a15b13..4a7cbe4af9e9b 100644
--- a/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
@@ -292,7 +292,7 @@ write_cmake_config("llvm-config") {
values = [
"LLVM_BUILD_LLVM_DYLIB=",
"LLVM_BUILD_SHARED_LIBS=",
- "LLVM_BUILD_TELEMETRY=",
+ "LLVM_ENABLE_TELEMETRY=",
"LLVM_DEFAULT_TARGET_TRIPLE=$llvm_target_triple",
"LLVM_ENABLE_DUMP=",
"LLVM_ENABLE_HTTPLIB=",
diff --git a/utils/bazel/llvm_configs/llvm-config.h.cmake b/utils/bazel/llvm_configs/llvm-config.h.cmake
index 239f9dd3f38db..4230eb850fa4b 100644
--- a/utils/bazel/llvm_configs/llvm-config.h.cmake
+++ b/utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -201,7 +201,7 @@
/* Define if logf128 is available */
#cmakedefine LLVM_HAS_LOGF128
-/* Define if building LLVM with LLVM_BUILD_TELEMETRY */
-#cmakedefine LLVM_BUILD_TELEMETRY ${LLVM_BUILD_TELEMETRY}
+/* Define if building LLVM with LLVM_ENABLE_TELEMETRY */
+#cmakedefine01 LLVM_ENABLE_TELEMETRY
#endif
More information about the lldb-commits
mailing list