[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@labath.sk>

Author: oontvoo

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

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,13 +62,15 @@ 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);
 }
 
 } // namespace telemetry
 } // namespace lldb_private
-
-#endif // LLVM_BUILD_TELEMETRY

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
   )

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

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")

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@")

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

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; }
 };

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)

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;
 

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)

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.

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=",

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