Define -DLLVM_BUILD_TELEMETRY to be used in ifdef

[oontvoo]

Background:

Telemetry code isn't always built (controlled by this LLVM_BUILD_TELEMETRY cmake flag)
This means users of the library may not have the library. So we're definding the -DLLVM_BUILD_TELEMETRY to be used in ifdef.

[labath]

How does this relate to #126715 (which defines the macro in lldb code)?

I think that defining some sort of a macro in llvm code makes sense, but this probably not the way to do that. Docs for add_definitions say (emphasis mine):

Adds definitions to the compiler command line for targets in the current directory, whether added before or after this command is invoked, and for the ones in sub-directories added after.

I.e.,, this creates a very inconsistent state where e.g. XRay has the macro defined, but TextAPI does not. This should probably go to llvm-config.h instead.

[oontvoo]

How does this relate to #126715 (which defines the macro in lldb code)?

I thought it might be better to define the macro at llvm level (because there might be other users of llvm::Telemetry, in which case, we don't want to have them defining different macros to mean the same thing)

[oontvoo]

I.e.,, this creates a very inconsistent state where e.g. XRay has the macro defined, but TextAPI does not. This should probably go to llvm-config.h instead.

Done - added the def to config.h.

[labath]

How does this relate to #126715 (which defines the macro in lldb code)?

I thought it might be better to define the macro at llvm level (because there might be other users of llvm::Telemetry, in which case, we don't want to have them defining different macros to mean the same thing)

Makes sense.

Done - added the def to config.h.

In that case, could you also delete the definition in llvm/lib/CMakeLists.txt and the one introduced in #126715 ?

[mgorny]

Yes, thanks. I'm sorry this didn't occur to me.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Vy Nguyen (oontvoo)

Changes

Background:

Telemetry code isn't always built (controlled by this LLVM_BUILD_TELEMETRY cmake flag)
This means users of the library may not have the library. So we're definding the -DLLVM_BUILD_TELEMETRY to be used in ifdef.

---

Full diff: https://github.com/llvm/llvm-project/pull/126746.diff

3 Files Affected:

• (modified) lldb/source/Core/CMakeLists.txt (-1)
• (modified) llvm/include/llvm/Config/llvm-config.h.cmake (+3)
• (modified) llvm/lib/CMakeLists.txt (+1)

diff --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index cf5f6ac9da4894a..82fb5f42f9f4b9e 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -18,7 +18,6 @@ endif()
 
 if (LLVM_BUILD_TELEMETRY)
    set(TELEMETRY_DEPS Telemetry)
-   add_definitions(-DLLDB_BUILD_TELEMETRY)
 endif()
 
 # TODO: Add property `NO_PLUGIN_DEPENDENCIES` to lldbCore
diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake b/llvm/include/llvm/Config/llvm-config.h.cmake
index 629977cc11d6836..239f9dd3f38db18 100644
--- a/llvm/include/llvm/Config/llvm-config.h.cmake
+++ b/llvm/include/llvm/Config/llvm-config.h.cmake
@@ -201,4 +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}
+
 #endif
diff --git a/llvm/lib/CMakeLists.txt b/llvm/lib/CMakeLists.txt
index d0a2bc929438179..08e3d7ddb8c12c7 100644
--- a/llvm/lib/CMakeLists.txt
+++ b/llvm/lib/CMakeLists.txt
@@ -43,6 +43,7 @@ add_subdirectory(TargetParser)
 add_subdirectory(TextAPI)
 if (LLVM_BUILD_TELEMETRY)
   add_subdirectory(Telemetry)
+  add_definitions(-DLLDB_BUILD_TELEMETRY)
 endif()
 add_subdirectory(ToolDrivers)
 add_subdirectory(XRay)

[oontvoo]

In that case, could you also delete the definition in llvm/lib/CMakeLists.txt and the one introduced in #126715 ?

done

[labath]

.. and fix the code so that it uses the new macro name ? (You'll need to include llvm-config.h before you can use the macro, and I'd recommend doing a build with telemetry disabled to check that everything works as it should)

[oontvoo]

.. and fix the code so that it uses the new macro name ? (You'll need to include llvm-config.h before you can use the macro, and I'd recommend doing a build with telemetry disabled to check that everything works as it should)

Done. (also tested with explicitly specifying -LLVM_BUILD_TELEMETRY=OFF in the cmake command)

[mgorny]

LGTM, thanks!

[llvm-ci]

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building lldb,llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2277

Here is the relevant piece of the build log for the reference

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/38/87' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-14468-38-87.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=87 GTEST_SHARD_INDEX=38 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************