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 <pa...@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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
